Closed
Bug 949108
Opened 11 years ago
Closed 11 years ago
Rooting hazards in nsScriptLoader.cpp due to AutoPushJSContext
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: sfink, Assigned: sfink)
References
Details
(Whiteboard: [qa-])
Attachments
(2 files, 2 obsolete files)
2.36 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
4.24 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
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)
Comment 2•11 years ago
|
||
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•11 years ago
|
||
So much nicer. I should've seen that. Thanks!
Attachment #8346089 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•11 years ago
|
Attachment #8346077 -
Attachment is obsolete: true
Attachment #8346077 -
Flags: review?(bzbarsky)
Comment 4•11 years ago
|
||
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•11 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/926aeca75e6c
Assignee | ||
Comment 6•11 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/f015670e3573 - backed out 926aeca75e6c
Assignee | ||
Comment 7•11 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)
Comment 8•11 years ago
|
||
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•11 years ago
|
||
It's an accumulation of cruft, but sure. Still far better than my original hack. Thanks!
Assignee | ||
Comment 10•11 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•11 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/e6f7273d46e9
Comment 12•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e6f7273d46e9
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment 14•11 years ago
|
||
The analysis seems to think that JSAutoRequest can GC, so this puts us right back where we were.
Updated•11 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 15•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
Attachment #8348166 -
Flags: review?(bobbyholley+bmo)
Updated•11 years ago
|
Attachment #8348166 -
Flags: review?(bobbyholley+bmo) → review+
Comment 18•11 years ago
|
||
Oh, and please add a comment in triggerActivityCallback explaining why we're suppressing GC.
Comment 19•11 years ago
|
||
Attachment #8348166 -
Attachment is obsolete: true
Attachment #8348194 -
Flags: review+
Updated•11 years ago
|
Whiteboard: [checkin-needed]
Updated•11 years ago
|
Attachment #8348194 -
Flags: checkin?
Comment 20•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/77bd9575650b
Whiteboard: [checkin-needed]
Updated•11 years ago
|
Attachment #8348194 -
Flags: checkin?
https://hg.mozilla.org/mozilla-central/rev/77bd9575650b
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Whiteboard: [qa-]
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•