978235 - ES6 Proxies: Implement [[IsExtensible]] trap

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Eric Faust [:efaust]]

We haven't got one. We need one. We should do that. Probably the JSObject::isExtensible() sites should be audited, but this shouldn't be hard.

Also wants tests.

Comment 1

[Eric Faust [:efaust]]

Attachment: Implement [[IsExtensible]]

Trap implementation with rudimentary test.

Comment 2

[Eric Faust [:efaust]]

perhaps we should use JSMSG_INVALID_TRAP_RESULT instead?

Comment 3

[Eric Faust [:efaust]]

Attachment: v2 - clean up test

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 8387210 [details] [diff] [review]
v2 - clean up test

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

::: js/src/jsproxy.cpp
@@ +2227,5 @@
>      // steps g-n are shared
>      return ArrayToIdVector(cx, proxy, target, trapResult, props, JSITER_OWNONLY, cx->names().keys);
>  }
>  
> +// ES6 5.9.3 Proxy.[[IsExtensible]](P)

Add the ES6 revision number you're talking about please. (The latest HTML is 20 Jan 2014, but there's a newer PDF dated 5 April.)

The section is 9.5.3 not 5.9.3.

@@ +2231,5 @@
> +// ES6 5.9.3 Proxy.[[IsExtensible]](P)
> +bool
> +ScriptedDirectProxyHandler::isExtensible(JSContext *cx, HandleObject proxy, bool *extensible)
> +{
> +    RootedObject handler(cx, GetDirectProxyHandlerObject(proxy));

Pre-existing, but the body of GetDirectProxyHandlerObject is puzzling to me. Seems like that should be a method of ProxyObject, or some subclass, and this should say

    proxy->as<ProxyObject>().handler()

At the very least it should assert that this is a scripted proxy. Right? Want to fix that?

@@ +2240,5 @@
> +    if (!JSObject::getProperty(cx, handler, handler, cx->names().isExtensible, &trap))
> +        return false;
> +
> +    if (trap.isUndefined())
> +        return DirectProxyHandler::isExtensible(cx, proxy, extensible);

It's more direct and more similar to the spec to say:

        return JSObject::isExtensible(cx, target, extensible);

::: js/src/tests/ecma_6/Proxy/proxy-isExtensible.js
@@ +1,2 @@
> +// Test ES6 Proxy trap compliance for Object.isExtensible() on exotic proxy
> +// objects.

Good test. Please also test:

- the target is extensible but then the hook does Object.preventExtensions(target) and returns true / false
- the hook returns something other than a boolean
- the hook throws
- the hook can call Object.isExtensible(proxy) recursively
- if the target is a proxy, *its* isExtensible hook is called
- Object.isFrozen/isSealed properly trigger the isExtensible hook
  - and can cope if it throws

Comment 5

[Jason Orendorff [:jorendorff]]

(In reply to Eric Faust [:efaust] from comment #2)
> perhaps we should use JSMSG_INVALID_TRAP_RESULT instead?

Nah, what you've got is fine.

Comment 6

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c70626e77e9f

Comment 7

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f75f603b27e4 since I didn't have any guess about which of your patches (if either) would have caused the https://tbpl.mozilla.org/php/getParsedLog.php?id=37794837&tree=Mozilla-Inbound assertion.

Comment 8

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1660c847ca2

Relanded. This was the faulty one. Thanks to terrence for the quick pastebin rs on the ipc auto compartment.

Comment 9

[Eric Faust [:efaust]]

Oh boy, that was fun. The last commit contained such wonderful lines as |false &&| in the condition for IsIonEnabled(). Backed out here: http://hg.mozilla.org/integration/mozilla-inbound/rev/f4929fbf4ea1

Relanded without that nonsense here:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5259b79d0c3f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0761b87e8509

Comment 10

[Eric Faust [:efaust]]

how fitting, I forgot the test. Boy, this has all gone exactly to plan. Let me tell you....

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c3a28d61f75

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5259b79d0c3f
https://hg.mozilla.org/mozilla-central/rev/0761b87e8509
https://hg.mozilla.org/mozilla-central/rev/4c3a28d61f75