WebAssembly JS API: wire up Instance imports

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[JF Bastien]

Imports should work.

[JF Bastien]

Created attachment 295916 [details]
WIP

WIP

[JF Bastien]

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.

[Saam Barati]

(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.

[JF Bastien]

> > 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.

[JF Bastien]

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.

[JF Bastien]

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.

[Saam Barati]

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.

[Saam Barati]

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*

[Saam Barati]

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?

[Saam Barati]

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.

[Saam Barati]

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.

[JF Bastien]

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!

[JF Bastien]

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.

[JF Bastien]

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.

[JF Bastien]

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.

[Keith Miller]

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.

[JF Bastien]

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

[Saam Barati]

(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

[JF Bastien]

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?

[JF Bastien]

Created attachment 296460 [details]
patch for landing

Patch for landing, and unblocking Saam+Keith. Follow-up will fix plenty more things.

[Saam Barati]

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?

[JF Bastien]

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.

[JF Bastien]

Created attachment 296492 [details]
patch

Fix build errors on non-cmake.

[WebKit Commit Bot]

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

[JF Bastien]

Created attachment 296493 [details]
patch

OOPS I did it again (and this patch undoes it).

[WebKit Commit Bot]

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

[WebKit Commit Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[JF Bastien]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[JF Bastien]

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.

[JF Bastien]

Created attachment 296544 [details]
patch

Upload the right patch...

[WebKit Commit Bot]

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.

[WebKit Commit Bot]

Comment on attachment 296544 [details]
patch

Clearing flags on attachment: 296544

Committed r209560: <http://trac.webkit.org/changeset/209560>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[JF Bastien]

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 ]