1539764 - Expose an helper on protocol's Front class to help retrieve the target of any front

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P3)

Description

[Alexandre Poirot [:ochameau]]

For now, there is no generic way to say to which particular target a given front is related to. The fronts have to bundle in some special locic to figure out to which particular target it relates. Or force the callsites to pass the target.

It will help working on fission if all the fronts were having a getter or a function to retrieve its related target front. It would help sort them in the UI by target, but also help retrieving their related target-scoped front like Inspector, Console or Thread fronts. You often have to call methods on the related targed-scoped front.

All the target-scoped fronts are instanciated from here:
https://searchfox.org/mozilla-central/rev/3d469329a42644b41e66944d8da22d78d8c0f5ec/devtools/shared/fronts/targets/target-mixin.js#213

I imagine that would be the starting point of this work as that's where we start creating fronts from a Target front.

Note that the inspector is still having a special codepath over here:
https://searchfox.org/mozilla-central/rev/3d469329a42644b41e66944d8da22d78d8c0f5ec/devtools/shared/fronts/targets/target-mixin.js#190

Then, we would need some work on Protocol.js's Front class in order to propagate this to all subsequent children of these target-scoped fronts.
https://searchfox.org/mozilla-central/rev/3d469329a42644b41e66944d8da22d78d8c0f5ec/devtools/shared/protocol.js#1265

Also note that the getFront method used in target-mixin.js is defined here:
https://searchfox.org/mozilla-central/rev/3d469329a42644b41e66944d8da22d78d8c0f5ec/devtools/shared/protocol.js#1632
And is also used for global scoped fronts, which aren't bound to any particular target. So we would have to accomodate that. Some fronts will have a related target, some others won't.

Comment 1

[Patrick Brosset <:pbro>]

Not sure I understand all the implications. Something like this would work, right? Can you help understand how this code might be better written?

diff --git a/devtools/shared/fronts/targets/target-mixin.js b/devtools/shared/fronts/targets/target-mixin.js
--- a/devtools/shared/fronts/targets/target-mixin.js
+++ b/devtools/shared/fronts/targets/target-mixin.js
@@ -182,17 +182,17 @@ function TargetMixin(parentClass) {
     // Temporary fix for bug #1493131 - inspector has a different life cycle
     // than most other fronts because it is closely related to the toolbox.
     // TODO: remove once inspector is separated from the toolbox
     async getInspector() {
       // the front might have been destroyed and no longer have an actor ID
       if (this._inspector && this._inspector.actorID) {
         return this._inspector;
       }
-      this._inspector = await getFront(this.client, "inspector", this.targetForm);
+      this._inspector = await getFront(this.client, "inspector", this.targetForm, this);
       this.emit("inspector", this._inspector);
       return this._inspector;
     }
 
     // Run callback on every front of this type that currently exists, and on every
     // instantiation of front type in the future.
     onFront(typeName, callback) {
       const front = this.fronts.get(typeName);
@@ -205,17 +205,17 @@ function TargetMixin(parentClass) {
     // Get a Front for a target-scoped actor.
     // i.e. an actor served by RootActor.listTabs or RootActorActor.getTab requests
     async getFront(typeName) {
       let front = this.fronts.get(typeName);
       // the front might have been destroyed and no longer have an actor ID
       if (front && front.actorID || front && typeof front.then === "function") {
         return front;
       }
-      front = getFront(this.client, typeName, this.targetForm);
+      front = getFront(this.client, typeName, this.targetForm, this);
       this.fronts.set(typeName, front);
       // replace the placeholder with the instance of the front once it has loaded
       front = await front;
       this.emit(typeName, front);
       this.fronts.set(typeName, front);
       return front;
     }
 
diff --git a/devtools/shared/protocol.js b/devtools/shared/protocol.js
--- a/devtools/shared/protocol.js
+++ b/devtools/shared/protocol.js
@@ -1637,28 +1637,29 @@ exports.dumpProtocolSpec = function() {
  * @param DebuggerClient client
  *    The DebuggerClient instance to use.
  * @param string typeName
  *    The type name of the front to instantiate. This is defined in its specifiation.
  * @param json form
  *    If we want to instantiate a global actor's front, this is the root front's form,
  *    otherwise we are instantiating a target-scoped front from the target front's form.
  */
-function getFront(client, typeName, form) {
+function getFront(client, typeName, form, target) {
   const type = types.getType(typeName);
   if (!type) {
     throw new Error(`No spec for front type '${typeName}'.`);
   }
   if (!type.frontClass) {
     lazyLoadFront(typeName);
   }
   // Use intermediate Class variable to please eslint requiring
   // a capital letter for all constructors.
   const Class = type.frontClass;
   const instance = new Class(client);
+  instance.target = target;
   const { formAttributeName } = instance;
   if (!formAttributeName) {
     throw new Error(`Can't find the form attribute name for ${typeName}`);
   }
   // Retrive the actor ID from root or target actor's form
   instance.actorID = form[formAttributeName];
   if (!instance.actorID) {
     throw new Error(`Can't find the actor ID for ${typeName} from root or target` +

Comment 2

[Alexandre Poirot [:ochameau]]

This looks good so far, but it is only the first step.

Here, only the target-scoped fronts (inspector, console, ...) are going to have the target attribute set.
Ideally, from this patch, we would start setting the target attribute on all the nested children of the target-scoped fronts. In the inspector context, it means that the walker and node fronts for example would have the target attribute being set.

This would require digging into Front class in order to propagate it. May be around here.

If the current patch is already helpful for you, we could proceed with it with some unit test and followup on propagating target to all the fronts. This test would be a good place to test this.

Comment 3

[Alexandre Poirot [:ochameau]]

Gabriel, Is this something you are looking into? As well as bug 1568126?

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Alexandre Poirot [:ochameau] from comment #3)

Gabriel, Is this something you are looking into? As well as bug 1568126?

Yes, this is something I will look into.

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1539764 - Add a targetFront attribute to the Front class to retrieve the target front. r=ochameau

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1539764 - Part 2: Add a targetScoped attribute to the Front class for children of target-scoped fronts.

Only children of the target-scoped fronts will have a targetScoped attribute set.
This targetScoped attribute returns the ancestor target-scoped front. As an example,
nodeFront.targetScoped would return the InspectorFront.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63fabd70675c
Add a targetFront attribute to the Front class to retrieve the target front. r=ochameau

Comment 8

[Noemi Erli[:noemi_erli]]

Backed out changeset 63fabd70675c (Bug 1539764) for causing failures in browser_ext_addon_debugging_netmonitor.js CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=258389556&resultStatus=testfailed%2Cbusted%2Cexception&revision=63fabd70675ca8c304c9488d9f5b6e7619d0e200

Failure logs:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=258389556&repo=autoland&lineNumber=26051
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=258381008&repo=autoland&lineNumber=7238
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=258384103&repo=autoland&lineNumber=6256

Backout: https://hg.mozilla.org/integration/autoland/rev/20d09e9aacdd03dd812baf94249c26a0cf62955d

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

The cause of the leaks was because we didn't set this.targetFront = null in destroy() in the Front.

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1539764 - Add a targetFront attribute to the Front class to retrieve the target front.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e0afea2187ed
Add a targetFront attribute to the Front class to retrieve the target front. r=ochameau

Comment 12

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/e0afea2187ed