1325195 - Worker debugging wrapper rules may not be workable

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Jim Blandy :jimb]

Our rules for cross-compartment wrappers in workers need to be revisited, because they make it far too easy to crash the content process.

In bug 1319049 comment 3, ejpbruel explained how we fail the assertion here:

    MOZ_CRASH("There should be no edges from the debuggee to the debugger.");

http://searchfox.org/mozilla-central/rev/4f5f9f3222f35fa4635ea96a0c963c130854ef19/dom/workers/RuntimeService.cpp#1019

Originally, there were no wrappers in workers at all, as there was only ever one global and one compartment in each worker. When we implemented worker debugging, the devtools server needed to have compartments for its own modules. Introducing other compartments made cross-compartment edges possible, so we had to choose a cross-comportment wrapper policy.

We assumed that the server would need to refer to objects in the debuggee compartment, if only to register the initial content global as a debuggee; we assumed that all other interaction would go through Debugger.Object objects, which are designed for the purpose. So we made server->content wrappers permitted, but left them completely opaque, to encourage the use of Debugger.Object instead.

We couldn't think of any reason that content code would ever legitimately hold a reference to an object in the devtools server compartment, so rather than ever create such a wrapper, we added the assertion above.

However, calling many native functions from compartment A on any `this` value in compartment B entails entering B, unwrapping `this`, and rewrapping the callee and arguments for B. Any native function that uses CallNonGenericMethod will behave this way.

This means that applying any of a large group of native functions from the server compartment to a `this` value in the content compartment will try to create a content->server edge and crash. This violates JavaScript developers' expectation of getting exceptions when they violate rules, instead of crashes.

Comment 1

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #0)
> This means that applying any of a large group of native functions from the
> server compartment to a `this` value in the content compartment will try to
> create a content->server edge and crash.

To be clear: we do this application of server native functions to content `this` values to produce better visible representations of various common classes of objects: RegExp and Date, for example. Debugger.Object doesn't provide a rich enough API to get the interesting details of such objects, so we use unsafeDereference on them.

Comment 2

[Yulia Startsev [:yulia]]

Sounds like quite an edge case -- Do you have any information about how common it is, or how it affects users?

We should fix this in any case, trying to figure out how high it is in terms of priority!

Comment 3

[Jason Laster [:jlast]]

Ping jim, is this still relevant?

Comment 4

[Jason Laster [:jlast]]

Brian, is this relevant?

Comment 5

[Brian Hackett [Laid off!]]

In bug 1522244 I replaced this crash with a JS exception, but I think it would be good to remove this check entirely because it interferes with attempts to access the debuggee object outside the Debugger.Object methods. This prevents server inspection logic written for the main thread from working in workers, and bug 1522244 and bug 1533202 had to add workarounds for these problems. Those workarounds have their own problems too, though, see bug 1547320.

Comment 6

[Brian Hackett [Laid off!]]

Attachment: patch

This patch changes the worker wrapping rules to allow the same logic used on the main thread to call debugger natives on debuggee values to work. When wrapping a debugger value into the debuggee, an opaque CCW is used. The only reason we do this when calling a debugger native on a debuggee value is to wrap the callee function into the debuggee before calling the native on it. This callee function isn't used at all so things work fine if it is an opaque CCW, and keeping these wrappers opaque seems like the right thing to do (the debuggee shouldn't have unfettered access to debugger objects it gets its hands on).

All the special worker logic to reimplement this stuff is removed as well, though we have to make sure to use the Debugger.Object API when accessing produced debuggee values (like iterators) as they are still opaque CCWs.

Comment 7

[Brian Hackett [Laid off!]]

Attachment: Bug 1325195 Part 1 - Create opaque CCWs when wrapping worker debugger objects into other compartments, r=asuth.

Comment 8

[Brian Hackett [Laid off!]]

Attachment: Bug 1325195 Part 2 - Remove worker specific code in object previews, r=loganfsmyth.

Depends on D32872

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/88266effc976
Part 1 - Create opaque CCWs when wrapping worker debugger objects into other compartments, r=asuth.
https://hg.mozilla.org/integration/mozilla-inbound/rev/86f6fc53e2fd
Part 2 - Remove worker specific code in object previews, r=loganfsmyth.

Comment 10

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/88266effc976
https://hg.mozilla.org/mozilla-central/rev/86f6fc53e2fd