1572409 - Make the debugger use the TargetList API and support target switching

Status: RESOLVED FIXED

(DevTools :: Debugger, task, P1)

Description

[Yulia Startsev [:yulia]]

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.

Comment 1

[Jason Laster [:jlast]]

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

Comment 2

[Jason Laster [:jlast]]

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;
 }

2. 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:

Comment 3

[Alexandre Poirot [:ochameau]]

(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.

2. 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.

Comment 4

[Jason Laster [:jlast]]

Alex, is this done?

Comment 5

[Alexandre Poirot [:ochameau]]

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.

Comment 6

[Alexandre Poirot [:ochameau]]

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.

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1572409 - Make the debugger use the TargetList.

Comment 8

[Alexandre Poirot [:ochameau]]

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.

Comment 9

[Julian Descottes [:jdescottes]]

Will give this a try this week.

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: Bug 1572409 - TargetList in debugger: use TargetList to retrieve the initial target

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: Bug 1572409 - TargetList in debugger: rename tabTarget to currentTarget

Depends on D55668

Comment 12

[Julian Descottes [:jdescottes]]

Attachment: Bug 1572409 - TargetList in debugger: Split global and target specific setup in commands and events modules

Depends on D55669

Comment 13

[Julian Descottes [:jdescottes]]

Attachment: Bug 1572409 - TargetList in debugger: Use watchTargets to support target switching

Depends on D55670

Comment 14

[Julian Descottes [:jdescottes]]

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

Comment 15

[Chris Peterson [:cpeterson]]

Tracking for Fission Nightly (M6)

Comment 16

[Julian Descottes [:jdescottes]]

Attachment: Bug 1572409 - Add a test for Debugger target-switching support

Depends on D55671

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/0e339f6175af
https://hg.mozilla.org/mozilla-central/rev/b6cd01327d3f
https://hg.mozilla.org/mozilla-central/rev/69e7c56dcc30
https://hg.mozilla.org/mozilla-central/rev/ec5d2d6f96bb
https://hg.mozilla.org/mozilla-central/rev/79e6b0610d0d