1319049 - Debugger: accessing a passed file object in a worker debugger's console crashes its tab

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[cachius]

Attachment: Testcase.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.98 Safari/537.36

Steps to reproduce:

I pass a File object to a worker via
worker.postMessage({file: file});
then debug it using about:debbuging
and try to inspect the file object in the console of the worker's debugger
through entering e.data.file.


Actual results:

The debugger changes to a broken behaviour, probably because the tab crashes with an error:
###!!! [Parent][MessageChannel] Error: (msgtype=0x2C0085,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv
###!!! [Parent][MessageChannel] Error: (msgtype=0x460103,name=PContent::Msg_AsyncMessage) Channel error: cannot send/recv


Expected results:

The debugger should display a representation of the file.

Comment 1

[Julian Descottes [:jdescottes]]

Just reproduced the issue, thanks for logging!
This doesn't seem specific to about:debugging, as I managed to reproduce the issue using the worker debugger directly.

Detailed STRs:
- go to https://bug1319049.bmoattachments.org/attachment.cgi?id=8812742
- use the file input to pick any file
- open the debugger, to break on the debugger statement after the creation of the worker
- click on "Pass file to worker"
- if you are using the old debugger frontend and worker debugging enabled you can start debugging the worker from here, otherwise go to about:debugging#workers and click Debug for the appropriate worker
- open the worker script in the new debugger window
- resume in the main tab, you should now break in the worker debugger window
- try to evaluate e.data.file in the console

The only error logs are the ones mentioned by cachius in the summary.

ni? for Eddy who might know more about this? Is there a known restriction that prevents inspecting Files in the context of a worker?

Comment 2

[Eddy Bruel [:ejpbruel]]

I produced the following C++ stack trace at the moment of the crash:
https://pastebin.mozilla.org/8930807

The corresponding JS stack (produced with DumpJSStack()) is:
https://pastebin.mozilla.org/8930808

Comment 3

[Eddy Bruel [:ejpbruel]]

The crash is caused by the following line:
let time = Date.prototype.getTime.call(obj.unsafeDereference());

which originates here:
https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/object.js#1172

Let's analyse what this line does:

1. We obtain the value of the property Data.prototype.getTime. Let's refer to this value as `a`. `a` is a
   function object that lives in the debugger's compartment. Let's call this compartment `A`.
2. Next, we call the function obj.unsafeDereference. Let's refer to the result of this call as `b`. `b` is
   a wrapper to an object that lives in the debuggee's compartment. Let's call this compartment `B`.
3. Finally, we call the function object `a`, using `b` as the this value.

Because `b` is a cross-compartment wrapper, as a result of this call, we eventually end up here:
https://dxr.mozilla.org/mozilla-central/source/js/src/proxy/CrossCompartmentWrapper.cpp?q=CrossCompartmentWrapper.cpp&redirect_type=direct#360

Let's see what this code does:

1. First, on line 367, we enter the compartment of the wrapped object of the this value. Since we used `b`
   as the this value, and `b` is a wrapper to an object in `B`, this means we are entering `B`.
2. Then, on line 381, we rewrap every argument to the call in `B`, including the callee. Since our 
   callee is `a`, and `a` lives in `A`, this creates a cross-compartment edge from `B` to `A`.
3. We explicitly disallow any edges from the debuggee's compartment (i.e. `B`) to the debugger's
   compartment (i.e. `A`), so our attempt to rewrap `a` in `B` ends up here:
   https://dxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.cpp?q=RuntimeService.cpp&redirect_type=direct#1019

So, why do we have this restriction? Well, the main reason is that we don't have XPConnect in workers. Since the debuggee is running content code, and the debugger is running chrome code, we need to be very careful about what kind of access we allow between these two. The rule that there should never be any edges from the debuggee to the debugger is very restrictive, but also guaranteed to be secure.

Comment 4

[Eddy Bruel [:ejpbruel]]

As for solutions, we have a couple of options. The most obvious one is 'don't use unsafeDereference'. By design, unsafeDereference bypasses the debugger API, and gives you direct access to objects in the debuggee. Needless to say, doing so is very dangerous, since it breaks assumptions that we typically hold to be true (which is the reason we're crashing here).

If unsafeDereference is so dangerous, why do we use it in the first place? Well, in some cases, the debugger API lacks the functionality we need to implement certain features. unsafeDereference was intended as a stopgap measure for those cases where the debugger API falls short, so that people wouldn't have to wait for the debugger API to be updated before they could implement the feature they need. That said, it was never intended as a long-term solution.

What we should do here is figure out what this line of code is trying to do, and then figure out if it is possible to accomplish the same thing without using unsafeDereference. If there is not, we should look into extending the debugger API so that it does become possible.

Comment 5

[Julian Descottes [:jdescottes]]

Thank you Eddy for the detailed investigation and explanation! Based on our discussion in IRC, I made a patch that should fix the previewers for both RegExp and Date objects.

try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=76610cd52de54deb380e23db0f83dd38b86fcf19

Comment 6

[Julian Descottes [:jdescottes]]

Forgot to add the new test file ... https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa3f131352f068373a2d2b05c630192595803f7d

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 1319049 - fix Date previewer for worker debugging;

Review commit: https://reviewboard.mozilla.org/r/99466/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99466/

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8819804 [details]
Bug 1319049 - fix Date previewer for worker debugging;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99466/diff/1-2/

Comment 9

[Julian Descottes [:jdescottes]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4db06bf4bbb1bb611d615d6b0be142e317e0b7c6

As discussed the regexp previewer has the same issue, but this changeset only fixes the Date previewer. 
Switching to a safer previewer changes how regexps are logged when the RegExp prototype has been modified (cf test devtools/client/webconsole/test/browser_webconsole_output_regexp.js).

I think we should rather update this test and fix the regexp previewer in the same way as the date previewer but I will do it in a separate changeset.

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: Bug 1319049 - fix RegExp previewer for worker debugging;

Review commit: https://reviewboard.mozilla.org/r/99488/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99488/

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 8819804 [details]
Bug 1319049 - fix Date previewer for worker debugging;

https://reviewboard.mozilla.org/r/99466/#review99788

Looks good, thanks for looking at that!

There is many more usages of unsafeDereference, I'm surprise it doesn't crash in the other places?
Especially in createBuiltinStringifier which is also used for RegExp and Date.

Comment 12

[Alexandre Poirot [:ochameau]]

Comment on attachment 8819820 [details]
Bug 1319049 - fix RegExp previewer for worker debugging;

https://reviewboard.mozilla.org/r/99488/#review99790

thanks for removing the useless test file!

Comment 13

[Julian Descottes [:jdescottes]]

(In reply to Alexandre Poirot [:ochameau] from comment #11)
> Comment on attachment 8819804 [details]
> Bug 1319049 - fix Date previewer for worker debugging;
> 
> https://reviewboard.mozilla.org/r/99466/#review99788
> 
> Looks good, thanks for looking at that!
> 
> There is many more usages of unsafeDereference, I'm surprise it doesn't
> crash in the other places?
> Especially in createBuiltinStringifier which is also used for RegExp and
> Date.

Thanks for the review!

Good point about the remaining spots using unsafeDereference. 

From what I tested, object.js does fail when trying to preview Set, Map or Array values because Cu is not defined on the worker thread. This doesn't result in tab crash though, because we don't try to call a method from one compartment on an object from another compartment.

Then we have the stringify() method. It should result in a tab crash, but I can't see when this code is called. I tried removing it and did a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=65b7dae4d13df91fb56349c8545f25334fd4d2f5 . Looks green so far.

Alex, what do you think, is it safe to remove it?

Comment 14

[Alexandre Poirot [:ochameau]]

(In reply to Julian Descottes [:jdescottes] from comment #13)
>
> From what I tested, object.js does fail when trying to preview Set, Map or
> Array values because Cu is not defined on the worker thread. This doesn't
> result in tab crash though, because we don't try to call a method from one
> compartment on an object from another compartment.

Just out of curiosity, do you know why there was multiple compartments involved in the codepath you modified and not in these other codepaths?

> Then we have the stringify() method. It should result in a tab crash, but I
> can't see when this code is called.
> 
> Alex, what do you think, is it safe to remove it?

You would need to modify scratchpad first:
http://searchfox.org/mozilla-central/search?q=getDisplayString&path=devtools%2F

Looks like this isn't covered by test :(

Also, there is an xpcshell test covering that actor request:
http://searchfox.org/mozilla-central/source/devtools/server/tests/unit/test_objectgrips-12.js#147

But it explains why it would never crash in workers!

Comment 15

[Julian Descottes [:jdescottes]]

(In reply to Alexandre Poirot [:ochameau] from comment #14)
> (In reply to Julian Descottes [:jdescottes] from comment #13)
> >
> > From what I tested, object.js does fail when trying to preview Set, Map or
> > Array values because Cu is not defined on the worker thread. This doesn't
> > result in tab crash though, because we don't try to call a method from one
> > compartment on an object from another compartment.
> 
> Just out of curiosity, do you know why there was multiple compartments
> involved in the codepath you modified and not in these other codepaths?
> 

I think it's because we throw an error earlier due to the missing Cu, before the cross compartment check can be executed.

For example if we modify the enumWeakSetEntries and call WeakSet.prototype.add.call(raw, 1); right before doing Cu.waiveXrays, then the tab will crash when inspecting WeakSet entries in a worker debugger.

> > Then we have the stringify() method. It should result in a tab crash, but I
> > can't see when this code is called.
> > 
> > Alex, what do you think, is it safe to remove it?
> 
> You would need to modify scratchpad first:
> http://searchfox.org/mozilla-central/
> search?q=getDisplayString&path=devtools%2F
> 
> Looks like this isn't covered by test :(
> 
> Also, there is an xpcshell test covering that actor request:
> http://searchfox.org/mozilla-central/source/devtools/server/tests/unit/
> test_objectgrips-12.js#147
> 
> But it explains why it would never crash in workers!

Ah good catch :/ I'll dig a bit more and see if this can be replaced by something else. Seems weird to have so much code used only by scratchpad.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f20395d42751
fix Date previewer for worker debugging;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/0fbc6cab7b7a
fix RegExp previewer for worker debugging;r=ochameau

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f20395d42751
https://hg.mozilla.org/mozilla-central/rev/0fbc6cab7b7a

Comment 18

[Jim Blandy :jimb]

Re: comment 3, I'm not sure that when we do something like `f.call(o)`, we have to enter `o`'s compartment and rewrap `f` for use there. Wouldn't we have to immediately turn around and re-enter `f`'s compartment and undo all that wrapping?

So I still don't understand why we're trying to create an edge from the debuggee to the debugger.

Comment 19

[Jim Blandy :jimb]

Jason Orendorff says:

Jason Orendorff, [21.12.16 11:44]
Many native functions f will enter O (because they have to unwrap o to do their job, and the fact that they're being called cross-compartment is one of those weird things, "only happens adversarially")

Jason Orendorff, [21.12.16 11:45]
yeah, Date#getTime is one of those

Jason Orendorff, [21.12.16 11:45]
CallNonGenericMethod does this.

Jason Orendorff, [21.12.16 11:46]
"non-generic" meaning, in order to do its job this method *must* unwrap this

Comment 20

[Jim Blandy :jimb]

Attachment: C++ stack from comment 2, saved from pastebin

We shouldn't be citing pastebin from bugzilla.

Comment 21

[Jim Blandy :jimb]

Attachment: JS stack from comment #2, saved from pastebin

Comment 22

[Jim Blandy :jimb]

I filed bug 1325195 to track the wrapper issue. We're going to have more of these problems.