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.

This is a pretty straightforward change.

Similar to above.

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.

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

Or "lazily" instead of "on demand".

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.

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.

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

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.

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

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

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

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

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?

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

Here's a patch that fixes those failing tests

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