Bug 172129

Created attachment 310489 [details] patch

Created attachment 310490 [details] patch Rebase.

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?

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

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

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

Created attachment 310609 [details] patch No oops.

Comment on attachment 310609 [details] patch Clearing flags on attachment: 310609 Committed r217097: <http://trac.webkit.org/changeset/217097>

All reviewed patches have been landed. Closing bug.