Bug 165118

Created attachment 295916 [details] WIP WIP

Created attachment 295949 [details] WIP I implemented the WebAssemblyFunction.cpp, llint-64, VMEntryRecord, and VM.h bits yesterday, and ran some tests. It seems to work on first try! Or rather, it doesn't seem to break JS. To be complete I'm still missing: - llint-32 - WasmB3IRGenerator.cpp is needed to do the wasm→JS transition (that's where test_Instance.js fails: trying to generate code after compileImportStubs) I'm trying to figure out how to handle compileImportStubs exactly. Should it look similar to WebAssemblyFunction (it calls C++ code callWebAssemblyFunction through the jsToWasmEntryPoint that createJSToWasmWrapper generates)? Also, I think going *in* to wasm I want to set Instance on VM, but going out (the code I'm about to write) I want to set it to nullptr (so JS frames have Instance==nullptr inside VMEntryRecord). If I do this then I think both C++ wasm↔JS functions should be next to each other so they don't diverge easily. I'll proceed when I get in to work, but I think the rest could use a review already.

(In reply to comment #2) > Created attachment 295949 [details] > WIP > > I implemented the WebAssemblyFunction.cpp, llint-64, VMEntryRecord, and VM.h > bits yesterday, and ran some tests. It seems to work on first try! Or > rather, it doesn't seem to break JS. > > To be complete I'm still missing: > - llint-32 > - WasmB3IRGenerator.cpp is needed to do the wasm→JS transition (that's > where test_Instance.js fails: trying to generate code after > compileImportStubs) > > I'm trying to figure out how to handle compileImportStubs exactly. Should it > look similar to WebAssemblyFunction (it calls C++ code > callWebAssemblyFunction through the jsToWasmEntryPoint that > createJSToWasmWrapper generates)? Also, I think going *in* to wasm I want to > set Instance on VM, but going out (the code I'm about to write) I want to > set it to nullptr (so JS frames have Instance==nullptr inside > VMEntryRecord). If I do this then I think both C++ wasm↔JS functions should > be next to each other so they don't diverge easily. I'm confused by what you mean here. Doesn't the instance live on VMEntryRecord? If so, it will just go away when we unwind the machine stack naturally. There is code that already resets the VM's vm entry frame pointer > > I'll proceed when I get in to work, but I think the rest could use a review > already.

> > I'm trying to figure out how to handle compileImportStubs exactly. Should it > > look similar to WebAssemblyFunction (it calls C++ code > > callWebAssemblyFunction through the jsToWasmEntryPoint that > > createJSToWasmWrapper generates)? Also, I think going *in* to wasm I want to > > set Instance on VM, but going out (the code I'm about to write) I want to > > set it to nullptr (so JS frames have Instance==nullptr inside > > VMEntryRecord). If I do this then I think both C++ wasm↔JS functions should > > be next to each other so they don't diverge easily. > I'm confused by what you mean here. Doesn't the instance live on > VMEntryRecord? > If so, it will just go away when we unwind the machine stack naturally. > There is code that already resets the VM's vm entry frame pointer I was just thinking that while in JS code you don't really want an instance to be set. It's harmless, but isn't really useful. Doesn't really matter though, given the change of direction we discussed in person.

Created attachment 296014 [details] WIP New direction: this patch keeps JSWebAssemblyInstance on VM, still sets it when entering wasm from JS, but it drops the VMEntryRecord and llint changes. They weren't needed to get the instance or do unwinding.

Created attachment 296328 [details] WIP I think the stub's codegen is mostly correct, but I'm investigating a crash in MacroAssemblerCodeRef::operator= at SC::Wasm::Plan::run (on the return from importStubGenerator). The code in WasmBinding.cpp is starting to be ready for review though.

Comment on attachment 296328 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=296328&action=review > Source/JavaScriptCore/wasm/WasmBinding.cpp:69 > + for (unsigned argNum = 0; argNum != argCount; ++argNum) { Nit: WebKit tends to use < instead of != for loops like this. > Source/JavaScriptCore/wasm/WasmBinding.cpp:94 > + break; Style: Should go inside the block. > Source/JavaScriptCore/wasm/WasmBinding.cpp:102 > + jit.loadFloat(JIT::Address(GPRInfo::callFrameRegister, frOffset), fprReg); Since this is a 32-bit load, do we know if it's stored in the payload or the Tag of the 64-bit call frame slot? > Source/JavaScriptCore/wasm/WasmBinding.cpp:127 > + break; ditto.

Comment on attachment 296328 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=296328&action=review > Source/JavaScriptCore/wasm/WasmBinding.cpp:90 > + jit.or64(JIT::TrustedImm64(TagTypeNumber), gprReg); There is a helper for this that you should use: In AssemblyHelpers.h: void boxInt32(GPRReg intGPR, JSValueRegs boxedRegs, TagRegistersMode mode = HaveTagRegisters) > Source/JavaScriptCore/wasm/WasmBinding.cpp:132 > + GPRReg importJSCellGPRReg = GPRInfo::argumentGPR0; Why not use regT0 and prevent the move below? You can assert regT0 is not callee save just for clarity if you want. > Source/JavaScriptCore/wasm/WasmBinding.cpp:146 > + JIT::Jump slowPath = jit.branchPtrWithPatch(MacroAssembler::NotEqual, importJSCellGPRReg, targetToCheck); Nit: I'd explicitly pass the fourth argument to this function in case anybody decides to change it. Also, it makes this code easier to read b/c we know zero is an invalid cell. > Source/JavaScriptCore/wasm/WasmBinding.cpp:152 > + jit.move(importJSCellGPRReg, GPRInfo::regT0); // Callee needs to be in regT0. > + jit.move(MacroAssembler::TrustedImmPtr(callLinkInfo), GPRInfo::regT2); // Link info needs to be in regT2. > + jit.nearCall(); // Slow call. It looks like you still need to do some stuff w/ the link buffer w/ the CallLinkInfo. > Source/JavaScriptCore/wasm/WasmBinding.cpp:220 > +} This function mostly LGTM. > Source/JavaScriptCore/wasm/WasmCallingConvention.h:46 > +// WebAssembly call frames use these sentinels instead of a JSObject* callee. Nit: JSCell* instead of JSObject*

Comment on attachment 296328 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=296328&action=review > Source/JavaScriptCore/wasm/WasmPlan.cpp:98 > + break; Style: Should be inside the block. > Source/JavaScriptCore/wasm/WasmPlan.h:74 > + Bag<CallLinkInfo>& callLinkInfos() > + { > + RELEASE_ASSERT(!failed()); > + return m_callLinkInfos; > + } Style: I think it's less surprising if we define this as: Bag<CallLinkInfo>&& takeCallLinkInfos() { RELEASE_ASSERT(!failed()); return WTFMove(m_callLinkInfos); } since it's eventually WTFMoved. > Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:82 > + for (unsigned i = 0; i != thisObject->m_numImports; ++i) Style: We tend to use < instead of != in WebKit. > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:77 > + Vector<WriteBarrier<JSCell>> importFunctions; This seems sketchy. This won't be tracked by GC. Maybe used MarkedArgumentBuffer? Or DeferGC around this?

Comment on attachment 296328 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=296328&action=review > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:86 > + JSValue m = importObject->get(state, import.module); > + JSObject* o = jsDynamicCast<JSObject*>(m); Can we pick variable names that are more descriptive in this loop? > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:91 > + JSValue v = o->get(state, import.field); Can't this throw? > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:112 > + importFunctions.uncheckedAppend(WriteBarrier<JSCell>(vm, o, cell)); Why does "o" ensure the lifetime of cell? As I said earlier, Vector<WriteBarrier<>> seems sketchy to me.

Comment on attachment 296328 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=296328&action=review > Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:65 > + memcpy(imports(), importFunctions.data(), m_numImports * sizeof(WriteBarrier<JSCell>)); You need a write barrier on the Instance after doing this for good measure. I don't think you do anything in between construction and now that would GC, but if anything was ever added, you'd need the write barrier. I think it's safer to just have it.

Created attachment 296377 [details] WIP Updated patch which refactors a bunch of our objects: - They mixed their lifetime a bunch, it was weird - The function index space wasn't really A Thing before because we only had internal functions, but now we also have imports I haven't addressed Saam's comments yet. This builds, but crashes in unknown code (FP is trashed, lldb is sad) so I assume it's trying to call the stub. It's late, I'll look tomorrow!

Created attachment 296397 [details] WIP Address all of Saam's comments except in WasmBinding.cpp I added: // FIXME Saam says: It looks like you still need to do some stuff w/ the link buffer w/ the CallLinkInfo. The test_Instance.js test still crashes at the same place, I need to fire up lldb and see why but I'm guessing it's around this generated code since the FP is trashed. This is now much cleaner! (In reply to comment #7) > > Source/JavaScriptCore/wasm/WasmBinding.cpp:102 > > + jit.loadFloat(JIT::Address(GPRInfo::callFrameRegister, frOffset), fprReg); > > Since this is a 32-bit load, do we know if it's stored in the payload or the > Tag of the 64-bit call frame slot? WasmCallingConvention.h seems to treat all offsets as 64-bit, and load from the "bottom". I think what I'm doing here matches up. Keith says it's the payload if he remember correctly. (In reply to comment #9) > > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:77 > > + Vector<WriteBarrier<JSCell>> importFunctions; > > This seems sketchy. This won't be tracked by GC. > Maybe used MarkedArgumentBuffer? Or DeferGC around this? Yeah that was really bad, now that I understand how this works. Good thing you caught it! There's a comment to remove MarkedArgumentBuffer, so I probably should avoid it. DeferGC is kindof a big hammer. I instead moved construction around so that the instance starts with space allocated and zero'd out, and then the WebAssemblyInstanceConstructor sets the WriteBarrier<JSCell> one at a time.

Created attachment 296400 [details] WIP Pass the function index space to B3 and the validator, instead of the internal function signatures (otherwise we'd be checking against the wrong signatures). WasmBinding.cpp still forthcoming.

Comment on attachment 296400 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=296400&action=review > Source/JavaScriptCore/wasm/WasmBinding.cpp:138 > + uint64_t thisArgument = JSValue::JSUndefined; // FIXME what does the WebAssembly spec say this should be? https://bugs.webkit.org/show_bug.cgi?id=165471 This is wrong, should be ValueUndefined.

Comment on attachment 296400 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=296400&action=review Some comments. I'm going to take a high level look at the patch now. > Source/JavaScriptCore/wasm/WasmFormat.h:148 > +struct WasmToJSStub { > + WasmToJSStub(MacroAssemblerCodeRef code) > + : code(code) > + { > + } > + MacroAssemblerCodeRef code; > }; why not just have a typedef? > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:225 > + if (!m_module->imports.tryReserveCapacity(importCount)) // FIXME this over-allocates when we fix the FIXMEs below. > + return false; > + if (!m_module->importFunctions.tryReserveCapacity(importCount)) // FIXME this over-allocates when we fix the FIXMEs below. > + return false; > + if (!m_functionIndexSpace.tryReserveCapacity(importCount)) // FIXME this over-allocates when we fix the FIXMEs below. We'll allocate some more here when we know how many functions to expect. you could probably fuse these into a bunch of || in one if. I think that will be a little easier to read. > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:289 > + if (!m_module->internalFunctionSignatures.tryReserveCapacity(count)) > + return false; > + if (!m_functionLocations.tryReserveCapacity(count)) > + return false; > + if (!m_functionIndexSpace.tryReserveCapacity(m_functionIndexSpace.size() + count)) ditto. > Source/JavaScriptCore/wasm/WasmPlan.cpp:157 > + size_t index = 0; > + for (auto& function : m_wasmInternalFunctions) { > + CodeLocationDataLabelPtr calleeMoveLocation = function->calleeMoveLocation; > + JSWebAssemblyCallee* callee = JSWebAssemblyCallee::create(globalObject->vm(), *function.get()); > > MacroAssembler::repatchPointer(calleeMoveLocation, callee); > > if (verbose) > dataLogLn("Made Wasm callee: ", RawPointer(callee)); > > - callback(i, callee); > + callback(index++, callee); I feel like it's an anti-pattern to use an iterator with an index count that needs to be manually updated on each iteration of the loop. If someone moves code around, all the sudden the loop stops working.

Created attachment 296412 [details] WIP Addressed Keith's comments, and my one comment about jsUndefined being wrong. I added jit.breakpoint() at the start of the stub, looking at what's broken now. Probably linking as Saam was pointing out, since the first call (fast one) is silly right now: Generated JIT code for WebAssembly import[0] stub for signature 0x112651888: Code at [0x4b4344c20920, 0x4b4344c209a0): 0x4b4344c20920: push %rbp 0x4b4344c20921: mov %rsp, %rbp 0x4b4344c20924: int3 0x4b4344c20925: mov $0x0, 0x10(%rbp) 0x4b4344c2092d: mov $0x1, 0x18(%rbp) 0x4b4344c20935: sub $0x30, %rsp 0x4b4344c20939: mov $0xffff000000000000, %r11 0x4b4344c20943: or %r11, %rdi 0x4b4344c20946: mov %rdi, 0x30(%rsp) 0x4b4344c2094b: mov 0x1129f5538, %rax 0x4b4344c20955: mov 0x30(%rax), %rax 0x4b4344c20959: mov %rax, 0x18(%rsp) 0x4b4344c2095e: mov $0x1, 0x20(%rsp) 0x4b4344c20966: mov $0x0, 0x28(%rsp) 0x4b4344c2096f: mov $0x0, %r11 0x4b4344c20979: cmp %r11, %rax 0x4b4344c2097c: jnz 0x4b4344c2098c 0x4b4344c20982: call 0x4b4344c20987 0x4b4344c20987: jmp 0x4b4344c2099b 0x4b4344c2098c: mov $0x112657200, %rdx 0x4b4344c20996: call 0x4b4344c2099b 0x4b4344c2099b: mov %rbp, %rsp 0x4b4344c2099e: pop %rbp 0x4b4344c2099f: ret

(In reply to comment #17) > Created attachment 296412 [details] > WIP > > Addressed Keith's comments, and my one comment about jsUndefined being wrong. > > I added jit.breakpoint() at the start of the stub, looking at what's broken > now. Probably linking as Saam was pointing out, since the first call (fast > one) is silly right now: > > Generated JIT code for WebAssembly import[0] stub for signature 0x112651888: > Code at [0x4b4344c20920, 0x4b4344c209a0): > 0x4b4344c20920: push %rbp > 0x4b4344c20921: mov %rsp, %rbp > 0x4b4344c20924: int3 > 0x4b4344c20925: mov $0x0, 0x10(%rbp) > 0x4b4344c2092d: mov $0x1, 0x18(%rbp) > 0x4b4344c20935: sub $0x30, %rsp > 0x4b4344c20939: mov $0xffff000000000000, %r11 > 0x4b4344c20943: or %r11, %rdi > 0x4b4344c20946: mov %rdi, 0x30(%rsp) > 0x4b4344c2094b: mov 0x1129f5538, %rax > 0x4b4344c20955: mov 0x30(%rax), %rax > 0x4b4344c20959: mov %rax, 0x18(%rsp) > 0x4b4344c2095e: mov $0x1, 0x20(%rsp) > 0x4b4344c20966: mov $0x0, 0x28(%rsp) > 0x4b4344c2096f: mov $0x0, %r11 > 0x4b4344c20979: cmp %r11, %rax > 0x4b4344c2097c: jnz 0x4b4344c2098c > 0x4b4344c20982: call 0x4b4344c20987 Yup, this basically means you forgot to link the call. > 0x4b4344c20987: jmp 0x4b4344c2099b > 0x4b4344c2098c: mov $0x112657200, %rdx > 0x4b4344c20996: call 0x4b4344c2099b > 0x4b4344c2099b: mov %rbp, %rsp > 0x4b4344c2099e: pop %rbp > 0x4b4344c2099f: ret

Created attachment 296419 [details] WIP OK it was linking! It now calls into JS and is sad because it gets a cell as 0x1: * frame #0: 0x00000001001a1530 JavaScriptCore`JSC::JSCell::isObject(this=0x0000000000000001) const + 16 at JSCellInlines.h:174 frame #1: 0x00000001001a14d5 JavaScriptCore`JSC::asObject(cell=0x0000000000000001) + 21 at JSObject.h:1236 frame #2: 0x00000001001a14b0 JavaScriptCore`JSC::asObject(value=JSValue @ 0x00007fff5fbfd5b8) + 32 at JSObject.h:1242 frame #3: 0x00000001001a1482 JavaScriptCore`JSC::Register::object(this=0x00007fff5fbfd978) const + 34 at JSObject.h:1420 frame #4: 0x00000001001a13c9 JavaScriptCore`JSC::ExecState::jsCallee(this=0x00007fff5fbfd960) const + 25 at CallFrame.h:90 frame #5: 0x000000010019f5fd JavaScriptCore`JSC::ExecState::vm(this=0x00007fff5fbfd960) const + 29 at JSCellInlines.h:126 frame #6: 0x0000000100f49ad8 JavaScriptCore`::operationLinkCall(execCallee=0x00007fff5fbfd920, callLinkInfo=0x0000000105069880) + 40 at JITOperations.cpp:872 That's from: // WebAssembly call frames use these sentinels instead of a JSCell* callee. struct CalleeSentinels { static const int wasmToJSStub = 1; }; Which I set on the stub's caller's frame in the stub's prologue: jit.emitFunctionPrologue(); jit.store64(JIT::TrustedImm32(0), JIT::Address(GPRInfo::callFrameRegister, CallFrameSlot::codeBlock * static_cast<int>(sizeof(Register)))); // FIXME Stop using 0 codeBlocks. https://bugs.webkit.org/show_bug.cgi?id=165321 jit.store64(JIT::TrustedImm32(CalleeSentinels::wasmToJSStub), JIT::Address(GPRInfo::callFrameRegister, CallFrameSlot::callee * static_cast<int>(sizeof(Register)))); maybe we do need a real callee instead of a sentinel?

Created attachment 296460 [details] patch for landing Patch for landing, and unblocking Saam+Keith. Follow-up will fix plenty more things.

Comment on attachment 296460 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=296460&action=review r=me with some comments/suggestions > JSTests/wasm/js-api/test_Instance.js:32 > +/* Can you add a FIXME to this? > Source/JavaScriptCore/wasm/WasmBinding.cpp:52 > + jit.breakpoint(); // FIXME make calling to JavaScript work. Please link a bug here. > Source/JavaScriptCore/wasm/WasmFormat.h:111 > +struct FunctionLocations { This should be "FunctionLocation" not "FunctionLocations" > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:366 > + || exp.functionIndex >= m_module->importFunctions.size() + m_module->internalFunctionSignatures.size()) Wouldn't this just be the size of the functionIndexSpace instead of the add? > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:414 > + || functionSize > length() - m_offset) What guarantees this subtraction is sound? > Source/JavaScriptCore/wasm/WasmPlan.cpp:97 > + for (unsigned importIndex = 0; importIndex != m_moduleInformation->imports.size(); ++importIndex) { Style: should be "<" not "!=" > Source/JavaScriptCore/wasm/WasmPlan.cpp:109 > + for (unsigned functionIndex = 0; functionIndex != m_functionLocations.size(); ++functionIndex) { Style: should be "<" not "!=" > Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:57 > + unsigned calleeIndex = functionIndexSpace - importCount(); Please add a debug assert to make sure this subtraction is sound. I know you do this in the caller of this function, but a debug assert here helps future callers not mess up. > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:97 > + RETURN_IF_EXCEPTION(scope, encodedJSValue()); Style nit: Can be: ```RETURN_IF_EXCEPTION(scope, { });``` > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:98 > + JSObject* object = jsDynamicCast<JSObject*>(importModuleValue); Small Nit: FWIW, importModuleValue.isObject() is faster than jsDynamicCast here. > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:104 > + RETURN_IF_EXCEPTION(scope, encodedJSValue()); ditto > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:113 > + if (JSModuleNamespaceObject* importedExports = jsDynamicCast<JSModuleNamespaceObject*>(object)) { I'm not sure why this is testing step "ii". Can you elaborate? Wouldn't the dynamic cast be for WebAssemblyFunction? > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:128 > + break; Style: "break" should go inside the block > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:146 > + break; ditto > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:155 > + break; ditto > Source/JavaScriptCore/wasm/js/WebAssemblyToJSCallee.h:34 > +class WebAssemblyToJSCallee : public JSCell { Nit: Maybe it's worth having "stub" somewhere in the name of this class?

Created attachment 296491 [details] patch Address Saam's comments. Looks like there were some bot issues, I'll look into those. > > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:414 > > + || functionSize > length() - m_offset) > > What guarantees this subtraction is sound? * length() comes from the parser and is the total size of the binary. * m_offset is only updated by the parser, and is the current position in that binary. * All of the parse*() functions check that what they're about to consume doesn't exceed the length, so length is always greater than (or equal!) to the offset. A malicious input can only control functionSize, we therefore cannot add anything to functionSize because it could overflow: m_offset + functionSize < length() This is intuitive to write but is integer-overflowable. > > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:113 > > + if (JSModuleNamespaceObject* importedExports = jsDynamicCast<JSModuleNamespaceObject*>(object)) { > > I'm not sure why this is testing step "ii". Can you elaborate? Wouldn't the > dynamic cast be for WebAssemblyFunction? Good catch! I was checking whether the module was a wasm module, but that's not right. > > Source/JavaScriptCore/wasm/js/WebAssemblyToJSCallee.h:34 > > +class WebAssemblyToJSCallee : public JSCell { > > Nit: Maybe it's worth having "stub" somewhere in the name of this class? We may change it to a thunk if we make it tail-call. I'd rather keep it as-is.

Created attachment 296492 [details] patch Fix build errors on non-cmake.

Comment on attachment 296492 [details] patch Rejecting attachment 296492 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 296492, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in JSTests/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/2649569

Created attachment 296493 [details] patch OOPS I did it again (and this patch undoes it).

Comment on attachment 296493 [details] patch Rejecting attachment 296493 [details] from commit-queue. Number of test failures exceeded the failure limit. Full output: http://webkit-queues.webkit.org/results/2649895

Created attachment 296499 [details] Archive of layout-test-results from webkit-cq-02 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: webkit-cq-02 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 296493 [details] patch Attachment 296493 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2649901 Number of test failures exceeded the failure limit.

Created attachment 296500 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 296493 [details] patch Attachment 296493 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2649887 Number of test failures exceeded the failure limit.

Created attachment 296501 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 296493 [details] patch Attachment 296493 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2649914 Number of test failures exceeded the failure limit.

Created attachment 296502 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 296519 [details] patch Fix lifetime of JSWebAssemblyCallee's std::unique_ptr<> members. IIUC it was the source of this crash: Thread 20 Crashed:: WebCore: Worker 0 com.apple.JavaScriptCore 0x000000010be4e6fa JSC::B3::Compilation::~Compilation() + 10 1 com.apple.JavaScriptCore 0x000000010be054c3 JSC::JSWebAssemblyCallee::destroy(JSC::JSCell*) + 35 2 com.apple.JavaScriptCore 0x000000010be3ff4d JSC::FreeList JSC::MarkedBlock::Handle::specializedSweep<(JSC::MarkedBlock::Handle::EmptyMode)1, (JSC::MarkedBlock::Handle::SweepMode)0, (JSC::MarkedBlock::Handle::SweepDestructionMode)1, (JSC::MarkedBlock::Handle::ScribbleMode)0, (JSC::MarkedBlock::Handle::NewlyAllocatedMode)0, (JSC::MarkedBlock::Handle::MarksMode)1>() + 205 3 com.apple.JavaScriptCore 0x000000010be3f942 JSC::FreeList JSC::MarkedBlock::Handle::sweepHelperSelectSweepMode<(JSC::MarkedBlock::Handle::EmptyMode)1, (JSC::MarkedBlock::Handle::SweepDestructionMode)1, (JSC::MarkedBlock::Handle::ScribbleMode)0, (JSC::MarkedBlock::Handle::NewlyAllocatedMode)0>(JSC::MarkedBlock::Handle::SweepMode) + 146 4 com.apple.JavaScriptCore 0x000000010be3cbc9 JSC::FreeList JSC::MarkedBlock::Handle::sweepHelperSelectEmptyMode<(JSC::MarkedBlock::Handle::SweepDestructionMode)1, (JSC::MarkedBlock::Handle::ScribbleMode)0>(JSC::MarkedBlock::Handle::SweepMode) + 89 5 com.apple.JavaScriptCore 0x000000010be36a56 JSC::MarkedBlock::Handle::sweep(JSC::MarkedBlock::Handle::SweepMode) + 726 6 com.apple.JavaScriptCore 0x000000010be36f2f JSC::MarkedBlock::Handle::lastChanceToFinalize() + 431 7 com.apple.JavaScriptCore 0x000000010be35a9d JSC::MarkedAllocator::lastChanceToFinalize() + 109 8 com.apple.JavaScriptCore 0x000000010be43318 JSC::MarkedSpace::lastChanceToFinalize() + 72 9 com.apple.JavaScriptCore 0x000000010bb4b604 JSC::Heap::lastChanceToFinalize() + 548 10 com.apple.JavaScriptCore 0x000000010c045b9a JSC::VM::~VM() + 266 11 com.apple.JavaScriptCore 0x000000010bd4aad2 JSC::JSLockHolder::~JSLockHolder() + 66 12 com.apple.WebCore 0x000000010e624ee6 WebCore::WorkerScriptController::~WorkerScriptController() + 198 (memory:2655) 13 com.apple.WebCore 0x000000010e627765 WTF::Function<void (WebCore::ScriptExecutionContext&)>::CallableWrapper<WebCore::WorkerThread::stop()::$_0::operator()(WebCore::ScriptExecutionContext&) const::'lambda'(WebCore::ScriptExecutionContext&)>::call(WebCore::ScriptExecutionContext&) + 37 (WorkerScriptController.h:47) 14 com.apple.WebCore 0x000000010e623cd3 WebCore::WorkerRunLoop::runCleanupTasks(WebCore::WorkerGlobalScope*) + 211 (utility:765) 15 com.apple.WebCore 0x000000010e62399f WebCore::WorkerRunLoop::run(WebCore::WorkerGlobalScope*) + 111 (utility:765) 16 com.apple.WebCore 0x000000010e627311 WebCore::WorkerThread::workerThread() + 929 (WorkerThread.cpp:194) 17 com.apple.JavaScriptCore 0x000000010c16f072 WTF::threadEntryPoint(void*) + 178 18 com.apple.JavaScriptCore 0x000000010c16f43f WTF::wtfThreadEntryPoint(void*) + 15 19 libsystem_pthread.dylib 0x00007fff9c3dc05a _pthread_body + 131 20 libsystem_pthread.dylib 0x00007fff9c3dbfd7 _pthread_start + 176 21 libsystem_pthread.dylib 0x00007fff9c3d93ed thread_start + 13

Comment on attachment 296519 [details] patch Attachment 296519 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2654049 Number of test failures exceeded the failure limit.

Created attachment 296528 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 296519 [details] patch Attachment 296519 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2654104 Number of test failures exceeded the failure limit.

Created attachment 296530 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 296519 [details] patch Attachment 296519 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2654108 Number of test failures exceeded the failure limit.

Created attachment 296531 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Created attachment 296543 [details] patch Saam psychic-debugged this to WebAssemblyToJSCallee using the wrong structure (JSWebAssemblyCallee's), before I even had the time to do a build which would repro the issue. My build is still going, but I think this is right so we'll see if the bots agree.

Created attachment 296544 [details] patch Upload the right patch...

Attachment 296544 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyToJSCallee.h:48: The parameter name "structure" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 296544 [details] patch Clearing flags on attachment: 296544 Committed r209560: <http://trac.webkit.org/changeset/209560>

All reviewed patches have been landed. Closing bug.

IIUC the failures are unrelated to my patch? Unexpected flakiness: timeouts (1) media/track/track-in-band-style.html [ Timeout Pass ] Regressions: Unexpected text-only failures (3) fast/css/font_property_normal.html [ Failure ] fast/css/image-set-unprefixed.html [ Failure ] fast/selectors/nth-child-bounds.html [ Failure ] Regressions: Unexpected crashes (1) animations/CSSKeyframesRule-name-null.html [ Crash ]