1287335 - Debugger makeDebuggeeValue can crash Firefox

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Oriol Brufau [:Oriol]]

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

Comment 1

[Oriol Brufau [:Oriol]]

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

Calling any promise getter manually produces the crash too.

Comment 2

[Till Schneidereit [:till]]

Attachment: 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.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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...

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

+cc ejpbruel, jimb: FYI

Comment 5

[Jim Blandy :jimb]

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

Comment 6

[Jim Blandy :jimb]

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.

Comment 7

[Till Schneidereit [:till]]

(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.

Comment 8

[Till Schneidereit [:till]]

See bug 911216 comment 241 for an explanation.

Comment 9

[Till Schneidereit [:till]]

(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

[Jim Blandy :jimb]

(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".

Comment 11

[Till Schneidereit [:till]]

(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.

Comment 12

[Till Schneidereit [:till]]

Attachment: Properly handle failure to unwrap cross-compartment wrappers in Promise-related DebuggerObject accessors

Comment 13

[Till Schneidereit [:till]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dca9c44aaa7

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ad0c8d5de33
Properly handle failure to unwrap cross-compartment wrappers in Promise-related DebuggerObject accessors. r=fitzgen

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8ad0c8d5de33