Closed
Bug 669228
Opened 13 years ago
Closed 13 years ago
Crash with InstallTrigger, mozRequestAnimationFrame, throwing proxy
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: jruderman, Assigned: mrbkap)
Details
(Keywords: assertion, crash, testcase, Whiteboard: [sg:critical?][needs aurora landing for Fx7][qa-])
Crash Data
Attachments
(2 files, 1 obsolete file)
379 bytes,
text/html
|
Details | |
6.66 KB,
patch
|
jorendorff
:
review+
asa
:
approval-mozilla-aurora+
asa
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
###!!! ASSERTION: No scope has this global object!: 'OKIfNotInitialized', file /builds/slave/m-cen-osx64-dbg/build/js/src/xpconnect/src/xpcwrappednativescope.cpp, line 840 >XPCWrappedNativeScope::FindInJSObjectScope [js/src/xpconnect/src/xpcwrappednativescope.cpp:841] >XPCWrappedNativeScope::FindInJSObjectScope [js/src/xpconnect/src/xpcprivate.h:1503] >GetContextFromObject [js/src/xpconnect/src/xpcwrappedjsclass.cpp:574] >nsXPCWrappedJSClass::CallMethod [js/src/xpconnect/src/xpcwrappedjsclass.cpp:1289] >nsXPCWrappedJS::CallMethod [js/src/xpconnect/src/xpcwrappedjs.cpp:585] >PrepareAndDispatch [xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_darwin.cpp:153] Debug crash: [@ XPCWrappedNativeScope::GetContext | GetContextFromObject] Opt crash: bp-be925f53-6a18-40b4-9829-a9a0a2110704 Security-sensitive for now because this involves a chrome-content boundary.
Comment 1•13 years ago
|
||
So we end up passing the Proxy to FindInJSObjectScope. The parent of the Proxy is an object with JSClass "jdummy", which has no parent. So that's the global we find.... It looks like this global would come from a JS_EnterCrossCompartmentCallScript when target->u.object is null. The places where we do this are: 1) evalInSandbox, apparently. 2) XPCThrower::ThrowBadParam calling XPCThrower::BuildAndThrowException which tries to create an exception, which calls nsXPConnect::GetCurrentJSStack which calls XPCJSStackFrame::CreateStack which tries to enter a compartment. #2 is where the global object in question comes from.... Is the issue here that we're pushing a null JSContext on the stack when invoking requestAnimationFrame callbacks and that gets XPConnect confused after all?
Comment 2•13 years ago
|
||
Blake, can you have a look here?
Assignee: nobody → mrbkap
Whiteboard: [sg:critical?]
Assignee | ||
Comment 3•13 years ago
|
||
This was a bit of a pain to track down. The testcase, as written is actually "fixed" by bug 634156. However, that fix is a workaround for the real problem (which I'm not yet sure how to fix). the problem, as bz found, is that we're ending up with a proxy whose parent is not an XPConnect global object. Then, later, we attempt to "call" the proxy, and XPConnect crashes. The way that we end up with a proxy parented to a jdummy is that we have an exception on the stack when XPConnect calls JS_EnterCrossCompartmentCallScript. That, in turn calls wrapPendingException which, in order to avoids leaks, calls setParent on the proxy. The easiest short-term fix is to avoid the setParent call when we're about to use the jdummy global. The longer-term fix would be to not give proxies parents at all (which will actually be possible with compartment-per-global).
Assignee | ||
Comment 4•13 years ago
|
||
I don't know of a way to test this... and it's pretty ugly.
Attachment #544386 -
Flags: review?(gal)
Updated•13 years ago
|
blocking2.0: --- → -
status1.9.1:
--- → unaffected
status1.9.2:
--- → unaffected
status-firefox5:
--- → wontfix
status-firefox6:
--- → affected
status-firefox7:
--- → affected
status-firefox8:
--- → affected
tracking-firefox5:
--- → -
tracking-firefox6:
--- → +
tracking-firefox7:
--- → +
tracking-firefox8:
--- → +
Comment 5•13 years ago
|
||
Andreas, can you review this security bug?
Comment 6•13 years ago
|
||
Dveditz, would we consider a "pretty ugly" fix that's difficult to test for Firefox 6 with only a few weeks to go in the cycle?
Updated•13 years ago
|
Whiteboard: [sg:critical?] → [sg:critical?][needs r+ from gal]
Comment 7•13 years ago
|
||
Comment on attachment 544386 [details] [diff] [review] Hacky fix Jason, could you help review this sg:critical bug please? Andreas is overloaded atm.
Attachment #544386 -
Flags: review?(gal) → review?(jorendorff)
Comment 8•13 years ago
|
||
Bear with me -- I don't really understand what's going on here, so I have a bunch of dumb questions. Was Proxy.create necessary to trigger this? Why? From what I can gather, the problem is that XPConnect crashes when it tries to call a certain kind of wrapped JSObject. If that's right, here are two approaches I might have tried: - Make it an invariant of nsXPCWrappedJS that the wrapped JSObject is parented to an XPConnect global. So, make the constructor fail gracefully if it is asked to wrap an object like this one. - Make nsXPCWrappedJS able to rwap such objects safely. So, make something under nsXPCWrappedJS::CallMethod detect this case and fail gracefully. It looks like this patch is trying to make such objects not exist. Is that right? Precisely what kind of JSObject is problematic? *Any* object that's not parented to an XPConnect global?
Comment 9•13 years ago
|
||
So the answer to the main point in comment 8 (about invariants) is that XPConnect assumes that all objects are parented to globals it knows about. It's a deeply rooted assumption. It would be totally impractical to enforce it at runtime every place any code passes a JSObject to XPConnect. mrbkap and I talked about this at some length, and we ended up here: <jorendorff> how about just getting rid of the AutoEnterScriptCompartment use in XPCJSStackFrame? <jorendorff> mrbkap: seems like you could expose AutoEnterFrameCompartment instead, which could use the frame's scope chain instead (?) <mrbkap> mm <mrbkap> jorendorff: how about both? <jorendorff> mrbkap: yeah, ok <jorendorff> mrbkap: definitely <jorendorff> the other AutoEnterScriptCompartment user is jsd, so <jorendorff> we'll get there <jorendorff> mrbkap: jdummy's days being numbered seems like a necessary ingredient to me <jorendorff> but in the meantime, we'll be extra careful with it -- fair enough <mrbkap> jorendorff: so, with compartment per global, we might be able to get rid of parent for proxies altogether. <mrbkap> Yeah.
Updated•13 years ago
|
Attachment #544386 -
Flags: review?(jorendorff)
Assignee | ||
Comment 10•13 years ago
|
||
I think this addresses your comments, jason. Let me know if I forgot anything. I also went back and forth on how/where to expose JS_EnterCrossCompartmentCallFrame/AutoEnterFramecompartment. Let me know if you have any comments on that. Is this going to get better with jsdbgapi2?
Attachment #544386 -
Attachment is obsolete: true
Attachment #548644 -
Flags: review?(jorendorff)
Comment 11•13 years ago
|
||
Comment on attachment 548644 [details] [diff] [review] Proposed fix v2 Yep, that's it. No comments on AutoEnterFrameCompartment; this seems fine. Removing JSD will get rid of the one remaining use of AutoEnterScriptCompartment. So jsdbg2 is moving us in that direction. Separately, compartment-per-global would kill this problem. So either one.
Attachment #548644 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 12•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/26e80814276e
Whiteboard: [sg:critical?][needs r+ from gal] → [sg:critical?][inbound]
Comment 13•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/26e80814276e
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical?][inbound] → [sg:critical?]
Target Milestone: --- → mozilla8
Assignee | ||
Comment 14•13 years ago
|
||
Comment on attachment 548644 [details] [diff] [review] Proposed fix v2 This patch should be safe enough to take on the branches.
Attachment #548644 -
Flags: approval-mozilla-beta?
Attachment #548644 -
Flags: approval-mozilla-aurora?
Comment 15•13 years ago
|
||
Comment on attachment 548644 [details] [diff] [review] Proposed fix v2 Please land this on Aurora after it's had a few more days on m-c. I was gonna hold off until the next triage on Tuesday to mark it but we can all save a little time if I go ahead and mark it now. Thanks.
Attachment #548644 -
Flags: approval-mozilla-beta?
Attachment #548644 -
Flags: approval-mozilla-beta-
Attachment #548644 -
Flags: approval-mozilla-aurora?
Attachment #548644 -
Flags: approval-mozilla-aurora+
Comment 16•13 years ago
|
||
Since the beta approval for the fix was denied we won't be fixing this for 6.
Updated•13 years ago
|
Whiteboard: [sg:critical?] → [sg:critical?][needs aurora landing for Fx7]
Assignee | ||
Comment 17•13 years ago
|
||
http://hg.mozilla.org/releases/mozilla-aurora/rev/25b41e0dcd7d Also marking as being fixed in Firefox 8, since this is fixed on trunk.
Comment 18•13 years ago
|
||
qa- as no QA fix verification needed
Whiteboard: [sg:critical?][needs aurora landing for Fx7] → [sg:critical?][needs aurora landing for Fx7][qa-]
Updated•12 years ago
|
Group: core-security
Did the testcase land?
You need to log in
before you can comment on or make changes to this bug.
Description
•