1136980 - Get rid of JS_SetParent

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Afaik, it's dead code.

We only have two callers, once bug 1136292 is fixed:

1)  nsGlobalWindow::SetNewDocument is doing:

        JS_SetParent(cx, outerObject, newInnerGlobal);

but parents are same-compartment, outerObject is a proxy, and proxies are always parented to globals once bug 1136925 is fixed.  So afaict the parent of outerObject here should already be newInnerGlobal, no?

2)  JSObject2JSObjectMap::Reparent calls it like so:

                 if (inner == aNewInner && inner != parent)
                     JS_SetParent(aCx, wrapper, aNewInner);

where inner and aNewInner are guaranteed to be inner windows and "parent" is the JS_GetParent of whatever is in mWaiverWrapperMap.  But, again, given that parents are same-compartment, how can we have inner != parent?

Or am I missing something?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Bobby, _am_ I missing something?

Comment 2

[Bobby Holley (:bholley)]

(In reply to Not doing reviews right now from comment #0)
> Afaik, it's dead code.
> 
> We only have two callers, once bug 1136292 is fixed:
> 
> 1)  nsGlobalWindow::SetNewDocument is doing:
> 
>         JS_SetParent(cx, outerObject, newInnerGlobal);
> 
> but parents are same-compartment, outerObject is a proxy, and proxies are
> always parented to globals once bug 1136925 is fixed.  So afaict the parent
> of outerObject here should already be newInnerGlobal, no?

Well, the key line here is:

outerObject = xpc::TransplantObject(cx, obj, outerObject);

Given how brain transplanting works, this means that outerObject is not necessarily its original value - it may have been gut-swapped with cross-compartment wrapper in the target scope, which then gets returned by TransplantObject. But as long as cross-compartment wrappers are always parented to globals, I agree that we should be guaranteed to end up with an outer parented to its current inner. Please assert this.


> 2)  JSObject2JSObjectMap::Reparent calls it like so:
> 
>                  if (inner == aNewInner && inner != parent)
>                      JS_SetParent(aCx, wrapper, aNewInner);
> 
> where inner and aNewInner are guaranteed to be inner windows and "parent" is
> the JS_GetParent of whatever is in mWaiverWrapperMap.  But, again, given
> that parents are same-compartment, how can we have inner != parent?

Hm. The caller does

XPCWrappedNativeScope* scope = xpc::ObjectScope(outerObject);

after the transplant, which will put us in the scope of newInnerGlobal, and then it passes newInnerGlobal to Reparent. That inner is now current, so this whole setup isn't going to do anything at all.

So yes, please just remove the whole Reparent machinery.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> But as long as cross-compartment wrappers are always parented to globals

After bug 1136925 all proxies are always parented to globals.

I'll add an assert, but that will end up dying when GetObjectParent/JS_GetParent die, of course.

> So yes, please just remove the whole Reparent machinery.

Sweet!

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Get rid of JS_SetParent uses in DOM/XPConnect

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Remove JS_SetParent

Comment 6

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cec80d0c3a02
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4516fac0a6f

Comment 7

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/cec80d0c3a02
https://hg.mozilla.org/mozilla-central/rev/d4516fac0a6f