Bug 163404 – WebAssembly JS API: implement basic stub

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[JF Bastien]

Implement and test the basic API, without going out of our way to make it work yet. Parent bug will be used to make each constructor / function work individually.

[JF Bastien]

Created attachment 291545 [details]
patch

Basic WebAssembly global object, its constructor / function properties (all useless for now), test, JSC option (defaults to false).

[Keith Miller]

Comment on attachment 291545 [details]
patch

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

Looks good but you have a couple issues. I think you need to add the getCallData/getConstructData to your Constructors.

> Source/JavaScriptCore/runtime/CommonIdentifiers.h:39
> +    macro(CompileError) \

IIRC you don't need to have the names in both and BuiltinNames.h. If so, I think we should try to reduce the number of common identifiers we have as they get included everywhere.

> Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:154
> +            m_ ## NAME ## Structure.set(vm, this, NAME ## Structure); \
> +        } \
> +        \
> +    private: \
> +        WebAssembly ## NAME ## Constructor(VM& vm, Structure* structure) \
> +            : InternalFunction(vm, structure) \
> +        { \
> +        } \
> +    \
> +    WriteBarrier<Structure> m_ ## NAME ## Structure; \

I don't think this will work because your structure will get GCed. You need to add a visitChildren method to ensure that the GC knows about these structures. In the future, I think you should just put the structure on the global object.

[JF Bastien]

Created attachment 291688 [details]
patch

Address review comments:
 - Implement getCallData/getConstructData/visitChildren, and add related tests (call function properties and assert that they throw, invoke constructor properties and assert that they also throw).
 - Don't use common identifiers, only use builtin names.

Also, merge with the change which just landed in https://bugs.webkit.org/show_bug.cgi?id=163267

I think this is now good to go!

[Keith Miller]

(In reply to comment #3)
> Created attachment 291688 [details]
> patch
> 
> Address review comments:
>  - Implement getCallData/getConstructData/visitChildren, and add related
> tests (call function properties and assert that they throw, invoke
> constructor properties and assert that they also throw).
>  - Don't use common identifiers, only use builtin names.
> 
> Also, merge with the change which just landed in
> https://bugs.webkit.org/show_bug.cgi?id=163267
> 
> I think this is now good to go!

Your patch looks a little light :P. I think you forgot to include some changes or something.

[JF Bastien]

(In reply to comment #4)
> (In reply to comment #3)
> > Created attachment 291688 [details]
> > patch
> > 
> > Address review comments:
> >  - Implement getCallData/getConstructData/visitChildren, and add related
> > tests (call function properties and assert that they throw, invoke
> > constructor properties and assert that they also throw).
> >  - Don't use common identifiers, only use builtin names.
> > 
> > Also, merge with the change which just landed in
> > https://bugs.webkit.org/show_bug.cgi?id=163267
> > 
> > I think this is now good to go!
> 
> Your patch looks a little light :P. I think you forgot to include some
> changes or something.

No that's all of it.

[JF Bastien]

Created attachment 291705 [details]
patch

MSVC bot was sad at zero-sized static array. make it happy by having at least one value in arrays (and initialize one of them which ins't default-initializable because the union is being silly). They're not read so their content doesn't matter (and they'll eventually be auto-generated when I implement them).

[JF Bastien]

Created attachment 291720 [details]
patch

Fixing MSVC bot made GTK bot unhappy because it's being silly about initialization of POD static globals (they should default init, GTK bot thinks a warning is cool). Whack that mole.

[Keith Miller]

Comment on attachment 291720 [details]
patch

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

r=me with some comments.

> Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:53
> +// FIXME https://bugs.webkit.org/show_bug.cgi?id=161709 Implement each of these in their own header file, and autogenerate *.lut.h files for *PrototypeTable.

nit: I think we usually put the url after the comment.

> Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:55
> +    class WebAssembly ## NAME : public JSDestructibleObject { \

Normally we would call this JSWebAssembly ## NAME.

The typical convention is JS ## NAME for instance objects and NAME ## Protototype / NAME ## Constructor for prototypes / constructors, respectively.

> Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:83
> +    const ClassInfo WebAssembly ## NAME::s_info = { "Object", &Base::s_info, 0, CREATE_METHOD_TABLE(WebAssembly ## NAME) }; \

"Object" should be something to identify this object to a JSC user. Perhaps "WebAssembly ## NAME". That string is only used by the inspector to produce nicely formatted names for each object it sees.

> Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:85
> +    class WebAssembly ## NAME ## Prototype : public WebAssembly ## NAME { \

Why does this subclass the instance object? Modern JS has prototypes act as normal objects with a few exceptions for Array.prototype, etc.

> Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:118
> +    const ClassInfo WebAssembly ## NAME ## Prototype::s_info = { "Object", &WebAssembly ## NAME::s_info, &NAME ## PrototypeTable, CREATE_METHOD_TABLE(WebAssembly ## NAME ## Prototype) }; \

Ditto.

> Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:159
> +            putDirectWithoutTransition(vm, vm.propertyNames->length, jsNumber(0), ReadOnly | DontEnum | DontDelete); \

Should this be functionLength and not 0?

[JF Bastien]

Created attachment 291831 [details]
patch

(In reply to comment #8)
> Comment on attachment 291720 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291720&action=review
> 
> r=me with some comments.
> 
> > Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:53
> > +// FIXME https://bugs.webkit.org/show_bug.cgi?id=161709 Implement each of these in their own header file, and autogenerate *.lut.h files for *PrototypeTable.
> 
> nit: I think we usually put the url after the comment.

Done.


> > Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:55
> > +    class WebAssembly ## NAME : public JSDestructibleObject { \
> 
> Normally we would call this JSWebAssembly ## NAME.
> 
> The typical convention is JS ## NAME for instance objects and NAME ##
> Protototype / NAME ## Constructor for prototypes / constructors,
> respectively.

I followed what IntlObject and IntlCollator do. Which is "right"?


> > Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:83
> > +    const ClassInfo WebAssembly ## NAME::s_info = { "Object", &Base::s_info, 0, CREATE_METHOD_TABLE(WebAssembly ## NAME) }; \
> 
> "Object" should be something to identify this object to a JSC user. Perhaps
> "WebAssembly ## NAME". That string is only used by the inspector to produce
> nicely formatted names for each object it sees.

Same here, IntlCollator (and all other Intl constructors) use "Object". Are they wrong? I can fix them as a separate patch.


> > Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:85
> > +    class WebAssembly ## NAME ## Prototype : public WebAssembly ## NAME { \
> 
> Why does this subclass the instance object? Modern JS has prototypes act as
> normal objects with a few exceptions for Array.prototype, etc.

Same here: that's what IntlCollatorPrototype does. I don't know what's right, let me know which to go for and I can separately fix Intl* if they're also wrong.



> > Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:118
> > +    const ClassInfo WebAssembly ## NAME ## Prototype::s_info = { "Object", &WebAssembly ## NAME::s_info, &NAME ## PrototypeTable, CREATE_METHOD_TABLE(WebAssembly ## NAME ## Prototype) }; \
> 
> Ditto.

Ditto².


> > Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:159
> > +            putDirectWithoutTransition(vm, vm.propertyNames->length, jsNumber(0), ReadOnly | DontEnum | DontDelete); \
> 
> Should this be functionLength and not 0?

Yeah I think you're right.

[JF Bastien]

Created attachment 291836 [details]
patch

We discussed this offline: Intl is wrong, so I updated WebAssembly as suggested.

[Saam Barati]

Comment on attachment 291836 [details]
patch

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

> Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:55
> +    class JSWebAssembly ## NAME : public JSDestructibleObject { \

Is the plan to add fields to these objects that will require destruction? If not, this can just inherit from JSNonFinalObject.

[JF Bastien]

(In reply to comment #11)
> Comment on attachment 291836 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291836&action=review
> 
> > Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:55
> > +    class JSWebAssembly ## NAME : public JSDestructibleObject { \
> 
> Is the plan to add fields to these objects that will require destruction? If
> not, this can just inherit from JSNonFinalObject.

Im not 100% sure I understand the semantics, but I believe so: WebAssembly's Module / Instance / Memory / Table would require destructor action I think. The two error types won't, and should do as you suggest.

[WebKit Commit Bot]

Comment on attachment 291836 [details]
patch

Clearing flags on attachment: 291836

Committed r207432: <http://trac.webkit.org/changeset/207432>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.