787856 - Allow for the possibility of dynamically-generated object prototypes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: changes to JSObject::getProto

There are a few reasons to do this:
1. It will help me with some GC work, where it's really important that when we wrap an object, we don't have to wrap its proto right away.
2. It will fix some weird semantic issues where changing the proto of an object will not be reflected in its CCWs.
3. Eddy eventually wants to add a getPrototypeOf hook to scripted proxies.

With this patch, we continue to store the proto in the TypeObject. However, if the proto is going to be dynamically generated, then we store 0x1 there instead and compute the proto as needed.

This is a bit dangerous, since code may be unprepared to handle reading the proto and getting 0x1. So I've replaced JSObject::getProto() with three things:

* Calling obj->getProto() is still possible, but it asserts if obj is a proxy. This allows a lot of code in jsarray, jstypedarray, etc. to continue working.

* Calling obj->getTaggedProto() returns a wrapper class (TaggedProto) around JSObject*. It allows you to test if the proto is null, dynamic, or an object.

* Calling JSObject::getProto(cx, obj, &proto) goes to the trouble of dynamically computing the proto when needed.

In all the places where we can handle dynamic protos, I've tried to change the code to take a TaggedProto rather than a JSObject* for the proto. The main place I didn't change is the actual type->proto field because it seemed like it would be a pain. We rarely access this field directly, so I don't think it will be a problem.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix GetObjectProto in jsfriendapi to be fallible

This is a pretty straightforward change.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix JS_GetPrototype to be fallible

Similar to above.

Comment 3

[Brian Hackett [Laid off!]]

Comment on attachment 657746 [details] [diff] [review]
changes to JSObject::getProto

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

::: js/src/jscompartment.cpp
@@ +236,5 @@
>          *vp = p->value;
>          if (vp->isObject()) {
>              RootedObject obj(cx, &vp->toObject());
>              JS_ASSERT(obj->isCrossCompartmentWrapper());
> +            JS_ASSERT(obj->getParent() == global);

Is this change related to this patch?

::: js/src/jsinfer.h
@@ +38,5 @@
> +    inline bool isDynamic() const;
> +    inline bool isObject() const;
> +    inline JSObject *toObject() const;
> +    inline JSObject *toObjectOrNull() const;
> +    JSObject *toObjectUnchecked() const { return proto; }

Maybe make this raw() or something similar?  toObjectUnchecked seems to imply it is an object, when it could be dynamic.

::: js/src/jsiter.cpp
@@ +631,2 @@
>                      proto->lastProperty() == lastni->shapes_array[1] &&
>                      !proto->getProto()) {

This 'if' clause is pretty nasty, can you split it into two 'if' statements and init proto when declaring it?  It's not clear that proto isn't used at all after being maybe-initialized in the middle of the clause.

::: js/src/jsobj.cpp
@@ +3812,5 @@
> +        for (TaggedProto obj2 = proto;
> +             obj2.isObject();
> +             obj2 = obj2.toObject()->getTaggedProto())
> +        {
> +            if (obj2.toObject() == obj) {

The semantics here seem weird, this will just stop checking for cycles if it sees a dynamic proto.  It seems like it would be better if dynamic prototypes were computed here to continue looking for a cycle in the loop.

Not sure how this interacts with scripted getPrototype hooks with side effects either.  Related note, with scripted getPrototype hooks we could still get cyclic protoype chains and will need to be able to break out of those via the operation callback.

::: js/src/jsobj.h
@@ +447,5 @@
>  
> +    inline JSObject *getProto() const;
> +    inline js::TaggedProto getTaggedProto() const;
> +    static inline bool getProto(JSContext *cx, js::HandleObject obj,
> +                                js::MutableHandleObject protop);

Can you add a comment explaining the difference between these methods?

@@ +1320,5 @@
>  CheckAccess(JSContext *cx, JSObject *obj, HandleId id, JSAccessMode mode,
>              js::Value *vp, unsigned *attrsp);
>  
> +extern bool
> +IsDelegate(JSContext *cx, js::HandleObject obj, const js::Value &v, bool *result);

unnecessary js::

::: js/src/jsobjinlines.h
@@ +703,5 @@
> +         obj.isObject();
> +         obj = obj.toObject()->getTaggedProto())
> +    {
> +        JS_ASSERT(obj.toObject() != this);
> +    }

Similar issue to SetProto here, it's not clear what this code is really testing now.  Add a comment or maybe just remove this block?

@@ +727,5 @@
> +
> +/* static */ inline bool
> +JSObject::getProto(JSContext *cx, js::HandleObject obj, js::MutableHandleObject protop)
> +{
> +    if (obj->isProxy()) {

The invariants here aren't quite clear, it seems like this would make more sense as obj->getTaggedProto().isDynamic() rather than obj->isProxy(), and assert isProxy() in the isDynamic() branch.

::: js/src/jsproxy.cpp
@@ +381,5 @@
>  bool
>  BaseProxyHandler::getPrototypeOf(JSContext *cx, JSObject *proxy, JSObject **proto)
>  {
>      // The default implementation here just uses proto of the proxy object.
> +    *proto = proxy->getTaggedProto().toObjectUnchecked();

Why can't this be proxy->getTaggedProto().toObjectOrNull()?  It seems like *proto should not be dynamic, as proto isn't a TaggedProto*, and any proxy with a dynamic proto would need a non-default hook here.

::: js/src/methodjit/Compiler.cpp
@@ +6608,5 @@
>      /* Walk prototype chain, break out on NULL or hit. */
>      masm.loadPtr(Address(obj, JSObject::offsetOfType()), obj);
>      masm.loadPtr(Address(obj, offsetof(types::TypeObject, proto)), obj);
> +    Jump isDynamic = masm.branch32(Assembler::Equal, obj, Imm32(1));
> +    stubcc.linkExit(isDynamic, Uses(2));

Comment above this needs fixing.

Comment 4

[David Mandelin [:dmandelin]]

Terminological nitpick: Wouldn't this more accurately be described as "creating an object's prototype on demand?"

Comment 5

[Andrew McCreight [:mccr8]]

Or "lazily" instead of "on demand".

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I went through changing all the "dynamic" names to "lazy". Then I remembered that the reason I chose dynamic is that, if we ever allow scripted proxies to have getPrototypeOf hooks, neither "lazy" nor "on demand" is really right. In theory, calling getPrototypeOf on the same object twice could return a different result each time.

However, since we really have no idea of the semantics of such a hook, I think I'll continue with the lazy name. It makes more sense for the current code.

Eddie, if you're still interested in implementing a getPrototypeOf hook, this bug should help you out. However, there are still a lot of semantic issues that will need to be resolved.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: change getEmptyType to use a table

Brian suggested this change before he left. It removes the singleton type objects for objects with empty protos and lazy protos and just uses the normal table lookup mechanism, as is done for type objects with regular protos.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: use lazy proto for cross-compartment wrappers

This actually makes it so that we don't wrap the proto for a cross-compartment wrapper unless we actually need it.

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 660629 [details] [diff] [review]
use lazy proto for cross-compartment wrappers

>+    RootedObject proto(cx);
>+    {
>+        RootedObject wrapped(cx, wrappedObject(proxy));
>+        AutoCompartment call(cx, wrapped);
>+        if (!JSObject::getProto(cx, wrapped, &proto))
>+            return false;
>+        if (proto)
>+            proto->setDelegate(cx);

What does this do?

>+    }
>+
>+    if (!proxy->compartment()->wrap(cx, proto.address()))
>+        return false;
>+
>+    *protop = proto;
>+    return true;
>+}

So we don't cache this, just in case the proto of the underlying object ever changes? I guess we'll at least have the cache of the crossCompartmentWrapper map. But I think that the lack of caching makes "dynamic" a more appropriate word than "lazy". Just bikeshedding though.

>-js::NukeCrossCompartmentWrapper(JSObject *wrapper)
>+js::NukeCrossCompartmentWrapper(JSContext *cx, JSObject *wrapper)


>     wrapper->setReservedSlot(JSSLOT_PROXY_EXTRA + 1, NullValue());
>+
>+    {
>+        AutoCompartment ac(cx, wrapper);
>+        RootedObject rootedWrapper(cx, wrapper);
>+        Rooted<TaggedProto> null(cx);
>+        SetProto(cx, rootedWrapper, null, false);
>+    }

What's the motivation here? A comment would be helpful.



>diff --git a/js/xpconnect/wrappers/WrapperFactory.cpp b/js/xpconnect/wrappers/WrapperFactory.cpp
>--- a/js/xpconnect/wrappers/WrapperFactory.cpp
>+++ b/js/xpconnect/wrappers/WrapperFactory.cpp
>@@ -399,21 +399,27 @@ WrapperFactory::Rewrap(JSContext *cx, JS

>+            {
>+                JSAutoCompartment ac(cx, obj);
>+                JSObject *unwrappedProto;
>+                if (!js::GetObjectProto(cx, obj, &unwrappedProto))
>+                    return NULL;
>+                if (unwrappedProto && IsCrossCompartmentWrapper(unwrappedProto))
>+                    unwrappedProto = Wrapper::wrappedObject(unwrappedProto);

Just curious - was this added for theoretical correctness, or did a test actually go orange? AFAIK we don't currently have any uses in the tree of non-proxy objects with proxies on the proto chain (which is where this would matter), but I agree that it's something we should support.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Bobby Holley (:bholley) from comment #9)
> Comment on attachment 660629 [details] [diff] [review]
> use lazy proto for cross-compartment wrappers
>
> >+        if (proto)
> >+            proto->setDelegate(cx);
> 
> What does this do?

Normally when something becomes the prototype of another object, we set a "delegate" flag on the prototype. This just ensures that that flag gets set.

> >diff --git a/js/xpconnect/wrappers/WrapperFactory.cpp b/js/xpconnect/wrappers/WrapperFactory.cpp
> >--- a/js/xpconnect/wrappers/WrapperFactory.cpp
> >+++ b/js/xpconnect/wrappers/WrapperFactory.cpp
> >@@ -399,21 +399,27 @@ WrapperFactory::Rewrap(JSContext *cx, JS
> 
> >+            {
> >+                JSAutoCompartment ac(cx, obj);
> >+                JSObject *unwrappedProto;
> >+                if (!js::GetObjectProto(cx, obj, &unwrappedProto))
> >+                    return NULL;
> >+                if (unwrappedProto && IsCrossCompartmentWrapper(unwrappedProto))
> >+                    unwrappedProto = Wrapper::wrappedObject(unwrappedProto);
> 
> Just curious - was this added for theoretical correctness, or did a test
> actually go orange? AFAIK we don't currently have any uses in the tree of
> non-proxy objects with proxies on the proto chain (which is where this would
> matter), but I agree that it's something we should support.

I did get a lot of failures in this code until I changed it to the current version. I'm not sure what you mean about non-proxy objects with proxies on the proto chain, though. Even if everything is a proxy, we still need to run this code. The alternative would be to put this logic in the getPrototypeOf proxy hook for COWs or something, but that seemed more complicated to me.

Comment 11

[Bobby Holley (:bholley)]

(In reply to Bill McCloskey (:billm) from comment #10)

> > >+            {
> > >+                JSAutoCompartment ac(cx, obj);
> > >+                JSObject *unwrappedProto;
> > >+                if (!js::GetObjectProto(cx, obj, &unwrappedProto))
> > >+                    return NULL;
> > >+                if (unwrappedProto && 

> I did get a lot of failures in this code until I changed it to the current
> version. I'm not sure what you mean about non-proxy objects with proxies on
> the proto chain, though.

This code accesses the underlying object being wrapped, |obj|, which is not a cross-compartment wrapper per the semantics of Rewrap. Then, we check if its proto is a cross-compartment wrapper. Am I missing something?

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0853ae2ee0f
https://hg.mozilla.org/integration/mozilla-inbound/rev/c372439f0aad
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd398d69d052
https://hg.mozilla.org/integration/mozilla-inbound/rev/7228effb2e5b

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a0853ae2ee0f
https://hg.mozilla.org/mozilla-central/rev/c372439f0aad
https://hg.mozilla.org/mozilla-central/rev/fd398d69d052
https://hg.mozilla.org/mozilla-central/rev/7228effb2e5b

Comment 14

[Eddy Bruel [:ejpbruel]]

This patch causes the following tests to fail for me:
js/src/jit-test/tests/proxy/testDirectProxyKeys11.js
js/src/jit-test/tests/proxy/testDirectProxySet7.js

I'm confused. How was this patch able to make it to m-c?

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Those tests are not checked in. Maybe you forgot to hg add them at some point.

Comment 16

[Eddy Bruel [:ejpbruel]]

Attachment: Fix failing direct proxy tests

Here's a patch that fixes those failing tests

Comment 17

[Eddy Bruel [:ejpbruel]]

(In reply to Bill McCloskey (:billm) from comment #15)
> Those tests are not checked in. Maybe you forgot to hg add them at some
> point.

That makes sense. Thanks for pointing that out.

I vaguely recall there being something wrong with those tests, so I'm reluctant to check them in now. Even so, the patch I filed still fixes the case where the target of a proxy is another proxy, so we might want to land it anyway. What do you think?

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 666227 [details] [diff] [review]
Fix failing direct proxy tests

Yes, I think we should check this in. Thanks for the fix. I think I missed these calls to getProto() because I wrote the patch before bug 703537 was checked in.

It would be good to have some sort of test where a proxy has a cross-compartment wrapper as its proto.

Comment 19

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22c9c298a21f

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/22c9c298a21f