Get rid of JS_SetParent

RESOLVED FIXED in Firefox 39

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla39
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(2 attachments)

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?
Bobby, _am_ I missing something?
Blocks: 805052
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
> 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!
Created attachment 8570101 [details] [diff] [review]
part 1.  Get rid of JS_SetParent uses in DOM/XPConnect
Attachment #8570101 - Flags: review?(bobbyholley)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Created attachment 8570102 [details] [diff] [review]
part 2.  Remove JS_SetParent
Attachment #8570102 - Flags: review?(jwalden+bmo)
Attachment #8570101 - Flags: review?(bobbyholley) → review+

Updated

3 years ago
Attachment #8570102 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/cec80d0c3a02
https://hg.mozilla.org/mozilla-central/rev/d4516fac0a6f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.