978279 - ES6 Proxies: Implement revocation semantics

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Eric Faust [:efaust]]

At present we have three identical functions called Revoke(), all of which have the same body, and two of which have the same arguments, but in different orders. I kid you not.

We have to implement revocation anyways, so this should be fixable.

Comment 1

[Eric Faust [:efaust]]

Attachment: proxyRevoke.patch

OK, so this patch does a few things

First, it does not address the Revoke() problem in jsobj.cpp, because it's not relevant

Second, it does basically touch every proxy test we have. I have taken the liberty of deleting a few dupicates, and beautifying a bunch of tests while I was updating them.

Jason, you and I discussed a bunch of things to test. Most of thoes are in the new testDirectProxyRevoke.js

I'm sure I've forgotten something. I am more than open to new ideas for things to test.

Comment 2

[Eric Faust [:efaust]]

Comment on attachment 8449838 [details] [diff] [review]
proxyRevoke.patch

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

::: js/src/jit-test/tests/proxy/testDirectProxyHas1.js
@@ +9,5 @@
>          configurable: true
>      }
> +});
> +
> +for (let p of [Proxy(target, {}), Proxy.revocable(target, {}).proxy]) {

|new Proxy|

::: js/src/jit-test/tests/proxy/testDirectProxyHasOwnProperty.js
@@ +11,5 @@
>  };
>  descs[Symbol.for("quux")] = {configurable: true};
>  var target = Object.create(proto, descs);
> +
> +for (let p of [Proxy(target, {}), Proxy.revocable(target, {}).proxy]) {

|new Proxy|

::: js/src/jsproxy.cpp
@@ +3011,5 @@
> +
> +static bool
> +proxy_revocable(JSContext *cx, unsigned argc, Value *vp)
> +{
> +    CallReceiver args = CallArgsFromVp(argc, vp);

CallReceiverFromVp

Comment 3

[Eric Faust [:efaust]]

I guess the revoke test should assert that the return value is undefined both on the first and on a second call.

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 8449838 [details] [diff] [review]
proxyRevoke.patch

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

Phew. Thanks for all the work on the tests.

::: js/src/jit-test/tests/proxy/testDirectProxyApply4.js
@@ +1,2 @@
> +load(libdir + "asserts.js");
> +// Revoked proxies should throw before calling the handler

Blah. It sucks having load() on the first line and a useful comment on the second line. Then you can't use head -n1 on a bunch of files to see which one you want. But it's always been like this in this directory, so whatever.

::: js/src/jit-test/tests/proxy/testDirectProxyConstruct3.js
@@ +1,3 @@
>  // Return the trap result
> +function setFoo(x,y) { this.foo = x + y; }
> +var handler = { construct: function (target, args) { return { foo : args[0] * args[1]}; } }

I liked it better on multiple lines :) but as you like it.

::: js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyDescriptor10.js
@@ +1,1 @@
> +// Return a new descriptor object that aggress with that returned by the trap

spelling nit: "agrees"

::: js/src/jit-test/tests/proxy/testDirectProxyIsExtensible1.js
@@ +22,5 @@
> +                                   TypeError, "Must throw if handler and target disagree.");
> +        else
> +            assertEq(Object.isExtensible(p), expectedResult, "Must return the correct value.");
> +        if (shouldCallHandler)
> +            assertEq(handlerCalled, true, "Must call handler trap if present");

It's only one-way? We can't assertEq(handlerCalled, shouldCallHandler)?

@@ +70,5 @@
>  
>  function truthyNonBool() { handlerCalled = true; return {}; }
>  handler.isExtensible = truthyNonBool;
> +testExtensible(sealed, true, true);
> +testExtensible(unsealed, true, false, true);

All this made my eyes cross. Don't bother fixing, but I wish the original author had designed a not-quite-so-boolean-happy test function:

    testExtensible(sealed, false, {shouldCallHandler: false});  // expect Object.isExtensible(p) to return false
                                                                // and not set handlerCalled
    testExtensible(unsealed, true);  // expect it to return true (and set handlerCalled, by default)
    testExtensible(targetSealed, TypeError);  // expect it to throw a TypeError (and set handlerCalled, by default)

::: js/src/js.msg
@@ +440,5 @@
>  MSG_DEF(JSMSG_PROXY_EXTENSIBILITY,      386, 0, JSEXN_TYPEERR, "proxy must report same extensiblitity as target")
>  MSG_DEF(JSMSG_PROXY_CONSTRUCT_OBJECT,   387, 0, JSEXN_TYPEERR, "proxy [[Construct]] must return an object")
>  MSG_DEF(JSMSG_PROXY_GETOWN_OBJORUNDEF,  388, 0, JSEXN_TYPEERR, "proxy [[GetOwnProperty]] must return an object or undefined")
>  MSG_DEF(JSMSG_CANT_REPORT_C_AS_NC,      389, 0, JSEXN_TYPEERR, "proxy can't report existing configurable property as non-configurable")
> +MSG_DEF(JSMSG_PROXY_REVOKED,            390, 0, JSEXN_TYPEERR, "Illegal operation attempted on revoked proxy")

In general, error messages start with a lowercase letter.

The ideal error message here is like, "{0} is a revoked proxy". Is that possible?

::: js/src/jsproxy.cpp
@@ +1090,5 @@
>  
>      static ScriptedDirectProxyHandler singleton;
> +
> +    static const int HANDLER_EXTRA;
> +    static const int REVOKE_SLOT;

These need brief comments.

You can add the '= 0' here and kill the definitions below. (The C++ standard has a totally special rule permitting this exact thing. See js::types::TypeObject::LAZY_SINGLETON for an example of a 'static const int' defined this way.)

@@ +3020,5 @@
> +    RootedValue proxyVal(cx, args.rval());
> +    MOZ_ASSERT(proxyVal.toObject().is<ProxyObject>());
> +
> +    RootedObject revoker(cx, NewFunctionWithReserved(cx, RevokeProxy, 0, 0, cx->global(),
> +    "RevokeProxy"));

Fix the indentation here.

Also I think it's OK to use NewFunctionByIdWithReserved here, using AtomToId(cx->names().revoke) for the id.

@@ +3022,5 @@
> +
> +    RootedObject revoker(cx, NewFunctionWithReserved(cx, RevokeProxy, 0, 0, cx->global(),
> +    "RevokeProxy"));
> +    if (!revoker) {
> +        js_ReportOutOfMemory(cx);

Remove the js_ReportOutOfMemory calls here and after NewBuiltinClassInstance below. Those functions already call it if needed.

::: js/src/vm/ProxyObject.cpp
@@ +70,5 @@
>  
>  void
> +ProxyObject::setSameCompartmentPrivate(const Value &priv)
> +{
> +    setSlot(PRIVATE_SLOT, priv);

Consider writing it like this:

void
ProxyObject::revoke()
{
    MOZ_ASSERT(handler() == &ScriptedDirectProxyHandler::singleton);
    setSlot(PRIVATE_SLOT, NullValue());  // clear the target
    setExtra(ScriptedDirectProxyHandler::HANDLER_EXTRA, NullValue());
}

This seems like a more natural API for ProxyObject to present, and it might make it easier to switch implementations to khuey-style later.

Comment 5

[Eric Faust [:efaust]]

(In reply to Jason Orendorff [:jorendorff] from comment #4)
> Comment on attachment 8449838 [details] [diff] [review]
> proxyRevoke.patch
> 
> Review of attachment 8449838 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Phew. Thanks for all the work on the tests.
> 
> ::: js/src/jit-test/tests/proxy/testDirectProxyApply4.js
> @@ +1,2 @@
> > +load(libdir + "asserts.js");
> > +// Revoked proxies should throw before calling the handler
> 
> Blah. It sucks having load() on the first line and a useful comment on the
> second line. Then you can't use head -n1 on a bunch of files to see which
> one you want. But it's always been like this in this directory, so whatever.
> 
> ::: js/src/jit-test/tests/proxy/testDirectProxyConstruct3.js
> @@ +1,3 @@
> >  // Return the trap result
> > +function setFoo(x,y) { this.foo = x + y; }
> > +var handler = { construct: function (target, args) { return { foo : args[0] * args[1]}; } }
> 
> I liked it better on multiple lines :) but as you like it.
> 
> ::: js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyDescriptor10.js
> @@ +1,1 @@
> > +// Return a new descriptor object that aggress with that returned by the trap
> 
> spelling nit: "agrees"
> 
> ::: js/src/jit-test/tests/proxy/testDirectProxyIsExtensible1.js
> @@ +22,5 @@
> > +                                   TypeError, "Must throw if handler and target disagree.");
> > +        else
> > +            assertEq(Object.isExtensible(p), expectedResult, "Must return the correct value.");
> > +        if (shouldCallHandler)
> > +            assertEq(handlerCalled, true, "Must call handler trap if present");
> 
> It's only one-way? We can't assertEq(handlerCalled, shouldCallHandler)?
> 
> @@ +70,5 @@
> >  
> >  function truthyNonBool() { handlerCalled = true; return {}; }
> >  handler.isExtensible = truthyNonBool;
> > +testExtensible(sealed, true, true);
> > +testExtensible(unsealed, true, false, true);
> 
> All this made my eyes cross. Don't bother fixing, but I wish the original
> author had designed a not-quite-so-boolean-happy test function:
> 
>     testExtensible(sealed, false, {shouldCallHandler: false});  // expect
> Object.isExtensible(p) to return false
>                                                                 // and not
> set handlerCalled
>     testExtensible(unsealed, true);  // expect it to return true (and set
> handlerCalled, by default)
>     testExtensible(targetSealed, TypeError);  // expect it to throw a
> TypeError (and set handlerCalled, by default)
> 
> ::: js/src/js.msg
> @@ +440,5 @@
> >  MSG_DEF(JSMSG_PROXY_EXTENSIBILITY,      386, 0, JSEXN_TYPEERR, "proxy must report same extensiblitity as target")
> >  MSG_DEF(JSMSG_PROXY_CONSTRUCT_OBJECT,   387, 0, JSEXN_TYPEERR, "proxy [[Construct]] must return an object")
> >  MSG_DEF(JSMSG_PROXY_GETOWN_OBJORUNDEF,  388, 0, JSEXN_TYPEERR, "proxy [[GetOwnProperty]] must return an object or undefined")
> >  MSG_DEF(JSMSG_CANT_REPORT_C_AS_NC,      389, 0, JSEXN_TYPEERR, "proxy can't report existing configurable property as non-configurable")
> > +MSG_DEF(JSMSG_PROXY_REVOKED,            390, 0, JSEXN_TYPEERR, "Illegal operation attempted on revoked proxy")
> 
> In general, error messages start with a lowercase letter.
> 
> The ideal error message here is like, "{0} is a revoked proxy". Is that
> possible?
> 

I loooked into that. As we discussed on IRC, various intricacies of our value decompilation scheme cause it to infinitely recurse in some cases.
> ::: js/src/jsproxy.cpp
> @@ +1090,5 @@
> >  
> >      static ScriptedDirectProxyHandler singleton;
> > +
> > +    static const int HANDLER_EXTRA;
> > +    static const int REVOKE_SLOT;
> 
> These need brief comments.
> 

Done 
> You can add the '= 0' here and kill the definitions below. (The C++ standard
> has a totally special rule permitting this exact thing. See
> js::types::TypeObject::LAZY_SINGLETON for an example of a 'static const int'
> defined this way.)

Interesting. I learn new things about C++ all the time :)
> 
> @@ +3020,5 @@
> > +    RootedValue proxyVal(cx, args.rval());
> > +    MOZ_ASSERT(proxyVal.toObject().is<ProxyObject>());
> > +
> > +    RootedObject revoker(cx, NewFunctionWithReserved(cx, RevokeProxy, 0, 0, cx->global(),
> > +    "RevokeProxy"));
> 
> Fix the indentation here.
> 
> Also I think it's OK to use NewFunctionByIdWithReserved here, using
> AtomToId(cx->names().revoke) for the id.

Done

> 
> @@ +3022,5 @@
> > +
> > +    RootedObject revoker(cx, NewFunctionWithReserved(cx, RevokeProxy, 0, 0, cx->global(),
> > +    "RevokeProxy"));
> > +    if (!revoker) {
> > +        js_ReportOutOfMemory(cx);
> 
> Remove the js_ReportOutOfMemory calls here and after NewBuiltinClassInstance
> below. Those functions already call it if needed.
> 

Done

> ::: js/src/vm/ProxyObject.cpp
> @@ +70,5 @@
> >  
> >  void
> > +ProxyObject::setSameCompartmentPrivate(const Value &priv)
> > +{
> > +    setSlot(PRIVATE_SLOT, priv);
> 
> Consider writing it like this:
> 
> void
> ProxyObject::revoke()
> {
>     MOZ_ASSERT(handler() == &ScriptedDirectProxyHandler::singleton);
>     setSlot(PRIVATE_SLOT, NullValue());  // clear the target
>     setExtra(ScriptedDirectProxyHandler::HANDLER_EXTRA, NullValue());
> }
> 
> This seems like a more natural API for ProxyObject to present, and it might
> make it easier to switch implementations to khuey-style later.

I agree, but at the moment, there's no real good way for ProxyObject to hear about ScriptedDirectProxyHandler. Perhaps this would be possible after bug 1031092? But really, this is moot, because we can just make it use ProxyObject::nuke() once we have a handler for the rejection. I have unfortunately left it as is.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ef39de0b6232
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/14dfcbd39f8b

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ef39de0b6232
https://hg.mozilla.org/mozilla-central/rev/14dfcbd39f8b

Comment 8

[Tooru Fujisawa [:arai]]

Updated following pages:
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy/revocable

Following page is already updated:
  https://developer.mozilla.org/en-US/Firefox/Releases/34

Comment 9

[Florian Scholz (Open Web Docs)]

Reviewed. Thanks, :arai!