Bug 165598 - [JSC] Module namespace object behaves like immutable prototype exotic object
Summary: [JSC] Module namespace object behaves like immutable prototype exotic object
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-12-08 02:24 PST by Yusuke Suzuki
Modified: 2016-12-10 03:57 PST (History)
5 users (show)

See Also:


Attachments
Patch (3.51 KB, patch)
2016-12-08 02:32 PST, Yusuke Suzuki
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Yusuke Suzuki 2016-12-08 02:32:47 PST
Created attachment 296507 [details]
Patch
Comment 2 Mark Lam 2016-12-08 10:14:54 PST
Comment on attachment 296507 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/JSModuleNamespaceObject.h:38
> +    // https://tc39.github.io/ecma262/#sec-module-namespace-exotic-objects-setprototypeof-v

I think we can do without this comment here.  It's easy enough to trace the addition of this attribute back to the ChangeLog which will give us the url.

> JSTests/modules/namespace-set-prototype-of.js:10
> +import * as namespace from "./namespace-set-prototype-of.js"
> +import { shouldBe, shouldThrow } from "./resources/assert.js";
> +
> +shouldThrow(() => {
> +    Object.setPrototypeOf(namespace, {});
> +}, `TypeError: Cannot set prototype of immutable prototype object`);
> +
> +shouldBe(Reflect.setPrototypeOf(namespace, {}), false);
> +shouldBe(Reflect.setPrototypeOf(namespace, null), true);
> +shouldBe(Object.setPrototypeOf(namespace, null), namespace);

You need also test namespace.__proto__.

Alternatively, you can add namespace as one of the object types to test in LayoutTests/js/script-tests/prototype-assignment.js.  Search for "this.testObject" which tests "{}.__proto__".  You can add a case for "thos.testNamespace" and test a namespace object.  The benefit of going with prototype-assignment.js is that it is more exhaustive in its testing against what is specified in the spec.
Comment 3 Yusuke Suzuki 2016-12-10 03:49:49 PST
Comment on attachment 296507 [details]
Patch

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

Thanks.

>> Source/JavaScriptCore/runtime/JSModuleNamespaceObject.h:38
>> +    // https://tc39.github.io/ecma262/#sec-module-namespace-exotic-objects-setprototypeof-v
> 
> I think we can do without this comment here.  It's easy enough to trace the addition of this attribute back to the ChangeLog which will give us the url.

Dropped.

>> JSTests/modules/namespace-set-prototype-of.js:10
>> +shouldBe(Object.setPrototypeOf(namespace, null), namespace);
> 
> You need also test namespace.__proto__.
> 
> Alternatively, you can add namespace as one of the object types to test in LayoutTests/js/script-tests/prototype-assignment.js.  Search for "this.testObject" which tests "{}.__proto__".  You can add a case for "thos.testNamespace" and test a namespace object.  The benefit of going with prototype-assignment.js is that it is more exhaustive in its testing against what is specified in the spec.

If we would like to test module namespace object, we need to extend the prototype-assignment.js to <script type="module">. And it prevents us from executing this layout test directly in the JSC shell.
So instead, I'll copy the tests from that and put them in JSTests/modules for module namespace object.
Comment 4 Yusuke Suzuki 2016-12-10 03:57:08 PST
Committed r209662: <http://trac.webkit.org/changeset/209662>