Eric Faust [:efaust] Assignee
	Description • 10 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.

	Eric Faust [:efaust] Assignee
	Comment 1 • 10 years ago

Trap implementation with rudimentary test.

	Eric Faust [:efaust] Assignee
	Comment 2 • 10 years ago

perhaps we should use JSMSG_INVALID_TRAP_RESULT instead?

	Jason Orendorff [:jorendorff]
	Comment 4 • 10 years ago

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

	Jason Orendorff [:jorendorff]
	Comment 5 • 10 years ago

(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.

	Eric Faust [:efaust] Assignee
	Comment 6 • 10 years ago

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

	Phil Ringnalda (:philor)
	Comment 7 • 10 years ago

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.

	Eric Faust [:efaust] Assignee
	Comment 8 • 10 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.

	Eric Faust [:efaust] Assignee
	Comment 9 • 10 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

	Eric Faust [:efaust] Assignee
	Comment 10 • 10 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

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 10 years ago

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