1522062 - [remote-dbg-next] Add Processes category and allow to debug main process

Status: RESOLVED FIXED

(DevTools :: about:debugging, enhancement, P1)

Description

[Julian Descottes [:jdescottes]]

For parity with the Connect page, we should allow users to debug the main process, at least when debugging remote runtimes.

For this-firefox we shouldbe able to allow users to debug the main process by using toolbox-process-window to spawn a new firefox instance. But if it proves difficult we might skip it.

Note that this is only about the main process, that we should support for parity with the Connect page. Listing all processes and adding relevant features around this will be done in later milestones. (see Bug 1488513)

Comment 1

[Julian Descottes [:jdescottes]]

Spent some time working on this, sharing some insights:

• main process only: we should focus only on the main process in this bug. It is tempting to display right away the content processes, however we don't have any easy way to explain "which" content they are responsible for, they would be more confusing than useful
• remote only: to enable main process debugging on This Firefox, we need to flip the preferences controlled by the "Enable extension debugging" checkbox. If we add this feature to This Firefox, we should change the label to "Enable extension and process debugging".
• behind a flag: maybe this category should be behind a preference
• missing icon: we don't have an icon for Processes. A quick search shows that cog+arrows seem to be the usual choice to represent this although it might conflict with the icon for our Setup page. I will open a follow up to find a dedicated icon, but will use the cog in the meantime

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1522062 - Add Processes category to debug the main process on remote runtimes

Comment 3

[Julian Descottes [:jdescottes]]

In the attached patch, the existing tests pass, but I didn't have time to add new tests for the feature.
Wanted to push a version before going on PTO but I still struggle with two things:

• for consistency, everywhere I assume we are dealing with an array of processes, even though here we know that we only care about the main process. And in a few places, (process-component-data.js, ProcessDetail.js) we rely on the fact that the process target is necessarily a main process (because we get localized strings that correspond to the main process). There's not a lot of adaptation needed if we want to support several processes as valid targets, but it still feels a bit shaky.
• stumbled upon an issue with target front lifecycle (see Bug 1531767)

Anyone is free to pick up the bug (and reuse the existing patch or not) if this can't wait after my PTO :)
Ni? for Daisuke since we briefly talked about it today.

Comment 4

[Daisuke Akatsuka (:daisuke)]

Yup, I'll take this bug :)

Comment 5

[Daisuke Akatsuka (:daisuke)]

(In reply to Julian Descottes [:jdescottes] (PTO til 03/18, contact :ladybenko for Remote Debugging) from comment #3)

In the attached patch, the existing tests pass, but I didn't have time to add new tests for the feature.
Wanted to push a version before going on PTO but I still struggle with two things:

• for consistency, everywhere I assume we are dealing with an array of processes, even though here we know that we only care about the main process. And in a few places, (process-component-data.js, ProcessDetail.js) we rely on the fact that the process target is necessarily a main process (because we get localized strings that correspond to the main process). There's not a lot of adaptation needed if we want to support several processes as valid targets, but it still feels a bit shaky.

I want to try to modify as following approach.

1. Get main process explicitly in requestProcesses() action.
2. Send the process as mainProcess to process-component-data middleware.
3. Localize the main process. Also, localize the label Main Process for the target runtime at same time, and push to details as description.
4. Send a new array which includes main process to next action.
5. In ProcessDetail.js, display the description.

Comment 6

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1522062: Add a test for main process. r?ladybenko

Depends on D21695

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f85aba0b4ab
Add Processes category to debug the main process on remote runtimes r=ladybenko
https://hg.mozilla.org/integration/autoland/rev/94948ac7f019
Add a test for main process. r=ladybenko

Comment 8

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/7f85aba0b4ab
https://hg.mozilla.org/mozilla-central/rev/94948ac7f019