1686344 - Support `--jsdebugger` and `--wait-for-jsdebugger` in background task mode

Status: RESOLVED FIXED

(Toolkit :: Application Update, enhancement, P3)

Description

[Nick Alexander :nalexander [he/him]]

This ticket tracks making firefox --backgroundtask ... --jsdebugger --wait-for-jsdebugger do the sensible thing, which is to launch the devtools (in a separate process) and to set a breakpoint in the background task JSM.

This is all follows the example of xpcshell and is more-or-less straightforward; the only tricky aspect is to listen for devtools-thread-{instantiated,ready} (-ready is post-Bug 1682780) early enough to arrange the breakpoint before the background task manager loads the task JSM.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1686344 - Support `--backgroundtask --jsdebugger` (and `--wait-for-jsdebugger`). r?Mossop!,jdescottes!

Background task mode is roughly equivalent to xpcshell, but inside
the regular browser startup flow. There is no browser window (no
Window at all) and there should be no content processes. It's
sufficient to treat it like xpcshell, with its own stripped-down
actor and a few tweaks to the integration points.

The structural changes in this commit keep --backgroundtask mode
slim in the regular case when the Devtools are not requested. This
is reflected in the small changes needed to the
browser_xpcom_graph_wait.js test: loading the Devtools
unconditionally causes a huge amount of code to be loaded. In order
to load the Devtools framework conditionally, we check for
Devtools-specific command line flags and delegate to Devtools when
appropriate. In order to check the command line flags, we turn the
BackgroundTasksManager into an XPCOM service, which allows it to be
instantiated by XPCOM in order to handle the command line.

One final note: this leaves two XPCOM components, "backgroundtasks"
and "backgroundtasksmanager". Why not combine them? This is
technically possible but not attractive: we really do want a natural
place for native/C++ code ("backgroundtasks") and JavaScript code
("backgroundtasksmanager").

Comment 2

[Nick Alexander :nalexander [he/him]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment https://bugzilla.mozilla.org/show_bug.cgi?id=1739115#c0)

We don't have any automated way to check that xpcshell test can be debugged, and as a result, we often break them.
Testing it would be hard, but we could at least check for each release that it's not broken.

Nicolas: thanks for taking steps to keep xpcshell debugging working each release: much appreciated.

I'd like to understand if we can do something automated, however. This bug will make firefox --backgroundtask ... --jsdebugger --wait-for-jsdebugger wait for the devtools to be connected before continuing. The advantage of the background task is that it's easy to launch from more types of test, including browser tests, where-as it's Not Easy to launch a "sub xpcshell test" from a browser test. The actual mechanism will be very close to the mechanism for xpcshell -- the only real differences will be in the way that the initial breakpoints is set -- so in theory testing this mechanism will both ensure that background tasks can be debugged but also that xpcshell tests can be debugged.

Can you suggest how to test this mechanism using the existing devtools testing helpers? IIUC, the existing tests such as https://searchfox.org/mozilla-central/source/devtools/client/framework/browser-toolbox/test/browser_browser_toolbox_debugger.js:

1. launch a separate/remote browser console process
2. connect that remote process to the process running the test harness
3. drive the remote console process to perform the test

What we want to do is something like the following:

1. launch a background task waiting to be debugged (firefox --backgroundtask <TEST TASK> --jsdebugger -- wait-for-jsdebugger)
2. connect the test harness process to the background task process
3. drive the test harness process to perform the test

But it's not clear that this can be done: perhaps using the devtools within the test harness process is not possible?

Reading https://searchfox.org/mozilla-central/source/devtools/client/framework/browser-toolbox/README.md, it sounds like what might be better is to have a test-specific background task that itself uses the various testing helpers to launch the browser toolbox process to connect to itself... but I don't see how to do the equivalent of the "toolbox task" when the thread of execution in the task itself should be blocked waiting for the debugger to connect.

Can you suggest a path forward? Perhaps a brief video call with one of the devtools experts would be in order, since I don't understand what's possible in that space and y'all probably don't have a great understanding of what's possible with background tasks.

Comment 3

[Julian Descottes [:jdescottes]]

We discussed this briefly on Elements last week so I will just paste my comments on this topic.

If we can start a Background Task which will spawn a BrowserToolbox from a browser mochitest, then I feel like we can reuse most of the initBrowserToolboxTask helper. If the preference devtools.browsertoolbox.enable-test-server is set, the BrowserToolbox automatically starts a test server which listens for connections on port 6001. So I imagine the steps will be

• test starts the Background Task with the --jsdebugger argument
• Background Task spawns the BrowserToolbox
• BrowserToolbox starts a test server (depends on pref devtools.browsertoolbox.enable-test-server, has to be true for the Background Task profile)
• BrowserToolbox automatically breaks on the first line
• test connects to the BrowserToolbox (we might have to poll on port 6001 until we get something here?)
• test then interacts with the BrowserToolbox to set breakpoints, resume, etc...

If we can get help/pointers to start the background task from the test, I hope the rest should be straightforward.

Comment 4

[Nick Alexander :nalexander [he/him]]

Attachment: fold! Bug 1686344 - Add test for `--backgroundtask --jsdebugger --wait-for-jsdebugger`. r?#devtools-reviewers

Comment 5

[Nick Alexander :nalexander [he/him]]

No need for more details from nchevobbe@ right now.

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1686344 - Pre: Swallow exception when destroying devtools server targeting `--backgroundtask`. r?#devtools-reviewers

The exception is like:

Handler function DebuggerTransport.prototype.onInputStreamReady threw an exception: Error: Unsupported resource type 'css-change' for contentProcessTarget
Stack: getResourceTypeEntry@resource://devtools/server/actors/resources/index.js:200:11
unwatchResources@resource://devtools/server/actors/resources/index.js:287:46
_unwatchTargetResources@resource://devtools/server/actors/targets/target-actor-mixin.js:154:24
removeSessionDataEntry@resource://devtools/server/actors/targets/target-actor-mixin.js:125:21
unwatchResources@resource://devtools/server/actors/watcher.js:637:21
destroy@resource://devtools/server/actors/watcher.js:172:10
destroy@resource://devtools/shared/protocol/Pool.js:210:17
destroy@resource://devtools/shared/protocol/Actor.js:76:11
destroy@resource://devtools/server/actors/descriptors/process.js:210:29
destroy@resource://devtools/shared/protocol/Pool.js:210:17
destroy@resource://devtools/server/actors/root.js:187:40
destroy@resource://devtools/shared/protocol/Pool.js:210:17
onTransportClosed/<@resource://devtools/server/devtools-server-connection.js:493:34
onTransportClosed@resource://devtools/server/devtools-server-connection.js:493:19
close@resource://devtools/shared/transport/transport.js:186:18
DebuggerTransport.prototype.onInputStreamReady<@resource://devtools/shared/transport/transport.js:339:14
exports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.js:103:22
Line: 200, column: 11
console.error: (new Error("Unsupported resource type 'css-change' for contentProcessTarget", "resource://devtools/server/actors/resources/index.js", 200))

Comment 7

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1686344 - Pre: Prefix child process output lines with PID>. r?#devtools-reviewers

We may get multiple lines or incomplete lines from the pipe, so we
need to split the data and keep the leftover. This simply makes
debugging a little more pleasant.

Comment 8

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1686344 - Add test for `--backgroundtask --jsdebugger --wait-for-jsdebugger`. r?#devtools-reviewers

This version is as simple as I can make it. It simply expects the JS
debugger to stop on the breakpoint added automatically by the
backgroundtask debugger command line processing (using
setBreakpointOnLoad) and disconnects, expecting the task to continue
execution and exit with exit code 0.

In the future, we'd like to interact with the task environment, for example to:

1. stop on the automatic breakpoint
2. continue
3. stop on a debugger;
4. set the task's exit code from a failure code to exit code 0
5. continue
6. verifies the tasks's exit code is 0.

Sadly my attempts to do this fail intermittently in automation.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a75f93181dab
Pre: Prefix child process output lines with PID>. r=devtools-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/82ea1a1e4f71
Support `--backgroundtask --jsdebugger` (and `--wait-for-jsdebugger`). r=mossop,jdescottes
https://hg.mozilla.org/integration/autoland/rev/ce693e8e4210
Add test for `--backgroundtask --jsdebugger --wait-for-jsdebugger`.  r=devtools-reviewers,jdescottes

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de23ad1de3ae
Fix es lint failure. a=lint-fix CLOSED TREE

Comment 11

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/a75f93181dab
https://hg.mozilla.org/mozilla-central/rev/82ea1a1e4f71
https://hg.mozilla.org/mozilla-central/rev/ce693e8e4210
https://hg.mozilla.org/mozilla-central/rev/de23ad1de3ae