Debugger makeDebuggeeValue can crash Firefox

RESOLVED FIXED in Firefox 50

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Oriol, Assigned: till)

Tracking

({crash, regression})

50 Branch
mozilla50
crash, regression
Points:
---

Firefox Tracking Flags

(firefox47 unaffected, firefox48 unaffected, firefox49 unaffected, firefox50 fixed)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Steps to reproduce

1. Open a new tab
2. Open the web console (it is privileged)
3. Run this code

    Components.utils.import('resource://gre/modules/jsdebugger.jsm');
    addDebuggerToGlobal(this);
    var iframe = document.createElement("iframe");
    document.body.appendChild(iframe);

4. Run this code

    var dbg = new Debugger().addDebuggee(iframe.contentWindow);
    dbg.makeDebuggeeValue([1,2,3]);

Firefox crashes.

Running the two blocks of code simultaneously works properly.

I noticed something like this when working in bug 1282257 comment 7. The crash happened when I attempted to use the referent of the debugger object. For some reason it was falsy.

Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=469d01eebea4e2055553289ce6542fc093460bbd&tochange=a80fdfc128b0c29dc34dd3fc28868252111a5d52

Some crash reports:

https://crash-stats.mozilla.com/report/index/a4c5c15e-2a19-4107-86f5-58b6f2160717
https://crash-stats.mozilla.com/report/index/7d4528e6-e2c8-4e95-9f85-e3b812160717
https://crash-stats.mozilla.com/report/index/9d418389-fe86-4206-864d-48d402160717
Flags: needinfo?(till)
(Reporter)

Updated

2 years ago
Crash Signature: js::DebuggerObject::isPromise → [@ js::DebuggerObject::isPromise ]
(Reporter)

Comment 1

2 years ago
The code in comment 0 produces the crash because the console calls the isPromise getter.

Calling any promise getter manually produces the crash too.
Crash Signature: [@ js::DebuggerObject::isPromise ] → [@ js::DebuggerObject::isPromise ] [@ js::DebuggerObject::promiseStateGetter ] [@ js::DebuggerObject::promiseLifetimeGetter ] [@ js::DebuggerObject::promiseTimeToResolutionGetter ] [@ js::DebuggerObject::promiseAllocationSiteGetter ] [@ js::DebuggerObject…
(Assignee)

Comment 2

2 years ago
Created attachment 8771971 [details] [diff] [review]
Properly handle failure to unwrap cross-compartment wrappers in Promise-related DebuggerObject accessors

This code didn't properly handle failure to unwrap cross-compartment wrappers. I *think* just throwing should be fine everywhere, but will trigger a try run once I figure out how to do that from this machine.
Assignee: nobody → till
Status: NEW → ASSIGNED
Flags: needinfo?(till)
Attachment #8771971 - Flags: review?(nfitzgerald)
Comment on attachment 8771971 [details] [diff] [review]
Properly handle failure to unwrap cross-compartment wrappers in Promise-related DebuggerObject accessors

Review of attachment 8771971 [details] [diff] [review]:
-----------------------------------------------------------------

Care to MOZ_MUST_USE CheckedUnwrap as well? I've seen this bug probably three times now...
Attachment #8771971 - Flags: review?(nfitzgerald) → review+
+cc ejpbruel, jimb: FYI

Comment 5

2 years ago
I don't think the promise code should be calling CheckedUnwrap there at all.

Comment 6

2 years ago
Okay, well, maybe it should be calling CheckedUnwrap, because we're trying to access state that isn't directly visible to JavaScript. I'm still a little worried that we may be traversing wrappers we shouldn't, but it does seem like there's a reason for the call to be there.
(Assignee)

Comment 7

2 years ago
(In reply to Jim Blandy :jimb from comment #6)
> Okay, well, maybe it should be calling CheckedUnwrap, because we're trying
> to access state that isn't directly visible to JavaScript. I'm still a
> little worried that we may be traversing wrappers we shouldn't, but it does
> seem like there's a reason for the call to be there.

There is: the alternative would've been to go through lots of the debugger JS code with a very fine comb and weed out all the places that rely on this in subtle ways. These exist because the DOM Promise implementation also does this unwrapping. I'm all for doing this, but it seems somewhat orthogonal.
(Assignee)

Comment 8

2 years ago
See bug 911216 comment 241 for an explanation.
(Assignee)

Comment 9

2 years ago
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #3)
> Comment on attachment 8771971 [details] [diff] [review]
> Properly handle failure to unwrap cross-compartment wrappers in
> Promise-related DebuggerObject accessors
> 
> Review of attachment 8771971 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Care to MOZ_MUST_USE CheckedUnwrap as well? I've seen this bug probably
> three times now...

That seems a bit out of scope for this. I also don't see how it would've caught this: the result is used, it's just not checked before using it.

Comment 10

2 years ago
(In reply to Till Schneidereit [:till] (ECOOP July 18 - July 22, pto July 23 - July 31) from comment #7)
> There is: the alternative would've been to go through lots of the debugger
> JS code with a very fine comb and weed out all the places that rely on this
> in subtle ways. These exist because the DOM Promise implementation also does
> this unwrapping. I'm all for doing this, but it seems somewhat orthogonal.

Couldn't you just look for uses of isPromise, etc. and then call D.O.p.unwrap first? That seems tedious, but it's not "[going] through lots of the debugger JS code with a very fine comb".
(Assignee)

Comment 11

2 years ago
(In reply to Jim Blandy :jimb from comment #10)
> Couldn't you just look for uses of isPromise, etc. and then call
> D.O.p.unwrap first? That seems tedious, but it's not "[going] through lots
> of the debugger JS code with a very fine comb".

I can't, because in various places, the devtools code invokes accessors on D.O instances in quite indirect ways that don't lend themselves to this kind of inspection. I encountered a few places where, e.g. it iterates over accessors and applies them to an instance. Not only is this hard to inspect, I'd also need to do the unwrapping inside those loops, changing their semantics in ways that I have a hard time reasoning about - in part because I don't understand why all of this is going on.
(Assignee)

Comment 12

2 years ago
Created attachment 8772815 [details] [diff] [review]
Properly handle failure to unwrap cross-compartment wrappers in Promise-related DebuggerObject accessors
Attachment #8771971 - Attachment is obsolete: true
Attachment #8772815 - Flags: review+

Comment 14

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ad0c8d5de33
Properly handle failure to unwrap cross-compartment wrappers in Promise-related DebuggerObject accessors. r=fitzgen
Keywords: checkin-needed

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8ad0c8d5de33
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
status-firefox47: --- → unaffected
status-firefox48: --- → unaffected
status-firefox49: --- → unaffected
You need to log in before you can comment on or make changes to this bug.