Closed Bug 1325195 Opened 7 years ago Closed 5 years ago

Worker debugging wrapper rules may not be workable

Categories

(DevTools :: Debugger, defect, P3)

defect

Tracking

(firefox69 fixed)

RESOLVED FIXED
Firefox 69
Tracking Status
firefox69 --- fixed

People

(Reporter: jimb, Assigned: bhackett1024)

References

(Blocks 1 open bug)

Details

(Whiteboard: [debugger-mvp])

Attachments

(3 files)

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.
(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.
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!
Flags: needinfo?(jimb)
Product: Firefox → DevTools

Ping jim, is this still relevant?

Blocks: dbg-api

Brian, is this relevant?

Flags: needinfo?(bhackett1024)

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.

Flags: needinfo?(bhackett1024)
Priority: -- → P3
Attached patch patchSplinter Review

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.

Assignee: nobody → bhackett1024
Status: NEW → ASSIGNED
Whiteboard: [debugger-mvp]
Pushed by bhackett@mozilla.com:
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.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 69
Regressions: 1558504
Blocks: 1547320
Flags: needinfo?(jimb)
See Also: → 1833303
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: