Closed Bug 669228 Opened 8 years ago Closed 8 years ago
Crash with Install
Trigger, moz Request Animation Frame, throwing proxy
###!!! 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.
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?
Blake, can you have a look here?
Assignee: nobody → mrbkap
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).
I don't know of a way to test this... and it's pretty ugly.
Attachment #544386 - Flags: review?(gal)
blocking2.0: --- → -
Andreas, can you review this security bug?
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?
Whiteboard: [sg:critical?] → [sg:critical?][needs r+ from gal]
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)
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?
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.
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?
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+
Whiteboard: [sg:critical?][needs r+ from gal] → [sg:critical?][inbound]
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical?][inbound] → [sg:critical?]
Target Milestone: --- → mozilla8
Comment on attachment 548644 [details] [diff] [review] Proposed fix v2 This patch should be safe enough to take on the branches.
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.
Since the beta approval for the fix was denied we won't be fixing this for 6.
Whiteboard: [sg:critical?] → [sg:critical?][needs aurora landing for Fx7]
http://hg.mozilla.org/releases/mozilla-aurora/rev/25b41e0dcd7d Also marking as being fixed in Firefox 8, since this is fixed on trunk.
qa- as no QA fix verification needed
Whiteboard: [sg:critical?][needs aurora landing for Fx7] → [sg:critical?][needs aurora landing for Fx7][qa-]
Did the testcase land?
You need to log in before you can comment on or make changes to this bug.