Closed Bug 1522062 Opened 7 months ago Closed 5 months ago

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

Categories

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

enhancement

Tracking

(firefox68 fixed)

RESOLVED FIXED
Firefox 68
Tracking Status
firefox68 --- fixed

People

(Reporter: jdescottes, Assigned: daisuke)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files)

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)

Priority: -- → P3
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Priority: P3 → P1

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

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.

Assignee: jdescottes → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(dakatsuka)
Priority: P1 → P2

Yup, I'll take this bug :)

Assignee: nobody → dakatsuka
Status: NEW → ASSIGNED
Flags: needinfo?(dakatsuka)
Priority: P2 → P1

(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.
See Also: → 1534180
Pushed by dakatsuka@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
Blocks: 1488513
You need to log in before you can comment on or make changes to this bug.