820577 - Support reparenting for new DOM binding objects

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Peter Van der Beken [:peterv]]

Attachment: v1

Unfortunately it's different enough from WN reparenting that it didn't really make sense to refactor the existing code. I put it in mozilla/dom, let me know what you think of that.

Comment 1

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

Comment on attachment 691070 [details] [diff] [review]
v1

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

::: dom/bindings/BindingUtils.cpp
@@ +1248,5 @@
>    return JS_WrapValue(cx, v);
>  }
>  
> +// Dynamically ensure that two objects don't end up with the same reserved slot.
> +class AutoCloneDOMObjectSlotGuard NS_STACK_CLASS {

{ on the next line

@@ +1250,5 @@
>  
> +// Dynamically ensure that two objects don't end up with the same reserved slot.
> +class AutoCloneDOMObjectSlotGuard NS_STACK_CLASS {
> +public:
> +  AutoCloneDOMObjectSlotGuard(JSObject *aOld, JSObject *aNew)

* to the left

@@ +1364,5 @@
> +    if (ww != obj) {
> +      JSObject *newwrapper =
> +        xpc::WrapperFactory::WrapSOWObject(aCx, newobj);
> +      if (!newwrapper) {
> +        MOZ_CRASH();

The first half of this function just returns failure, why not here?

::: dom/bindings/BindingUtils.h
@@ +1002,5 @@
> +{
> +  static JSObject* Get(JSContext* cx, JSObject* obj)
> +  {
> +    MOZ_CRASH();
> +    return nullptr;

Fun. Can we make this a compiler error?

::: dom/bindings/DOMJSClass.h
@@ +135,5 @@
>    eInterfacePrototype
>  };
>  
> +typedef JSObject* (*ParentObjectGetter)(JSContext* cx, JSObject* obj);
> +typedef JSObject* (*ProtoObjectGetter)(JSContext* cx, JSObject* global);

We're not terrible consistent about this, but 'a' for arguments please

Comment 2

[Bobby Holley (:bholley)]

I'm working on this review, but I think I need to sleep on parts of it. I'll pick it back up in the morning.

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 691070 [details] [diff] [review]
v1

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

r- just because there are a number of issues left to be discussed and sorted out.

I didn't review the codegen changes - bz is going to look at those.

What's the story on SOWs? Is that handled in another patch somewhere? The SCSW code that I know depends on the wrappee being an XPCWN, which will change when Node becomes a new binding.

::: content/base/src/nsNodeUtils.cpp
@@ +538,5 @@
> +          rv = xpc->ReparentWrappedNativeIfFound(aCx, wrapper, aNewScope, aNode);
> +        }
> +      }
> +      if (NS_FAILED(rv)) {
> +        aNode->mNodeInfo.swap(nodeInfo);

I know this is unrelated to your change, but what the heck is this about?

::: dom/bindings/BindingUtils.cpp
@@ +1283,5 @@
> +  }
> +
> +  // ReparentWrapperIfFound is really only meant to be called from DOM code
> +  // which must happen only on the main thread. Bail if we're on some other
> +  // thread or have a non-main-thread-only wrapper.

This comment is obsolete.

@@ +1288,5 @@
> +
> +  JSAutoCompartment ac(aCx, aNewGlobalObject);
> +
> +  if (js::GetObjectCompartment(aOldGlobalObject) !=
> +      js::GetObjectCompartment(aNewGlobalObject)) {

Can you give me an example of the same-compartment case? Given that the net effect of such a call would be JS_SetParent, I don't think it's worth adding a level of indentation / branching to this already-complicated function. I'd prefer to either assert against the same-compartment case, or move the cross-compartment handling into a helper function.

@@ +1290,5 @@
> +
> +  if (js::GetObjectCompartment(aOldGlobalObject) !=
> +      js::GetObjectCompartment(aNewGlobalObject)) {
> +    // Oh, so now we need to move the wrapper to a different compartment.
> +    NS_ASSERTION(aNewParent, "won't be able to find the new parent");

If we're going to assert that the arguments are non-null, we should do it at the beginning of the function. and MOZ_ASSERT.

@@ +1296,5 @@
> +    // First, the clone of the reflector, get a copy of its
> +    // properties and clone its expando chain. The only part that is
> +    // dangerous here if we have to return early is that we must avoid
> +    // ending up with two reflectors pointing to the same native. Other than
> +    // that, the objects we create will just go away if we return early.

I found these sentences a bit hard to parse, especially the first one.

@@ +1310,5 @@
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    js::SetReservedSlot(newobj, DOM_OBJECT_SLOT,
> +                        js::GetReservedSlot(obj, DOM_OBJECT_SLOT));

Why is this necessary? It looks like JS_CloneObject ends up invoking CopySlots.

@@ +1375,5 @@
> +        MOZ_CRASH();
> +      }
> +
> +      obj = newobj;
> +      SetSystemOnlyWrapper(obj, *ww);

I can't review this correctly without having seen the SOW patch.

@@ +1394,5 @@
> +      MOZ_CRASH();
> +    }
> +  }
> +
> +  // We might need to call a hook here similar to PostTransplant.

PostTransplant is used for <plugin> only. Is that being converted to the new bindings? Is there a bug on it? We'll need to file a blocking bug to sort out the plugin proto fixup.

@@ +1398,5 @@
> +  // We might need to call a hook here similar to PostTransplant.
> +
> +  // Now we can just fix up the parent and return the wrapper
> +
> +  if (aNewParent && !JS_SetParent(aCx, obj, aNewParent)) {

Why are we checking for aNewParent here?

::: dom/bindings/BindingUtils.h
@@ +1650,5 @@
>  
> +nsresult
> +ReparentWrapper(JSContext* aCx, JSObject* aOldGlobalObject,
> +                JSObject* aNewGlobalObject, JSObject* aNewParent,
> +                JSObject* obj);

Hm, why do we need aOldGlobalObject and aNewGlobalObject? Shouldn't obj and aNewParent be enough?

Also, aObj for consistency I'd think.

I think I'd switch the order too. So:
ReparentWrapper(JSContext* aCx, JSObject* aObj, JSObject* aNewParent).

::: dom/bindings/DOMJSClass.h
@@ +152,5 @@
>  
>    const NativePropertyHooks* mNativeHooks;
>  
> +  ParentObjectGetter mGetParentObject;
> +  ProtoObjectGetter mGetProto;

The naming here is kind of unfortunate. I originally thought that we were appending the "Object" suffix to differentiate it from GetParent, which I presumed was the native method on nsINode etc that returned an nsISupports. But it looks like _that_ method is actually called GetParentObject as well :-(.

So in that case, should we at least align the names here, with mGetParent and mGetProto? I don't have a super strong opinion, I just want to make sure we give it some thought.

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +1671,5 @@
>  //
>  // See bug 751995 for more information.
>  
> +static nsresult
> +RescueOrphans(XPCCallContext& ccx, JSObject* obj)

So, the support for new binding objects here only takes effect when said objects appear on the parent chain of XPCWNs, right?

I want to have a detailed discussion about this, so any r+ is contingent on us having that.

@@ +1713,5 @@
> +        } else {
> +            MOZ_ASSERT(IsDOMObject(obj));
> +            const DOMClass* domClass = GetDOMClass(obj);
> +            parentObj = domClass->mGetParentObject(ccx, obj);
> +        }

Yikes. Do we have any test coverage covering this case?

@@ +1722,5 @@
> +        bool ok = MorphSlimWrapper(ccx, parentObj);
> +        NS_ENSURE_TRUE(ok, NS_ERROR_FAILURE);
> +    }
> +
> +    // Get the WN corresponding to the parent, and recursively fix it up.

This comment is obsolete.

@@ +1726,5 @@
> +    // Get the WN corresponding to the parent, and recursively fix it up.
> +    rv = RescueOrphans(ccx, parentObj);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    parentObj = js::GetObjectParent(obj);

Why do we need to reassign parentObj here?

@@ +1751,4 @@
>  }
>  
>  // Recursively fix up orphans on the parent chain of a wrapper. Note that this
>  // can cause a wrapper to move even if IsOrphan() is false, since its parent

IsOrphan() is gone now.

Comment 4

[Peter Van der Beken [:peterv]]

The SOW patch is in bug 815149, you're welcome to review it too ;-).

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 691070 [details] [diff] [review]
v1

So we're now null-checking "self" for all finalizers?  Why do we need to do that?

Assuming we do, r=me on the codegen bits.

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 691070 [details] [diff] [review]
v1

I've now looked at the SOW patch enough to know what it does (though not carefully enough to constitute a review).

>+    }
>+
>+    // Before proceeding, eagerly create any same-compartment security wrappers
>+    // that the object might have. This forces us to take the 'WithWrapper' path
>+    // while transplanting that handles this stuff correctly.
>+    JSObject* ww;
>+    {
>+      JSAutoCompartment innerAC(aCx, aOldGlobalObject);
>+      ww = xpc::WrapperFactory::WrapForSameCompartment(aCx, obj);
>+      if (!ww) {
>+        return NS_ERROR_FAILURE;
>+      }

I'm concerned about passing obj into XPConnect for security wrapping after we've already nulled out the private. I'd prefer to do this earlier in the function.

Comment 7

[Peter Van der Beken [:peterv]]

Attachment: v1.1

(In reply to :Ms2ger from comment #1)
> { on the next line

> * to the left

Done.

> The first half of this function just returns failure, why not here?

Because it's too hard to recover.

> Fun. Can we make this a compiler error?

Not really.

> We're not terrible consistent about this, but 'a' for arguments please

Done.

(In reply to Bobby Holley (:bholley) from comment #3)
> I know this is unrelated to your change, but what the heck is this about?

If we fail we revert the adoption.

> This comment is obsolete.

Removed.

> Can you give me an example of the same-compartment case?

We always reparent after adopting, even when adopting same-compartment (note that we can also have multiple documents per compartment). I made the same-compartment case bail out quickly.

> I found these sentences a bit hard to parse, especially the first one.

Rewrote it a bit.

> Why is this necessary? It looks like JS_CloneObject ends up invoking
> CopySlots.

Only for proxies, no?

> PostTransplant is used for <plugin> only. Is that being converted to the new
> bindings? Is there a bug on it? We'll need to file a blocking bug to sort
> out the plugin proto fixup.

Will file a bug, but we're not converting <plugin> right now.

> Hm, why do we need aOldGlobalObject and aNewGlobalObject? Shouldn't obj and
> aNewParent be enough?
> 
> Also, aObj for consistency I'd think.
> 
> I think I'd switch the order too. So:
> ReparentWrapper(JSContext* aCx, JSObject* aObj, JSObject* aNewParent).

I ended up needing only aCx and aObj.

> So in that case, should we at least align the names here, with mGetParent
> and mGetProto? I don't have a super strong opinion, I just want to make sure
> we give it some thought.

Done.

> So, the support for new binding objects here only takes effect when said
> objects appear on the parent chain of XPCWNs, right?
> 
> I want to have a detailed discussion about this, so any r+ is contingent on
> us having that.

I think we ended up deciding it was ok.

> > +        } else {
> > +            MOZ_ASSERT(IsDOMObject(obj));
> > +            const DOMClass* domClass = GetDOMClass(obj);
> > +            parentObj = domClass->mGetParentObject(ccx, obj);
> > +        }
> 
> Yikes. Do we have any test coverage covering this case?

> This comment is obsolete.

Removed.

> Why do we need to reassign parentObj here?

Removed.

(In reply to Boris Zbarsky (:bz) from comment #5)
> So we're now null-checking "self" for all finalizers?  Why do we need to do
> that?

In some failure cases we can end up with a clone that doesn't have a pointer to the native, it's not referenced by anything but of course it can be destroyed.

(In reply to Bobby Holley (:bholley) from comment #6)
> I'm concerned about passing obj into XPConnect for security wrapping after
> we've already nulled out the private. I'd prefer to do this earlier in the
> function.

Moved up and replaced the JSAutoCompartment to a MOZ_ASSERT.

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: v1.2

(In reply to Peter Van der Beken [:peterv] from comment #7)
> Moved up and replaced the JSAutoCompartment to a MOZ_ASSERT.

That was pretty bogus, readded the JSAutoCompartment. This one's green on try.

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 694381 [details] [diff] [review]
v1.2

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

> (In reply to Peter Van der Beken [:peterv] from comment #7)
> > Moved up and replaced the JSAutoCompartment to a MOZ_ASSERT.
> 
> That was pretty bogus, readded the JSAutoCompartment. This one's green on
> try.

Does that mean that we might be in an arbitrary compartment when entering the function? I think it's generally safer to be sure about the compartment we're in if we can (for example, the early-return might hit a compartment mismatch for the JS_SetParent). Can you enter the old compartment at the top of the function before doing anything else, or assert it and require it of the API consumers? No need to add extra braces - the destructor ordering should take care of it.

r=bholley with that :-)

Comment 10

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9edcce2eabae

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/9edcce2eabae