Bug 172129

Summary: WebAssembly: exports is a getter
Product: WebKit Reporter: JF Bastien <jfbastien>
Component: JavaScriptCoreAssignee: JF Bastien <jfbastien>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fpizlo, jfbastien, keith_miller, mark.lam, msaboff, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 161709    
Attachments:
Description Flags
patch
none
patch
saam: review+, saam: commit-queue-
patch
commit-queue: commit-queue-
patch none

Description JF Bastien 2017-05-15 11:12:20 PDT
Make sure we have the right semantics: https://github.com/WebAssembly/design/pull/1062
Comment 1 JF Bastien 2017-05-17 23:43:10 PDT
Created attachment 310489 [details]
patch
Comment 2 JF Bastien 2017-05-17 23:53:34 PDT
Created attachment 310490 [details]
patch

Rebase.
Comment 3 Saam Barati 2017-05-18 09:25:03 PDT
Comment on attachment 310490 [details]
patch

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

r=me

> JSTests/wasm/js-api/test_basic_api.js:74
> +        assert.throws(() => WebAssembly.Instance.prototype.exports = undefined, TypeError, `Attempted to assign to readonly property.`);

Please add a test where you do getOwnProperty and assert the property descriptor is what you expect. Also, please add tests where you throw because the |this| is not an instance.

> Source/JavaScriptCore/wasm/js/WebAssemblyInstancePrototype.cpp:48
> + exports webAssemblyInstanceProtoFuncExports DontEnum|Accessor 0

Is it a configurable property?
Comment 4 JF Bastien 2017-05-18 10:49:14 PDT
(In reply to Saam Barati from comment #3)
> Comment on attachment 310490 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310490&action=review
> 
> r=me
> 
> > JSTests/wasm/js-api/test_basic_api.js:74
> > +        assert.throws(() => WebAssembly.Instance.prototype.exports = undefined, TypeError, `Attempted to assign to readonly property.`);
> 
> Please add a test where you do getOwnProperty and assert the property
> descriptor is what you expect.

AFAICT a [[Get]] without [[Set]] has getOwnPropertyDescriptor of {"enumerable":false,"configurable":true} does that seem right to you? I'm not super clear on property descriptors, and getters...

None of the accessors specify their own property info.

> Also, please add tests where you throw
> because the |this| is not an instance.

Will do, I tested locally and forgot to add.

> > Source/JavaScriptCore/wasm/js/WebAssemblyInstancePrototype.cpp:48
> > + exports webAssemblyInstanceProtoFuncExports DontEnum|Accessor 0
> 
> Is it a configurable property?
Comment 5 Saam Barati 2017-05-18 10:57:07 PDT
(In reply to JF Bastien from comment #4)
> (In reply to Saam Barati from comment #3)
> > Comment on attachment 310490 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=310490&action=review
> > 
> > r=me
> > 
> > > JSTests/wasm/js-api/test_basic_api.js:74
> > > +        assert.throws(() => WebAssembly.Instance.prototype.exports = undefined, TypeError, `Attempted to assign to readonly property.`);
> > 
> > Please add a test where you do getOwnProperty and assert the property
> > descriptor is what you expect.
> 
> AFAICT a [[Get]] without [[Set]] has getOwnPropertyDescriptor of
> {"enumerable":false,"configurable":true} does that seem right to you? I'm
> not super clear on property descriptors, and getters...
> 
> None of the accessors specify their own property info.
It looks like we should default to configurable:false
https://tc39.github.io/ecma262/#table-4

> 
> > Also, please add tests where you throw
> > because the |this| is not an instance.
> 
> Will do, I tested locally and forgot to add.
> 
> > > Source/JavaScriptCore/wasm/js/WebAssemblyInstancePrototype.cpp:48
> > > + exports webAssemblyInstanceProtoFuncExports DontEnum|Accessor 0
> > 
> > Is it a configurable property?
Comment 6 JF Bastien 2017-05-18 21:42:30 PDT
Created attachment 310607 [details]
patch

From the discussion it sounds like the enumerable / configurable properties are sill up in the air. I forked to an issue:
  https://github.com/WebAssembly/design/issues/1069
I'll create an issue here and fix if things change (for all accessors, not just this one).
Comment 7 WebKit Commit Bot 2017-05-18 21:44:43 PDT
Comment on attachment 310607 [details]
patch

Rejecting attachment 310607 [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-01', 'validate-changelog', '--check-oops', '--non-interactive', 310607, '--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/3773872
Comment 8 JF Bastien 2017-05-18 21:47:52 PDT
Created attachment 310609 [details]
patch

No oops.
Comment 9 WebKit Commit Bot 2017-05-18 22:27:32 PDT
Comment on attachment 310609 [details]
patch

Clearing flags on attachment: 310609

Committed r217097: <http://trac.webkit.org/changeset/217097>
Comment 10 WebKit Commit Bot 2017-05-18 22:27:33 PDT
All reviewed patches have been landed.  Closing bug.