Bug 165121 - WebAssembly JS API: export a module namespace object instead of a module environment
Summary: WebAssembly JS API: export a module namespace object instead of a module envi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: JF Bastien
URL:
Keywords:
Depends on: 164757
Blocks: 161709 169951
  Show dependency treegraph
 
Reported: 2016-11-28 16:03 PST by JF Bastien
Modified: 2017-03-22 01:36 PDT (History)
7 users (show)

See Also:


Attachments
Patch (16.45 KB, patch)
2016-11-30 02:15 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (17.76 KB, patch)
2016-11-30 02:18 PST, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description JF Bastien 2016-11-28 16:03:14 PST
JSWebAssemblyInstance::finishCreation should do this:
    putDirect(vm, Identifier::fromString(&vm, "exports"), JSValue(m_moduleNamespaceObject.get()), None);
Not its constructor.

These tests should pass in test_basic_api.js:
        assert.isUndef(instance.exports.__proto__);
        assert.eq(Reflect.isExtensible(instance.exports), false);
        assert.eq(Symbol.iterator in instance.exports, true);
        assert.eq(Symbol.toStringTag in instance.exports, true);

WebAssembly doesn't have circular linking and fancy things yet, so it's slightly simpler than ES6 modules. I'm sure this isn't super hard to fix, but the current code does what we need to get off the ground so I'd rather figure it out later.
Comment 1 Yusuke Suzuki 2016-11-30 01:49:21 PST
I'll look into it.
Comment 2 Yusuke Suzuki 2016-11-30 02:15:28 PST
Created attachment 295708 [details]
Patch
Comment 3 Yusuke Suzuki 2016-11-30 02:18:56 PST
Created attachment 295709 [details]
Patch
Comment 4 Yusuke Suzuki 2016-11-30 02:21:13 PST
Comment on attachment 295709 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295709&action=review

> Source/JavaScriptCore/wasm/WasmFormat.cpp:37
> +#endif // COMPILER(GCC) && ASSERT_DISABLED

This is B3 way for the following switch's warning.
Comment 5 JF Bastien 2016-11-30 10:11:38 PST
Comment on attachment 295709 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295709&action=review

Awesome, this was way simpler than I thought it was! :-)
Looks good.

>> Source/JavaScriptCore/wasm/WasmFormat.cpp:37
>> +#endif // COMPILER(GCC) && ASSERT_DISABLED
> 
> This is B3 way for the following switch's warning.

I'm auto-generating that code here: https://bugs.webkit.org/show_bug.cgi?id=164724
Comment 6 Yusuke Suzuki 2016-11-30 19:09:17 PST
Comment on attachment 295709 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295709&action=review

>>> Source/JavaScriptCore/wasm/WasmFormat.cpp:37
>>> +#endif // COMPILER(GCC) && ASSERT_DISABLED
>> 
>> This is B3 way for the following switch's warning.
> 
> I'm auto-generating that code here: https://bugs.webkit.org/show_bug.cgi?id=164724

Nice. Once the above patch is landed, we can simply drop this!
Comment 7 Saam Barati 2016-11-30 19:31:50 PST
Comment on attachment 295709 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295709&action=review

r=me

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:36
> +#include "LLIntThunks.h"

Was this file needed?
Comment 8 Yusuke Suzuki 2016-11-30 19:52:18 PST
Comment on attachment 295709 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295709&action=review

Thanks!

>> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:36
>> +#include "LLIntThunks.h"
> 
> Was this file needed?

It is necessary for `vmEntryToWasm`.
Comment 9 Yusuke Suzuki 2016-11-30 19:54:48 PST
Committed r209171: <http://trac.webkit.org/changeset/209171>