ES6 Proxies: Implement [[IsExtensible]] trap

RESOLVED FIXED in mozilla31

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: efaust, Assigned: efaust)

Tracking

unspecified
mozilla31
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:p1])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
Component: JavaScript Engine → JavaScript: Standard Library
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [js:p1]
(Assignee)

Comment 1

5 years ago
Posted patch Implement [[IsExtensible]] (obsolete) — Splinter Review
Trap implementation with rudimentary test.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8386427 - Flags: review?(jorendorff)
(Assignee)

Comment 2

5 years ago
perhaps we should use JSMSG_INVALID_TRAP_RESULT instead?
(Assignee)

Comment 3

5 years ago
Attachment #8386427 - Attachment is obsolete: true
Attachment #8386427 - Flags: review?(jorendorff)
Attachment #8387210 - Flags: review?(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
Attachment #8387210 - Flags: review?(jorendorff) → review+
(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.
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.
(Assignee)

Comment 8

5 years ago
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.
(Assignee)

Comment 9

5 years ago
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
(Assignee)

Comment 10

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/5259b79d0c3f
https://hg.mozilla.org/mozilla-central/rev/0761b87e8509
https://hg.mozilla.org/mozilla-central/rev/4c3a28d61f75
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.