1340268 - [[HasProperty]] on module namespace object should work even when binding is uninitialized

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

m1.js:
---
import './m2.js'
---

m2.js:
---
import * as ns from './m2.js';

print('foo' in ns);

export let foo;
---

Start with `mozjs --module ./m1.js`.


Expected: Prints "true"
Actual: Throws ReferenceError
---
././m2.js:3:1 ReferenceError: can't access lexical declaration `foo' before initialization
Stack:
  @././m2.js:3:1
  Reflect.Loader<@shell/ModuleLoader.js:26:16
---

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1340268-namespace-has-uninitialised

Patch to implement hasOwn() in the proxy handler for module namespace objects, rather than relying on the implementation in BaseProxyHandler which calls getOwnPropertyDescriptor() which fails due to the uninitialized binding.

Also fixed an existing bug in has() which meant it didn't work.

I assume that this will be covered by test262 so I don't need to add test code - please let me know if that's not the case.

Comment 2

[André Bargull [:anba]]

(In reply to Jon Coppeard (:jonco) from comment #1)
> Also fixed an existing bug in has() which meant it didn't work.

Oh, I probably should have spotted that in bug 1326453. :-)

But taking a step back, it seems like ModuleNamespaceObject::ProxyHandler::has() will never be called, because BaseProxyHandler::mHasPrototype is `true` for ModuleNamespaceObject::ProxyHandler. That also means ModuleNamespaceObject::ProxyHandler::set() is never called, which is observable in the following test case:

m.js:
---
import* as self from "./m.js";

self.default = 0;

export default null;
---

SpiderMonkey throws |ReferenceError: can't access lexical declaration `default' before initialization|, but according to the spec, the test case should throw a TypeError, because the module namespace object's [[Set]] internal method always returns `false`.

I guess that means ModuleNamespaceObject::ProxyHandler shouldn't set BaseProxyHandler::mHasPrototype to `true`, right?


> 
> I assume that this will be covered by test262 so I don't need to add test
> code - please let me know if that's not the case.

Yes, this is covered by test262 (https://github.com/tc39/test262/blob/master/test/language/module-code/namespace/internals/has-property-str-found-uninit.js). I'm currently working on extending the jstests harness to run module tests, so we should be able to run test262 module tests by next week.

Comment 3

[André Bargull [:anba]]

(In reply to André Bargull from comment #2)
> I'm currently working on extending the jstests harness to run module tests, [...].

Tracked in bug 1340583.

Comment 4

[Tom S [:evilpie]]

Sorry, ignore this if it is irrelevant: If [[Has]] and [[GetOwnProperty]] behave differently, hasOwn (this is SpiderMonkey non-standard optimization, mostly for DOM objectS) needs to behave like [[GetOwnProperty]] and not like [[Has]]. This is because HasOwnProperty in the spec uses [[GetOwnProperty]] internally.

Comment 5

[André Bargull [:anba]]

(In reply to Tom Schuster [:evilpie] from comment #4)
> Sorry, ignore this if it is irrelevant: If [[Has]] and [[GetOwnProperty]]
> behave differently, hasOwn (this is SpiderMonkey non-standard optimization,
> mostly for DOM objectS) needs to behave like [[GetOwnProperty]] and not like
> [[Has]]. This is because HasOwnProperty in the spec uses [[GetOwnProperty]]
> internally.

No, it isn't irrelevant. With the current patch the following test probably (*) prints "true" instead of throwing a ReferenceError.

m.js:
---
import* as self from "./m.js";

print("HasOwn:", Object.prototype.hasOwnProperty.call(self, "default"));

export default null;
---

(*) I haven't actually tested it, but it seems pretty clear given the call hierarchy:
js::Proxy::hasOwn(JSContext *, HandleObject, HandleId, bool *) : bool
-> 	js::HasOwnProperty(JSContext *, HandleObject, HandleId, bool *) : bool
  ->		js::obj_hasOwnProperty(JSContext *, unsigned int, Value *) : bool

Comment 6

[André Bargull [:anba]]

Comment on attachment 8838576 [details] [diff] [review]
bug1340268-namespace-has-uninitialised

Review of attachment 8838576 [details] [diff] [review]:
-----------------------------------------------------------------

r- based on comments #2 and #4-#5. It seems like ModuleNamespaceObject::ProxyHandler needs to set BaseProxyHandler::mHasPrototype to `false` and then we'll need to add the missing proxy handler traps to ModuleNamespaceObject::ProxyHandler.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: bug1340268-namespace-has-uninitialised v2

Setting mHasPrototype worked and was much simpler. With this we pass both tests.

Comment 8

[André Bargull [:anba]]

Comment on attachment 8839182 [details] [diff] [review]
bug1340268-namespace-has-uninitialised v2

Review of attachment 8839182 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!

There's another pre-existing bug in the module namespace handler similar to the bug in has(). The get() handler also doesn't handle symbol properties correctly, so we need to change these two lines https://dxr.mozilla.org/mozilla-central/rev/16effd5b21ab03629feca04b5b83911bb757394c/js/src/builtin/ModuleObject.cpp#459-460 to be `vp.setUndefined(); return true;`. This needs to be fixed before the patch can be checked-in, but I don't think a new review is necessary.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6449178dd01f
[[HasProperty]] on module namespace object should work even when binding is uninitialized r=anba

Comment 10

[André Bargull [:anba]]

(In reply to André Bargull from comment #8)
> There's another pre-existing bug in the module namespace handler similar to
> the bug in has(). The get() handler also doesn't handle symbol properties
> correctly, so we need to change these two lines
> https://dxr.mozilla.org/mozilla-central/rev/
> 16effd5b21ab03629feca04b5b83911bb757394c/js/src/builtin/ModuleObject.cpp#459-
> 460 to be `vp.setUndefined(); return true;`. This needs to be fixed before
> the patch can be checked-in, but I don't think a new review is necessary.

Meh, I should have read the rest of the get() handler, because there's another issue in that method: If the lookup fails in https://dxr.mozilla.org/mozilla-central/rev/16effd5b21ab03629feca04b5b83911bb757394c/js/src/builtin/ModuleObject.cpp#465-466, we also need to return `undefined`.

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6449178dd01f

Comment 12

[André Bargull [:anba]]

(In reply to André Bargull from comment #10)
> Meh, I should have read the rest of the get() handler, because there's
> another issue in that method: If the lookup fails in
> https://dxr.mozilla.org/mozilla-central/rev/
> 16effd5b21ab03629feca04b5b83911bb757394c/js/src/builtin/ModuleObject.cpp#465-
> 466, we also need to return `undefined`.

Filed bug 1341256.