926012 - Sanify proxy prototype scheme

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Faust [:efaust]]

The proxy prototype saving scheme is completely nuts. We have internal hooks to do proto lookups, but they are only sometimes used, and only internally.

What's more, we want to be able to allow sets as well on proxies, in some cases (bug 889198), and having a setPrototypeOf hook that's sanely used would be a good step towards implementing the ES6 hooks (bug 888969).

Comment 1

[Eric Faust [:efaust]]

Attachment: Part 1: Clean up __proto__ setting semantics on native objects

Clean up __proto__ sets through both the public API and __proto__ setter.

Comment 2

[Eric Faust [:efaust]]

Attachment: Part 2: Allow proxy __proto__ sets

Allow setting __proto__ on proxies, paying careful attention to the existing LazyProto machinery.

This patch also includes logic for clarifying the use of Proxy::LazyProto in the getPrototypeOf() cases of proxy-internal code.

Comment 3

[Eric Faust [:efaust]]

Attachment: Part 3: Convert js::Wrapper to using Proxy::LazyProto exclusively

Since we now have setPrototypeOf() machinery, we can cleanly move to an all-lazy model for all wrappers.

Comment 4

[Eric Faust [:efaust]]

(In reply to Eric Faust [:efaust] from comment #2)
> This patch also includes logic for clarifying the use of Proxy::LazyProto in
> the getPrototypeOf() cases of proxy-internal code.

Until Wrapper is lazified, this will cause incorrectness, as they will not have LazyProto in the prototype slot in order to get the handler called. Afterwards, all should be well. the relevant changes should maybe be landed as a Part 4 (in particular the changes from |handler->getPrototypeOf()| to |JSObject::getProto()| and the assert in BaseProxyHandler::getPrototypeOf()), but since it doesn't materially affect review, I will let you use your imagination :).

The idea we are striving for here is that it's an opt-in system. Either you don't have traps, and you use the normal setup in the TypeObject, or you do have traps, and put LazyProto in the TypeObject, and your traps are always called.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 816185 [details] [diff] [review]
Part 3: Convert js::Wrapper to using Proxy::LazyProto exclusively

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

::: dom/base/nsGlobalWindow.cpp
@@ +2045,1 @@
>    return NS_OK;

If you do this, SetOuterObject should be made to return void.

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 816185 [details] [diff] [review]
Part 3: Convert js::Wrapper to using Proxy::LazyProto exclusively

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

Eric and I discussed this extensively over IRC. I expressed dissatisfaction about the fact that the handler traps wouldn't be called if there were something in the proto slot, and argued that we should always call the handler, and have BaseProxyHandler::{get,set}Prototype return the value in the slot. Brian brought up the point that the proto is actually stored in the type object. It seems to me that proxies shouldn't have types at all, but maybe this is too much trouble to change.

So I'm ok with the VM only calling the handler trap if the value in the slot is something other than Proxy::LazyProto. In that case, BaseProxyHandler::{get,set}Prototype should be MOZ_NOTREACHED, since we should never hit a trap that needs to pull the proto out of the slot.

We also need very clear and detailed documentation in a comment about how the whole setup works, probably above the declaration of BaseProxyHandler::{get,set}Prototype.

I think we need to override XrayWrapper::setPrototypeOf to throw, because mutating the prototype of an Xray wrapper should not mutate the prototype of the underlying object. peterv is going to fix this up very shortly so that mutating the proxy of an Xray wrapper will make it non-lazy and have it store the prototype directly on the Xray wrapper itself.

::: dom/base/nsGlobalWindow.cpp
@@ +2045,1 @@
>    return NS_OK;

Please kill SetOuterObject entirely.

::: js/src/jsproxy.cpp
@@ +507,5 @@
> +DirectProxyHandler::setPrototypeOf(JSContext *cx, HandleObject proxy, HandleObject proto, bool *bp)
> +{
> +    RootedObject target(cx, proxy->as<ProxyObject>().target());
> +    return JSObject::setProto(cx, target, proto, bp);
> +}

Hm, I don't think we can land this without corresponding support for ScriptedDirectProxyHandler, because if these traps aren't scriptable, we effectively kill the ability to implement a full membrane with scripted proxies, because the consumer can always read and mutate the prototypes of the underlying object, regardless of whatever revocation the handler may have tried to perform.

If that bit of work is too much to do right now, a stop-gap solution would just be to override ScriptedDirectProxyHandler and make both of these traps have non-lazy proto-slot behavior.

Or am I missing something?

::: js/src/jswrapper.h
@@ +119,5 @@
>      virtual bool regexp_toShared(JSContext *cx, HandleObject proxy, RegExpGuard *g) MOZ_OVERRIDE;
>      virtual bool defaultValue(JSContext *cx, HandleObject wrapper, JSType hint,
>                                MutableHandleValue vp) MOZ_OVERRIDE;
>      virtual bool getPrototypeOf(JSContext *cx, HandleObject proxy, MutableHandleObject protop);
> +    virtual bool setPrototypeOf(JSContext *cx, HandleObject proxy, HandleObject proto, bool *bp);

Please annotate both of these as MOZ_OVERRIDE.

::: js/xpconnect/wrappers/WaiveXrayWrapper.cpp
@@ +97,5 @@
> +    if (!CrossCompartmentWrapper::getPrototypeOf(cx, wrapper, protop))
> +        return false;
> +    if (protop)
> +        protop.set(WrapperFactory::WaiveXray(cx, protop));
> +    return !protop || JS_WrapObject(cx, protop.address());

This misses some important subtleties in WaiveXrayAndUnwrap. I'm assuming you did it this way because you're dealing with an object, rather than a value?

Please make an overload of WaiveXrayAndUnwrap that takes an object, and fix this up to use that.

Comment 7

[Eric Faust [:efaust]]

Comment on attachment 816184 [details] [diff] [review]
Part 2: Allow proxy __proto__ sets

Cancelling review in response to Bobby's comments on the general system.

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 816181 [details] [diff] [review]
Part 1: Clean up __proto__ setting semantics on native objects

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

::: js/src/js.msg
@@ +414,5 @@
>  MSG_DEF(JSMSG_TYPEDOBJECT_NOT_TYPE_OBJECT, 361, 0, JSEXN_ERR, "Expected a type object")
>  MSG_DEF(JSMSG_TOO_MANY_CON_SPREADARGS, 362, 0, JSEXN_RANGEERR, "too many constructor arguments")
>  MSG_DEF(JSMSG_TOO_MANY_FUN_SPREADARGS, 363, 0, JSEXN_RANGEERR, "too many function arguments")
>  MSG_DEF(JSMSG_DEBUG_NOT_DEBUGGEE,     364, 2, JSEXN_ERR, "{0} is not a debuggee {1}")
> +MSG_DEF(JSMSG_SETINHERITANCE_FAIL, 365, 0, JSEXN_TYPEERR, "[[SetInheritance]] failed")

Apparently the latest ES6 draft makes this [[GetPrototypeOf]] and [[SetPrototypeOf]].  Bikeshed needs new paint!

::: js/src/jsapi.cpp
@@ +2601,5 @@
>      CHECK_REQUEST(cx);
>      assertSameCompartment(cx, obj, proto);
>  
> +    bool dummy;
> +    return JSObject::setProto(cx, obj, proto, &dummy);

Hmm.  Is it really the case that this shouldn't throw if setting fails?  I'd rather see

  if (!JSObject::setProto(..., &succeeded))
    return false;
  if (!succeeded) {
    throw; 
    return false;
  }
  return true;

::: js/src/jsobj.cpp
@@ +2954,5 @@
>  };
>  
>  bool
>  js::SetClassAndProto(JSContext *cx, HandleObject obj,
> +                     const Class *clasp, Handle<js::TaggedProto> proto)

Could you add bool *succeeded to the argument list here, and set it in the two cases in this method that actually are successful?  I'd rather see the successfulness contained within the single method that does all the work of prototype-swapping.

::: js/src/jsobj.h
@@ +468,5 @@
>      }
>      static inline bool getProto(JSContext *cx, js::HandleObject obj,
>                                  js::MutableHandleObject protop);
> +    static inline bool setProto(JSContext *cx, js::HandleObject obj,
> +                                js::HandleObject proto, bool *bp);

Add a /**/ comment by this explaining what |bp| should be and how it's modified.  Also rename it |bool *succeeded| so it's more obvious what the meaning is.  This'll also accord with the outparam-naming I have for a partial patch to add a similar outparam to all the defineProperty and setProperty hooks.

::: js/src/jsobjinlines.h
@@ +394,5 @@
>      }
>  }
>  
> +/* static */ inline bool
> +JSObject::setProto(JSContext *cx, js::HandleObject obj, js::HandleObject proto, bool *bp)

Use JS:: here for the arguments, as that (not js::) is the canonical location.

I guess this needs to be declared inline for performance or something?  OOL if possible, please.

::: js/src/vm/GlobalObject.cpp
@@ +110,5 @@
>  ProtoSetterImpl(JSContext *cx, CallArgs args)
>  {
>      JS_ASSERT(TestProtoSetterThis(args.thisv()));
>  
> +    // Step 2

Don't add this.  The ES6 algorithm returns the provided argument, not |undefined|, for primitive |this|, or for a primitive zeroth argument (or none at all).  If we're going to make that change, and okay, sure, whatever, either make it all the way, or don't make it at all in this patch.  I'd rather not see it change in this patch, myself -- keep semantic changes outside the scope of this bug to separate revisions.

Comment 9

[Jeff Walden [:Waldo]]

Aargh!  I had this queued up last night, and submitted it, but I hit the 3+-days-old interstitial and didn't notice til just now.  Hopefully you had other things to do and could stand a little delay.  :-\

Comment 10

[Eric Faust [:efaust]]

OK, since Peter, Bobby and I are currently stationed at the far corners of the globe, perhaps it makes more sense to try and do this discussion of xray protos in an asynchronus fashion.

So, the current patch is bad, because it forwards to the target, which explicitly has a modifiable and different prototype chain than the xrays.

All the xrays to a given reflector have to share a single prototype.

It made sense, then, to put the prototype in a slot of the expando on the reflector, in the same way as we do other chrome properties.

The problem, as I see it, is that this /forces/ the existence of an expando on every xray'd reflector. As there is currently machinery to "fault in" expandos at the time of first use, I assume that there is no small memory cost to this idea.

Bobby, is this prohibitive? Is there another place we can do it?

Peter suggested perhaps knowing a "default" answer that we can pass back from the hook if there's no expando, and then creating one to do the first set. We could even do the thing that we do with the rest of the properties and fault in the default value and just ensureExpando() on first getPrototypeOf(), but this seems like it doesn't save us much, as prototype lookups are common.

Comment 11

[Eric Faust [:efaust]]

Redo CC and needinfo. Mid-air collision.

Comment 12

[Bobby Holley (:bholley)]

(In reply to Eric Faust [:efaust] from comment #10)
> The problem, as I see it, is that this /forces/ the existence of an expando
> on every xray'd reflector. As there is currently machinery to "fault in"
> expandos at the time of first use, I assume that there is no small memory
> cost to this idea.

Yeah, we shouldn't do that. We should only use the slot on the expando object in the case where someone has explicitly munged the prototype of the Xray (which should be very rare). All the other cases should be statelessly computable.

> Peter suggested perhaps knowing a "default" answer that we can pass back
> from the hook if there's no expando, and then creating one to do the first
> set.

Yes, this is my thinking too.

> but this seems like it doesn't save us much, as prototype lookups are common.

That wouldn't matter, because we'd only be creating the expando object on the first _set_. If we just need to _get_ the prototype, and there's either (a) no expando object or (b) the slot in the expando object is undefined, we just compute the right prototype.

Before peter's patch, this will just be a wrapper of the prototype of the underlying object (which is unideal, but should suffice for a temporary fix as long as we don't forward proto sets on the Xray to the underlying object). After peter's patch, we'll compute the proper Xray prototype.

Comment 13

[Eric Faust [:efaust]]

https://tbpl.mozilla.org/?tree=Try&rev=856fb217da86

Comment 14

[Eric Faust [:efaust]]

Attachment: Part 2: Allow proxy __proto__ sets

reposted with a few tweaks. In particular, a block comment.

Comment 15

[Eric Faust [:efaust]]

Attachment: Part 3: Convert js::Wrapper to using Proxy::LazyProto exclusively

Feedback addressed. WaiveXrayWrapper::getPrototypeOf fixed, as well as Xray prototype expando stashing implemented.

Comment 16

[Bobby Holley (:bholley)]

Comment on attachment 823613 [details] [diff] [review]
Part 3: Convert js::Wrapper to using Proxy::LazyProto exclusively

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

This is excellent. Thank you for doing it, and I'm really sorry about the review delay. r=bholley with comments addressed.

::: js/src/jswrapper.cpp
@@ +141,5 @@
>                               unsigned flags)
>  {
>      // Allow wrapping outer window proxies.
>      JS_ASSERT(!obj->is<WrapperObject>() || obj->getClass()->ext.innerObject);
> +    JS_ASSERT(wrappedProto == Proxy::LazyProto);

This function only has one consumer. Rather than doing this, can we just alter the signature to avoid passing wrappedProto?

::: js/src/shell/js.cpp
@@ +3820,5 @@
> +
> +    RootedValue priv(cx, ObjectValue(*obj));
> +    ProxyOptions options;
> +    options.setCallable(obj->isCallable());
> +    return NewProxyObject(cx, handler, priv, proto, parent, options);

Is it possible to just invoke Wrapper::New and then manually swap out the prototype? I'm concerned about the other bits in this function going out of sync with those in Wrapper::New.

::: js/xpconnect/wrappers/WaiveXrayWrapper.cpp
@@ +97,5 @@
> +    if (!CrossCompartmentWrapper::getPrototypeOf(cx, wrapper, protop))
> +        return false;
> +    if (!protop)
> +        return true;
> +    return WrapperFactory::WaiveXrayAndWrap(cx, protop);

I'd prefer to write this as:

return CrossCompartmentWrapper::getPrototypeOf(cx, wrapper, protop) &&
       (!protop || WrapperFactory::WaiveXrayAndWrap(cx, protop));

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +538,5 @@
> +    RootedObject obj(cx, JSVAL_TO_OBJECT(*vp));
> +    if (!WaiveXrayAndWrap(cx, &obj))
> +        return false;
> +
> +    *vp = OBJECT_TO_JSVAL(obj);

ObjectValue(*obj);

@@ +545,5 @@
> +
> +bool
> +WrapperFactory::WaiveXrayAndWrap(JSContext *cx, MutableHandleObject object)
> +{
> +

Please call the argument |objArg|, and MOZ_ASSERT(objArg) here.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +1805,5 @@
> +    // The expando lives in the target's compartment, so do our installation there.
> +    JSAutoCompartment ac(cx, target);
> +
> +    RootedValue v(cx, ObjectValue(*proto));
> +    if (!JS_WrapValue(cx, v.address()))

shouldn't this be &v? I can never keep track of where we are with rooting...

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 823611 [details] [diff] [review]
Part 2: Allow proxy __proto__ sets

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

This patch just adds architecture but doesn't unbreak the resulting world, and then Part 3 fixes everything, right?

ecma_5/extensions/cross-global-getPrototypeOf.js probably can be reenabled (with the one line un-commented out) with the patches here.  Do so if you can (feel free to push to a followup patch if there's anything at all non-trivial involved in making it work when enabled).

::: js/src/jsobjinlines.h
@@ +396,5 @@
>  
>  /* static */ inline bool
>  JSObject::setProto(JSContext *cx, js::HandleObject obj, js::HandleObject proto, bool *bp)
>  {
> +    /* Proxies live in their own little world */

Period at end of sentence.

::: js/src/jsproxy.cpp
@@ +365,5 @@
>  
>  bool
>  BaseProxyHandler::getPrototypeOf(JSContext *cx, HandleObject proxy, MutableHandleObject protop)
>  {
> +    MOZ_CRASH("Must override getPrototypeOf with lazy prototype.");

MOZ_ASSUME_UNREACHABLE

Comment 18

[Eric Faust [:efaust]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8e34b4680c17
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8ba79063973d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ae50af337766

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8e34b4680c17
https://hg.mozilla.org/mozilla-central/rev/8ba79063973d
https://hg.mozilla.org/mozilla-central/rev/ae50af337766