Closed Bug 669228 Opened 13 years ago Closed 13 years ago

Crash with InstallTrigger, mozRequestAnimationFrame, throwing proxy

Categories

(Core :: XPConnect, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla8
Tracking Status
firefox5 - wontfix
firefox6 - wontfix
firefox7 + fixed
firefox8 + fixed
blocking2.0 --- -
status2.0 --- wontfix
status1.9.2 --- unaffected
status1.9.1 --- unaffected

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)

###!!! 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
Whiteboard: [sg:critical?]
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).
Attached patch Hacky fix (obsolete) — Splinter Review
I don't know of a way to test this... and it's pretty ugly.
Attachment #544386 - Flags: review?(gal)
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.
Attachment #544386 - Flags: review?(jorendorff)
Attached patch Proposed fix v2Splinter Review
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 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+
http://hg.mozilla.org/integration/mozilla-inbound/rev/26e80814276e
Whiteboard: [sg:critical?][needs r+ from gal] → [sg:critical?][inbound]
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
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 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+
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-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.