949108 - Rooting hazards in nsScriptLoader.cpp due to AutoPushJSContext

Status: RESOLVED FIXED

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

Description

[Steve Fink [:sfink] [:s:]]

Hazard:

Function 'uint32 nsScriptLoader::EvaluateScript(nsScriptLoadRequest*, nsString*, void**)' has unrooted 'unrootedGlobal' of type 'JSObject*' live across GC call 'void mozilla::AutoPushJSContext::AutoPushJSContext(JSContext*)' at content/base/src/nsScriptLoader.cpp:1005

The explanation for why AutoPushJSContext can GC is bogus, so I won't paste it here. But it *can* GC, at least in general, because it constructs a Maybe<AutoCxPusher>, which calls GetScriptContextFromJSContext and other things. At least, it does enough scary stuff that annotating it as never GC'ing doesn't feel safe.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Rooting hazards in nsScriptLoader.cpp due to AutoPushJSContext

A horrible way to paper over the problem -- re-fetch the global. This maybe wouldn't be so bad if I weren't doing it via GetScriptContext. But in the off chance that this is all cheap enough (and cold enough) to not matter, r? bz.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8346077 [details] [diff] [review]
Rooting hazards in nsScriptLoader.cpp due to AutoPushJSContext

How about we just root before pushing?  That's a totally reasonable thing to do, and I think the right thing to do here.  So:

  JSContext* unpushedCx = context->GetNativeContext();
  JS::Rooted<JSObject*> global(unpushedCx, unrootedGlobal);

  AutoPushJSContext cx(unpushedCx);

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: Rooting hazards in nsScriptLoader.cpp due to AutoPushJSContext

So much nicer. I should've seen that. Thanks!

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8346089 [details] [diff] [review]
Rooting hazards in nsScriptLoader.cpp due to AutoPushJSContext

r=me

Comment 5

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/926aeca75e6c

Comment 6

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/f015670e3573 - backed out 926aeca75e6c

Comment 7

[Steve Fink [:sfink] [:s:]]

Backed out because it asserts that we're trying to root while not in a request (see bug 883450). bholley, looks like I'm trespassing in your playground. How do you think we should handle this?

Comment 8

[Bobby Holley (:bholley)]

Yeah, thank heaven for those assertions.

If these are the only issues like this, we an just construct a JSAutoRequest ar(unpushedContext); on the stack, before the rooted. That work?

Comment 9

[Steve Fink [:sfink] [:s:]]

It's an accumulation of cruft, but sure. Still far better than my original hack. Thanks!

Comment 10

[Steve Fink [:sfink] [:s:]]

I am attempting to re-land this. My first try push is https://tbpl.mozilla.org/?tree=Try&rev=5b165374da4d which admittedly looks a bit grim for mochitest-bc, especially since I didn't see these failures on the base inbound revision. I just pushed https://tbpl.mozilla.org/?tree=Try&rev=388757310054 in hopes that a rebase will be prettier.

Comment 11

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e6f7273d46e9

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e6f7273d46e9

Comment 14

[Terrence Cole [:terrence]]

The analysis seems to think that JSAutoRequest can GC, so this puts us right back where we were.

Comment 15

[Bobby Holley (:bholley)]

(In reply to Terrence Cole [:terrence] from comment #14)
> The analysis seems to think that JSAutoRequest can GC, so this puts us right
> back where we were.

Uh, given that you need to be in a request in order to root anything, how the hell does that work?

Comment 16

[Terrence Cole [:terrence]]

(In reply to Bobby Holley (:bholley) from comment #15)
> (In reply to Terrence Cole [:terrence] from comment #14)
> > The analysis seems to think that JSAutoRequest can GC, so this puts us right
> > back where we were.
> 
> Uh, given that you need to be in a request in order to root anything, how
> the hell does that work?

I'm guessing horrible hacks at some level.

This particular case appears to be due to the fieldCall to JSRuntime::activityCallback. In the browser this just pokes the watchdog, so I think we should suppress GC across the call. Patch in a moment.

Comment 17

[Terrence Cole [:terrence]]

Attachment: suppress_across_activity_callback-v0.diff

Comment 18

[Bobby Holley (:bholley)]

Oh, and please add a comment in triggerActivityCallback explaining why we're suppressing GC.

Comment 19

[Terrence Cole [:terrence]]

Attachment: suppress_across_activity_callback-vReady.diff

Comment 20

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/77bd9575650b

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/77bd9575650b