Closed Bug 1572409 Opened 2 years ago Closed 1 year ago

Make the debugger use the TargetList API and support target switching

Categories

(DevTools :: Debugger, task, P1)

task

Tracking

(Fission Milestone:M6, firefox73 fixed)

RESOLVED FIXED
Firefox 73
Fission Milestone M6
Tracking Status
firefox73 --- fixed

People

(Reporter: ystartsev, Assigned: jdescottes)

References

(Blocks 3 open bugs)

Details

(Whiteboard: dt-fission-m2-mvp)

Attachments

(5 files, 1 obsolete file)

With the work we are currently doing to support target switching (https://bugzilla.mozilla.org/show_bug.cgi?id=1565263).

With fission, we will not have a consistent top level target, but the debugger currently expects this: https://searchfox.org/mozilla-central/rev/9ae20497229225cb3fa729a787791f424ff8087b/devtools/client/debugger/src/client/firefox/commands.js#60-61

TabTarget should also be renamed to target, to match the rest of the codebase.

Blocks: dbg-fission
Depends on: 1565263
Whiteboard: dt-fission

Sounds good. I think the debugger should be fine either way.

Priority: -- → P3
Priority: P3 → P2
Blocks: dbg-protocol-js
No longer blocks: dbg-fission

Just reviewed Bug 1565263, and I think we can implement this functionality in two different ways:

  1. by notifying the debugger that the target has been switched. Which is conceptually similar to telling the debugger to go to a source location.
diff --git a/devtools/client/debugger/panel.js b/devtools/client/debugger/panel.js
index 983c34b05064..62f153b52e68 100644
--- a/devtools/client/debugger/panel.js
+++ b/devtools/client/debugger/panel.js
@@ -68,6 +68,10 @@ DebuggerPanel.prototype = {
     return this._store.getState();
   },

+  switchTarget: function(newTarget) {
+   // we might need to clear the store in a similar manner to navigate
+   dispatch({ action: "SWITCH-TARGET", target: newTarget })
+   // we might need to re-fetch threads based on the new target
+   dispatch(updateThreads())
+    this.action.switchTarget(newTarget);
+  },
+
   getToolboxStore: function() {
     return this.toolbox.store;
   },
diff --git a/devtools/client/debugger/src/actions/toolbox.js b/devtools/client/debugger/src/actions/toolbox.js
index 6b31c0f428a0..397ac91c412d 100644
--- a/devtools/client/debugger/src/actions/toolbox.js
+++ b/devtools/client/debugger/src/actions/toolbox.js
@@ -40,3 +40,9 @@ export function unHighlightDomElement(grip: Grip) {
     return panel.unHighlightDomElement(grip);
   };
 }
+
+export function switchTarget(newTarget) {
+  return ({ client }: ThunkArgs) => {
+    return client.switchTarget(newTarget);
+  };
+}
diff --git a/devtools/client/debugger/src/client/firefox/commands.js b/devtools/client/debugger/src/client/firefox/commands.js
index 9abca08a9a85..e8b308d0985e 100644
--- a/devtools/client/debugger/src/client/firefox/commands.js
+++ b/devtools/client/debugger/src/client/firefox/commands.js
@@ -65,6 +65,11 @@ function setupCommands(dependencies: Dependencies) {
   breakpoints = {};
 }

+function switchTarget(newTarget) {
+  currentTarget = newTarget;
+  currentThreadFront = newTarget.threadFront;
+}
+
 function hasWasmSupport() {
   return supportsWasm;
 }
  1. by emitting an event on the appropriate client/front instance. Either the DebuggerClient or RootFront should work.
diff --git a/devtools/client/debugger/src/client/firefox/commands.js b/devtools/client/debugger/src/client/firefox/commands.js
index 9abca08a9a85..f26233a1c57b 100644
--- a/devtools/client/debugger/src/client/firefox/commands.js
+++ b/devtools/client/debugger/src/client/firefox/commands.js
@@ -63,6 +63,11 @@ function setupCommands(dependencies: Dependencies) {
   targets = {};
   sourceActors = {};
   breakpoints = {};
+
+  debuggerClient.on("switch-target", async newTarget => {
+    currentTarget = newTarget;
+    currentThreadFront = await newTarget.getFront("thread");
+  });
 }

 function hasWasmSupport() {
@@ -356,9 +361,7 @@ function setEventListenerBreakpoints(ids: string[]) {
 }

 // eslint-disable-next-line
-async function getEventListenerBreakpointTypes(): Promise<
-  EventListenerCategoryList
-> {
+async function getEventListenerBreakpointTypes(): Promise<EventListenerCategoryList> {
   let categories;
   try {
     categories = await currentThreadFront.getAvailableEventBreakpoints();
diff --git a/devtools/client/framework/toolbox.js b/devtools/client/framework/toolbox.js
index a402630680d4..31989b91c054 100644
--- a/devtools/client/framework/toolbox.js
+++ b/devtools/client/framework/toolbox.js
@@ -464,6 +464,13 @@ Toolbox.prototype = {
     this.doc.querySelector("window").classList.toggle("dragging");
   },

+ async switchToTarget(newTarget) {
+    this._target = newTarget;
+
+    // Notify panels that the target has been switched
+    this.target.client.emit("switch-target", newTarget);
+ },
+
   /**
    * Get/alter the target of a Toolbox so we're debugging something different.
    * See Target.jsm for more details.

If we rely on the toolbox event, the design will be a bit different:

No longer blocks: dbg-protocol-js
Summary: Refactor tabTarget and thread in the debugger to not be global → The debugger should support target switching
Flags: needinfo?(poirot.alex)

(In reply to Jason Laster [:jlast] from comment #2)

Just reviewed Bug 1565263, and I think we can implement this functionality in two different ways:

  1. by notifying the debugger that the target has been switched. Which is conceptually similar to telling the debugger to go to a source location.

I'm still waiting for more feedback, but we will most likely proceed with this way of doing target switching.
We may revisit that later if there is opportunities to clean up things with Target descriptors and the Resource API.

  1. by emitting an event on the appropriate client/front instance. Either the DebuggerClient or RootFront should work.

I don't see us do something like that, at least in the first iteration. We may later have some target-switching event, but that would be on the target, or the Descriptor (more likely, but for now, we only have that API for Processes and Addons), or the resource API to come.

Flags: needinfo?(poirot.alex)

Alex, is this done?

Flags: needinfo?(poirot.alex)

No. Bug 1565263 is still in progress.
But the way to support target-switching should be stable:

  • Toolbox will call panel's switchToTarget method, passing the new target object
  • when this get called, you should unregister the previous target
  • and acknowledge the new. Fetch sources, set RDP event listeners...

You may start crafting a patch based on the attached patches and look at WebConsole integration.
I'm currently revisiting my approach to DebuggerClient destruction, which is the source of complexity here
My hope is that I will finally be able to proceed.

Flags: needinfo?(poirot.alex)

Bug 1471754 and the TargetList will kill two birds with one stone.
Using TargetList.watchTarget will allow to observe additional targets as well as changes made to the top level target.

This WIP patch https://phabricator.services.mozilla.com/D48861 demonstrate a beginning of usage of this new API for the debugger.
More work will be needed here to properly support it.

Depends on: 1471754
Summary: The debugger should support target switching → Make the debugger use the TargetList API and support target switching

https://phabricator.services.mozilla.com/D48861 demonstrate a very first approach on how to possibly use the TargetList in the Debugger.
But the integration is imperfect for various reasons:

  • First, I hacked something gross to hand over the TargetList down to events and targets module. We should probably replace currentTarget by TargetList. You can derivate the current top level target via targetList.targetFront. We should avoid storing the currentTarget anywhere as it can change when navigating to another process, instead currentTarget should always be a getter mapped to targetList.targetFront.
    Should I open a bug to focus on just that?
  • Then, I updated setupEvents to use TargetList.watchTargets and updateProcessTargets to use TargetList.getAllTargets. It works, but we do not benefit from TargetList improvements: the fact that it notifies about which Target is available and which Target has been destroyed. In setupEvents we call actions.updateThreads sources, anytime a target is created or destroyed. This will ultimately call updateProcessTargets, which will fetch all the targets. We fetch all the threads and them compare the previous and the new list of threads. But this is all unecessary as the two callbacks passed to TargetList.watchTargets will highlight which thread has been created and which thread has been destroyed.
    Should I also open a bug about just that and use this one as a meta? (This would depends on the previous task)

Even if I wrote this WIP patch, I'm not planning to work on this soon. So feel free to pick up that work.

Priority: P2 → P3
Whiteboard: dt-fission → dt-fission-reserve

Will give this a try this week.

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

Attached a few patches. They are focused on the top level target, not on the worker targets.

Attachment #9113221 - Attachment description: Bug 1572409 - TargetList in debugger: retrieve initial target with watchTargets → Bug 1572409 - TargetList in debugger: Split global and target specific setup in commands and events modules
Attachment #9113222 - Attachment description: Bug 1572409 - TargetList in debugger: Support target switching (WIP) → Bug 1572410 - TargetList in debugger: Use watchTargets to support target switching
Attachment #9113222 - Attachment description: Bug 1572410 - TargetList in debugger: Use watchTargets to support target switching → Bug 1572409 - TargetList in debugger: Use watchTargets to support target switching
Depends on: 1602734
Whiteboard: dt-fission-reserve → dt-fission-m2-mvp

Tracking for Fission Nightly (M6)

Fission Milestone: --- → M6
Blocks: 1605743
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0e339f6175af
TargetList in debugger: use TargetList to retrieve the initial target r=jlast
https://hg.mozilla.org/integration/autoland/rev/b6cd01327d3f
TargetList in debugger: rename tabTarget to currentTarget r=jlast
https://hg.mozilla.org/integration/autoland/rev/69e7c56dcc30
TargetList in debugger: Split global and target specific setup in commands and events modules r=jlast
https://hg.mozilla.org/integration/autoland/rev/ec5d2d6f96bb
TargetList in debugger: Use watchTargets to support target switching r=ochameau,jlast
https://hg.mozilla.org/integration/autoland/rev/79e6b0610d0d
Add a test for Debugger target-switching support r=jlast,ochameau
Attachment #9104909 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.