ES6 Proxies: Implement [[IsExtensible]] trap

RESOLVED FIXED in mozilla31



5 years ago
5 years ago


(Reporter: efaust, Assigned: efaust)


Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [js:p1])


(1 attachment, 1 obsolete attachment)



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]

Comment 1

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

Comment 2

5 years ago
perhaps we should use JSMSG_INVALID_TRAP_RESULT instead?

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


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 since I didn't have any guess about which of your patches (if either) would have caused the assertion.

Comment 8

5 years ago

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

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:

Relanded without that nonsense here:

Comment 10

5 years ago
how fitting, I forgot the test. Boy, this has all gone exactly to plan. Let me tell you....
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.