Closed Bug 858107 Opened 7 years ago Closed 7 years ago

GC: Some more rooting in XPConnect

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(2 files, 4 obsolete files)

No description provided.
Attachment #733378 - Flags: review?(bobbyholley+bmo)
Attached patch 2 - Misc rooting in XPConnect (obsolete) — Splinter Review
Attachment #733381 - Flags: review?(bobbyholley+bmo)
Comment on attachment 733378 [details] [diff] [review]
1 - Add an autorooter for XPCCallContext

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

What is CustomAutoRooter? I don't see it anywhere in the tree and there are no comments in the patch.
Attachment #733378 - Flags: review?(bobbyholley+bmo)
Comment on attachment 733381 [details] [diff] [review]
2 - Misc rooting in XPConnect

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

Looks like this patch uses the XPCCallContext above. Why does it need to be a separate class? It seems kind of like a footgun to have to root it separately. Does it need to happen for every XPCCallContext? If so, why not force it in the constructor?
Attachment #733381 - Flags: review?(bobbyholley+bmo)
(In reply to Bobby Holley (:bholley) from comment #2)
> Comment on attachment 733378 [details] [diff] [review]
> What is CustomAutoRooter? I don't see it anywhere in the tree and there are
> no comments in the patch.

Looks like it got backed out briefly: see bug 855350.
(In reply to Bobby Holley (:bholley) from comment #2)

> What is CustomAutoRooter?

Apologies - I only just landed it on inbound before I asked for review for this patch.

As Terrence said it's in bug 855350, or here: https://mxr.mozilla.org/mozilla-central/source/js/src/jsapi.h#664

Re XPCCallContext::AutoRooter - there is a small runtime cost to rooting, so the current thinking is to only root things where the static analysis tells us it's necessary (which is when they are in scope over a call to something that can trigger a GC).

Also, XPCCallContext doesn't always seem to have a JSContext*, and this would be required in the constructor to make it root itself automatically.
(In reply to Jon Coppeard (:jonco) from comment #5)
 
> Re XPCCallContext::AutoRooter - there is a small runtime cost to rooting, so
> the current thinking is to only root things where the static analysis tells
> us it's necessary (which is when they are in scope over a call to something
> that can trigger a GC).

I'd rather not have to rely on the static analysis in order to know during development / reviews whether this rooting magic is necessary or not.

> Also, XPCCallContext doesn't always seem to have a JSContext*, and this
> would be required in the constructor to make it root itself automatically.

Why not have a Maybe<> of the auto-rooter that gets constructed in XPCCallContext::Init iff we have both a cx and and |obj|? These should be the cases where we need to root, AFAICT. We'd just need to construct it after the null check for obj in XPCCallContext::Init.
(In reply to Bobby Holley (:bholley) from comment #6)
> (In reply to Jon Coppeard (:jonco) from comment #5)
>  
> > Re XPCCallContext::AutoRooter - there is a small runtime cost to rooting, so
> > the current thinking is to only root things where the static analysis tells
> > us it's necessary (which is when they are in scope over a call to something
> > that can trigger a GC).
> 
> I'd rather not have to rely on the static analysis in order to know during
> development / reviews whether this rooting magic is necessary or not.

Yes, I think we should just root everything in the browser and only worry about performance problems if and when they crop up.

> > Also, XPCCallContext doesn't always seem to have a JSContext*, and this
> > would be required in the constructor to make it root itself automatically.
> 
> Why not have a Maybe<> of the auto-rooter that gets constructed in
> XPCCallContext::Init iff we have both a cx and and |obj|? These should be
> the cases where we need to root, AFAICT. We'd just need to construct it
> after the null check for obj in XPCCallContext::Init.

The rooted data structure is a singly-linked list: construction and destruction of Rooted objects *must* happen in a stacky manner. The worry with using Maybe is code like:

{
  Maybe<Rooted> foo1;
  Rooted foo2(cx);
  // push foo2
  foo1.construct(cx);
  // push foo1
  // pop foo2
  // pop foo1 <-- Crash!
}

Unfortunately, it's quite hard to add an assertion to make this usage easy to debug, so introducing these sorts of potential errors is somewhat worrysome.

Something that may be more palatable here is to make Init do JS_AddRoot and ~XPCCallContext do JS_RemoveRoot.
(In reply to Terrence Cole [:terrence] from comment #7)
> Something that may be more palatable here is to make Init do JS_AddRoot and
> ~XPCCallContext do JS_RemoveRoot.

At that point, shouldn't we just use a RootedObject for the 2 JSObjects here? XPCCallContext is only ever constructed on the stack. And initializing a RootedObject to null is fast, right?

Oh I see, the issue is that we might not have a JSContext at initialization time. I think I can fix that. sec.
Actually, the reviewer mechanics of this will get more cumbersome if I write the patch. ;-)

So anyway. Just write a patch that factors out the JSContext selection from XPCCallContext::Init into a helper function that we can invoke in the constructor when initializing the cx. If that seems like too much work, you could try just initializing your rooted objects with (cx ? cx : AutoJSContext()), which I _think_ would probably be fine.
(In reply to Bobby Holley (:bholley) from comment #9)
> Actually, the reviewer mechanics of this will get more cumbersome if I write
> the patch. ;-)
> 
> So anyway. Just write a patch that factors out the JSContext selection from
> XPCCallContext::Init into a helper function that we can invoke in the
> constructor when initializing the cx. If that seems like too much work, you
> could try just initializing your rooted objects with (cx ? cx :
> AutoJSContext()), which I _think_ would probably be fine.

Ah, right, I had forgotten that we can just get a random JSContext! That pretty much solves all of our problems: we just use the cx as a place to hang the root list and we iterate all contexts for exact roots on every GC.

Thanks Bobby! This should come out much nicer now.
(In reply to Terrence Cole [:terrence] from comment #10)
> Ah, right, I had forgotten that we can just get a random JSContext!

Well, AutoJSContext here won't be random - it's the exact same JSContext that will end up getting computed if no cx is passed (stack-top cx if it exists, SafeJSContext otherwise).
It seems it's not possible to factor out the selection of the JSContext* from Init() because it can fail to give an answer, e.g. if mXPC is NULL Init() just returns.

Using AutoJSContext() as described is also problematic because it has an assertion that it is not used a a temporary.  Presumably this is because it can return a context that it pushed onto the context stack in its constructor that will have already been popped of by its destructor before it can be used.

So I have updated the patch as suggested in comment 6, using a Maybe<AutoRooter>.

I don't think the problem Terrence describes in comment 7 is possible in this case because XPCCallContext::Init() is always called in the constructor, so there's no possibility for a separate root to be constructed between the outer Maybe object being constructed and Maybe::contruct() constructing the inner root object if that happens.
Attachment #733378 - Attachment is obsolete: true
Attachment #734612 - Flags: review?(bobbyholley+bmo)
Attached patch 2 - Misc rooting in XPConnect (obsolete) — Splinter Review
Attachment #733381 - Attachment is obsolete: true
Attachment #734613 - Flags: review?(bobbyholley+bmo)
Comment on attachment 734612 [details] [diff] [review]
1 - Add an autorooter for XPCCallContext

(In reply to Jon Coppeard (:jonco) from comment #12)
> Created attachment 734612 [details] [diff] [review]
> 1 - Add an autorooter for XPCCallContext
> 
> It seems it's not possible to factor out the selection of the JSContext*
> from Init() because it can fail to give an answer, e.g. if mXPC is NULL
> Init() just returns.
> 
> Using AutoJSContext() as described is also problematic because it has an
> assertion that it is not used a a temporary.  Presumably this is because it
> can return a context that it pushed onto the context stack in its
> constructor that will have already been popped of by its destructor before
> it can be used.

Ah, ok. But a random JSContext is totally fine, right? If so, let's just use the SafeJSContext (XPCJSRuntime::Get()->GetJSContextStack()->GetSafeJSContext()) and use RootedObject.
Attachment #734612 - Flags: review?(bobbyholley+bmo)
Comment on attachment 734613 [details] [diff] [review]
2 - Misc rooting in XPConnect

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

r=bholley if we can remove the AutoRooter in SandboxOptions as suggested. If not, I want to look at that part again.

::: js/xpconnect/src/XPCWrappedNativeProto.cpp
@@ +87,5 @@
>          jsclazz = &XPC_WN_NoMods_NoCall_Proto_JSClass;
>      }
>  
> +    JS::RootedObject parent(ccx, mScope->GetGlobalJSObject());
> +    JS::RootedObject proto(ccx, JS_GetObjectPrototype(ccx, parent));

This really seems like it shouldn't be necessary. Shouldn't JS_GetObjectPrototype return an handle which would allow us to pass it straight to JS_NewObjectWithUniqueType?

::: js/xpconnect/src/XPCWrappedNativeScope.cpp
@@ +206,5 @@
>          return nullptr;
>  
>      // The call to wrap() here is necessary even though the object is same-
>      // compartment, because it applies our security wrapper.
> +    JS::RootedObject obj(ccx.GetJSContext(), wrapper->GetFlatJSObject());

ccx should implicitly cast to JSContext with some operator overloading magic.

::: js/xpconnect/src/xpcprivate.h
@@ +4179,5 @@
>          , wantComponents(true)
>          , wantXHRConstructor(false)
>          , proto(NULL)
>          , sameZoneAs(NULL)
> +        , rooter(cx, this)

This is a stack-only. Can't we just use RootedObject for proto and sameZoneAs?

@@ +4203,5 @@
> +            if (so->sameZoneAs)
> +                traceObject(trc, &so->sameZoneAs, "SandboxOptions::sameZoneAs");
> +        }
> +
> +        SandboxOptions *so;;

Double semicolon.
Attachment #734613 - Flags: review?(bobbyholley+bmo) → review+
(In reply to Bobby Holley (:bholley) from comment #15)
> ::: js/xpconnect/src/XPCWrappedNativeProto.cpp
> @@ +87,5 @@
> >          jsclazz = &XPC_WN_NoMods_NoCall_Proto_JSClass;
> >      }
> >  
> > +    JS::RootedObject parent(ccx, mScope->GetGlobalJSObject());
> > +    JS::RootedObject proto(ccx, JS_GetObjectPrototype(ccx, parent));
> 
> This really seems like it shouldn't be necessary. Shouldn't
> JS_GetObjectPrototype return an handle which would allow us to pass it
> straight to JS_NewObjectWithUniqueType?

We've tried a couple times to return handles to things that are not on the stack: every time so far has ended in catastrophe. I'm guessing that this particular case would not be problematic -- I can't see the global getting collected with a user on stack, even on error paths -- but I'd really prefer to keep our "never return handles" rule intact. If this is a pervasive problem, I'd certainly reconsider, but it looks like it won't be if we make js::GetObjectCompartment and JSAutoCompartment take a JSObject*.
Attachment #734612 - Attachment is obsolete: true
Attachment #735166 - Flags: review?(bobbyholley+bmo)
Updated patch in line with comments and carried r+ forward.
Attachment #734613 - Attachment is obsolete: true
Attachment #735167 - Flags: review+
Comment on attachment 735166 [details] [diff] [review]
1 - Root XPCCallContext

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

::: js/xpconnect/src/XPCInlines.h
@@ +40,5 @@
>      return mState != INIT_FAILED;
>  }
>  
>  inline nsXPConnect*
> +

What's this about?
Attachment #735166 - Flags: review?(bobbyholley+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/53f30efb50b0
https://hg.mozilla.org/mozilla-central/rev/a2fbe8dc1ee1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.