Rooting hazards in nsScriptLoader.cpp due to AutoPushJSContext

RESOLVED FIXED in mozilla29

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

unspecified
mozilla29
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8346077 [details] [diff] [review]
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.
Attachment #8346077 - Flags: review?(bzbarsky)
(Assignee)

Updated

4 years ago
Blocks: 898606
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);
(Assignee)

Comment 3

4 years ago
Created attachment 8346089 [details] [diff] [review]
Rooting hazards in nsScriptLoader.cpp due to AutoPushJSContext

So much nicer. I should've seen that. Thanks!
Attachment #8346089 - Flags: review?(bzbarsky)
(Assignee)

Updated

4 years ago
Attachment #8346077 - Attachment is obsolete: true
Attachment #8346077 - Flags: review?(bzbarsky)
Comment on attachment 8346089 [details] [diff] [review]
Rooting hazards in nsScriptLoader.cpp due to AutoPushJSContext

r=me
Attachment #8346089 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 5

4 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/926aeca75e6c
(Assignee)

Comment 6

4 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/f015670e3573 - backed out 926aeca75e6c
(Assignee)

Comment 7

4 years ago
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?
Flags: needinfo?(bobbyholley+bmo)
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?
Flags: needinfo?(bobbyholley+bmo)
(Assignee)

Comment 9

4 years ago
It's an accumulation of cruft, but sure. Still far better than my original hack. Thanks!
(Assignee)

Comment 10

4 years ago
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.
(Assignee)

Comment 11

4 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/e6f7273d46e9
https://hg.mozilla.org/mozilla-central/rev/e6f7273d46e9
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Duplicate of this bug: 931991
The analysis seems to think that JSAutoRequest can GC, so this puts us right back where we were.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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?
(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.
Created attachment 8348166 [details] [diff] [review]
suppress_across_activity_callback-v0.diff
Attachment #8348166 - Flags: review?(bobbyholley+bmo)
Attachment #8348166 - Flags: review?(bobbyholley+bmo) → review+
Oh, and please add a comment in triggerActivityCallback explaining why we're suppressing GC.
Created attachment 8348194 [details] [diff] [review]
suppress_across_activity_callback-vReady.diff
Attachment #8348166 - Attachment is obsolete: true
Attachment #8348194 - Flags: review+
Whiteboard: [checkin-needed]
Attachment #8348194 - Flags: checkin?
https://hg.mozilla.org/integration/mozilla-inbound/rev/77bd9575650b
Whiteboard: [checkin-needed]
Attachment #8348194 - Flags: checkin?
https://hg.mozilla.org/mozilla-central/rev/77bd9575650b
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.