Closed Bug 978235 Opened 10 years ago Closed 10 years ago

ES6 Proxies: Implement [[IsExtensible]] trap


(Core :: JavaScript: Standard Library, defect)

Not set





(Reporter: efaust, Assigned: efaust)



(Whiteboard: [js:p1])


(1 file, 1 obsolete file)

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]
Attached patch Implement [[IsExtensible]] (obsolete) — Splinter Review
Trap implementation with rudimentary test.
Assignee: nobody → efaustbmo
Attachment #8386427 - Flags: review?(jorendorff)
perhaps we should use JSMSG_INVALID_TRAP_RESULT instead?
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.

Relanded. This was the faulty one. Thanks to terrence for the quick pastebin rs on the ipc auto compartment.
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:
how fitting, I forgot the test. Boy, this has all gone exactly to plan. Let me tell you....
Closed: 10 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.