870081 - JS debugger: RootActor implementations should be unified

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Jim Blandy :jimb]

Attachment: Share a common RootActor implementation amongst browser, Fennec, B2G, and xpcshell tests.

At the moment, we have separate root actor implementations for xpcshell tests, the browser, Fennec, and B2G. This makes it hard to test changes in friendlier environments (xpcshell) before tackling the more recalcitrant (B2G), and makes extending the root actor protocol harder.

All those contexts should share a single root actor implementation that takes parameters to customize it for a particular context.

Attached is a first cut; tested only on xpcshell, but should get the idea across.

Comment 1

[Jim Blandy :jimb]

Attachment: Share a common RootActor implementation amongst browser, Fennec, B2G, and xpcshell tests.

This version has a BrowserTabList that seems to actually work; mochitests on the way. (There are some implementation notes above the definition of the BrowserTabList constructor. I've learned a lot, and fixed some bugs in the prior implementation.)

We need equivalents to this patch's BrowserTabList for B2G and Android: GaiaTabList and MobileTabList? The spec they must implement is described in the comments above the new RootActor constructor added in this patch. Once I've got the mochitest done, toolkit/devtools/debugger/tests/mochitest/browser_BrowserTabList.js, then that can serve as further documentation.

Comment 2

[Dave Camp (:dcamp)]

Panos, are you the best person here to make sure this works on b2g and fennec?

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v2

(In reply to Dave Camp (:dcamp) from comment #2)
> Panos, are you the best person here to make sure this works on b2g and
> fennec?

Sure, just rebased it along with its dependencies and will now build and test.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v2.1

Err, that was actually the wrong patch. This is the right one.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 753177 [details] [diff] [review]
Patch v2.1

Review of attachment 753177 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/root.js
@@ +251,5 @@
> +    return reply;
> +  },
> +
> +  onTabListChanged: function () {
> +    this.conn.send({ actor:"root", type:"tabListChanged" });

I think you mean { from:"root", type:"tabListChanged" } here.
And "tabListChanged" needs to be added to UnsolicitedNotifications in dbg-client.jsm.

Comment 6

[Jim Blandy :jimb]

(In reply to Panos Astithas [:past] from comment #5)
> Comment on attachment 753177 [details] [diff] [review]
> Patch v2.1
> 
> Review of attachment 753177 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/server/actors/root.js
> @@ +251,5 @@
> > +    return reply;
> > +  },
> > +
> > +  onTabListChanged: function () {
> > +    this.conn.send({ actor:"root", type:"tabListChanged" });
> 
> I think you mean { from:"root", type:"tabListChanged" } here.
> And "tabListChanged" needs to be added to UnsolicitedNotifications in
> dbg-client.jsm.

Yes. I've fixed both of those problems in my local copy; I'll update the patch here.

Comment 7

[Jim Blandy :jimb]

Oh, I see, you're updating it now. Yes, sorry, fine.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Fennec patch

This makes Fennec work again.

Comment 9

[Alexandre Poirot [:ochameau]]

Oh! I immediatly wanted such abtraction when I started working on b2g actors.
I was even wondering if we should have an even more abstracted root actor.
Here, the base root actor is still Tab oriented, but when working with b2g,
the rootactor will most likely handle apps rather than tabs.

You made the most important thing: share most of root actor implementation in order to make platform specific actor way easier to implement. Now I'm suggesting to have the icing on the cake and improve the semantic when using the RootActor on b2g:
 * rename onListTabs to onListActors,
 * same thing for all other names involving Tab(s)
But I can easily imagine that it would break various code and may not worth it...

In anycase, this improvement will make it super easy to make a live list of existing tabs after bug 875104 gets landed!

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: B2G and Fennec changes v2

This works on both b2g desktop and my Android device. Testing b2g device will take a little longer. Jim, does this look like what you had imagined?

Comment 11

[Jim Blandy :jimb]

(In reply to Alexandre Poirot (:ochameau) from comment #9)
> Oh! I immediatly wanted such abtraction when I started working on b2g actors.
> I was even wondering if we should have an even more abstracted root actor.
> Here, the base root actor is still Tab oriented, but when working with b2g,
> the rootactor will most likely handle apps rather than tabs.

Let's talk about this more on-line, but some thoughts for the moment:

I think it is probably too late to rename the 'listTabs' request; we have just enough people doing cross-version debugging that we should keep names stable.

However, it is always okay to add more properties to an existing reply; old clients will ignore them. So, for example, we could add an 'appList' parameter to RootActor; if provided, the root actor would reply to listTabs requests like this:

{
  "from": "child1.conn0:root",
  "selected": 0,
  "tabs": [],
  "apps": [
    { url: "app://costcontrol.gaiamobile.org/widget.html" },
    { url: "app://homescreen.gaiamobile.org/index.html#root", active: true },
    { url: "app://settings.gaiamobile.org/index.html#root", active: true },
    { url: "app://sms.gaiamobile.org/index.html" },
    { url: "app://keyboard.gaiamobile.org/index.html" }
  ]
  "chromeDebugger": "puppet:conn0.chromeDebugger1"
}

However, I am not sure I understand the relationship between child processes, web pages, and apps in B2G. Can a child process contain more than one app? Are web pages always managed by the parent process, or do child processes handle those, too?

The plan as of the moment is for the main process's root actor to enumerate child processes, and let the client attach to them. Attaching to a child process starts a root actor in the child process. So the scope of a listTabs request is only the single child process.

If each child process hosts only a single app or page, this is a little strange; the listTabs reply will only ever have one thing in it. But I think it's worthwhile having an "agent" in the child process, for chrome debugging, profiling, and other things like that.

Comment 12

[Jim Blandy :jimb]

Comment on attachment 754543 [details] [diff] [review]
B2G and Fennec changes v2

Review of attachment 754543 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, this looks like what we want. It seems like we will indeed be able to share a RootActor implementation, which feels great.

This is a "work in progress" patch, which is fine, but for what it's worth, I'm hoping that GaiaTabList can be greatly simplified over BrowserTabList. For example, I gather there's only ever one top-level XUL window; managing their comings and goings is responsible for a lot of complexity in BrowserTabList. In fact, since there's only one browser tab (that's the way the code looks, at least), GaiaTabList ought to be able to be extremely simple, to the point that it need not share code with BrowserTabList at all.

::: b2g/chrome/content/dbg-browser-actors.js
@@ +52,3 @@
>  }
>  
> +GaiaTabList.prototype = new BrowserTabList();

How about:

GaiaTabList.prototype = Object.create(BrowserTabList.prototype);

?

@@ +98,2 @@
>  
> +Object.defineProperty(BrowserTabActor.prototype, "title", {

Could we call it GaiaTabActor, and say:

GaiaTabActor.prototype = Object.create(BrowserTabActor.prototype);

as above? Modifying BrowserTabActor in place seems strange. (Although, I guess we'll never use that code, and it's nice to let the old definitions get GC'd.)

Ideally, BrowserTabActor could be parameterized the way RootActor is, but that's certainly not appropriate for this patch.

::: mobile/android/chrome/content/dbg-browser-actors.js
@@ +51,3 @@
>  }
>  
> +MobileTabList.prototype = new BrowserTabList();

Here, too.

Comment 13

[Alexandre Poirot [:ochameau]]

(In reply to Jim Blandy :jimb from comment #11)
> (In reply to Alexandre Poirot (:ochameau) from comment #9)
> > Oh! I immediatly wanted such abtraction when I started working on b2g actors.
> > I was even wondering if we should have an even more abstracted root actor.
> > Here, the base root actor is still Tab oriented, but when working with b2g,
> > the rootactor will most likely handle apps rather than tabs.
> 
> Let's talk about this more on-line, but some thoughts for the moment:
> 
> I think it is probably too late to rename the 'listTabs' request; we have
> just enough people doing cross-version debugging that we should keep names
> stable.

Yes, that's why I was suggesting that as the "icing on the cake".

> 
> However, it is always okay to add more properties to an existing reply; old
> clients will ignore them. So, for example, we could add an 'appList'
> parameter to RootActor; if provided, the root actor would reply to listTabs
> requests like this:
> 
> {
>   "from": "child1.conn0:root",
>   "selected": 0,
>   "tabs": [],
>   "apps": [
>     { url: "app://costcontrol.gaiamobile.org/widget.html" },
>     { url: "app://homescreen.gaiamobile.org/index.html#root", active: true },
>     { url: "app://settings.gaiamobile.org/index.html#root", active: true },
>     { url: "app://sms.gaiamobile.org/index.html" },
>     { url: "app://keyboard.gaiamobile.org/index.html" }
>   ]
>   "chromeDebugger": "puppet:conn0.chromeDebugger1"
> }

Yes, that's what we do and will most likely end up doing, but as time goes, the listTabs will end up returning even more random things.
For example, I don't think we will provide tabs access in first iteration of b2g support. So that we will only return apps here and no tabs.
Also we are using Tab actors "BrowserTabActor" for apps by giving an <iframe mozapp> instead of the <xul:browser>. It works as both have similar APIs, but again we are hijacking tabs stuff to make that work easily for apps.
So far, I've only seen bad semantics, nothing really important but as I see many big refactoring in devtools... I suggested to also tweak it.

Otherwise, one another thing I've been wanting is to be more lazy.
For example, it may be easier to create actors on demand.
So that listTabs ends up being the root actor "connect" command that offer a way to fetch on-demand other live-data (tab titles/urls) and/or actors (tabs, apps, chrome).

> 
> However, I am not sure I understand the relationship between child
> processes, web pages, and apps in B2G. Can a child process contain more than
> one app? 

Each app lives in its own child process.

> Are web pages always managed by the parent process, or do child
> processes handle those, too?

There is no nested child process. But that's something we would like to implement for the browser app. So currently, Browser app itself lives in parent process and its content tabs are in child processes. And I think that each tab also lives in its own child process, but we may also share child processes for tabs, like Chrome.

> 
> The plan as of the moment is for the main process's root actor to enumerate
> child processes, and let the client attach to them. Attaching to a child
> process starts a root actor in the child process. So the scope of a listTabs
> request is only the single child process.
> 
> If each child process hosts only a single app or page, this is a little
> strange; the listTabs reply will only ever have one thing in it. But I think
> it's worthwhile having an "agent" in the child process, for chrome
> debugging, profiling, and other things like that.

Sounds reasonable. I just have many ideas in mind where listTabs looks wrong and I'll most likely suggest some tweaks in my App actor integration.
For example, the app actor should be loaded and created on-demand.
It will also be loaded once in the parent process and would not have any sense in any child.

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: B2G and Fennec changes v3

As discussed in IRC I had opted for a minimum-changes approach, but I agree that simplifying the b2g actors is better. I incorporated all of your comments above and from IRC, and I also changed the applicationType property of the "hello" packet to "operating-system" from "browser", in the case of Firefox OS. Is that OK? I think that you had suggested something like that in the past, and perhaps the client or simulator code could use it to tune their behavior.

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Alexandre Poirot (:ochameau) from comment #13)
> (In reply to Jim Blandy :jimb from comment #11)
> > (In reply to Alexandre Poirot (:ochameau) from comment #9)
> > However, it is always okay to add more properties to an existing reply; old
> > clients will ignore them. So, for example, we could add an 'appList'
> > parameter to RootActor; if provided, the root actor would reply to listTabs
> > requests like this:
> > 
> > {
> >   "from": "child1.conn0:root",
> >   "selected": 0,
> >   "tabs": [],
> >   "apps": [
> >     { url: "app://costcontrol.gaiamobile.org/widget.html" },
> >     { url: "app://homescreen.gaiamobile.org/index.html#root", active: true },
> >     { url: "app://settings.gaiamobile.org/index.html#root", active: true },
> >     { url: "app://sms.gaiamobile.org/index.html" },
> >     { url: "app://keyboard.gaiamobile.org/index.html" }
> >   ]
> >   "chromeDebugger": "puppet:conn0.chromeDebugger1"
> > }
> 
> Yes, that's what we do and will most likely end up doing, but as time goes,
> the listTabs will end up returning even more random things.
> For example, I don't think we will provide tabs access in first iteration of
> b2g support. So that we will only return apps here and no tabs.
> Also we are using Tab actors "BrowserTabActor" for apps by giving an <iframe
> mozapp> instead of the <xul:browser>. It works as both have similar APIs,
> but again we are hijacking tabs stuff to make that work easily for apps.
> So far, I've only seen bad semantics, nothing really important but as I see
> many big refactoring in devtools... I suggested to also tweak it.

The big benefit from using the existing semantics is that a generic client can support both cases without unnecessary code duplication. As long as that premise holds, I think it would be best to take advantage of it. If we come to a point where handling apps and tabs becomes fundamentally different, then by all means let's change it.

> Otherwise, one another thing I've been wanting is to be more lazy.
> For example, it may be easier to create actors on demand.
> So that listTabs ends up being the root actor "connect" command that offer a
> way to fetch on-demand other live-data (tab titles/urls) and/or actors
> (tabs, apps, chrome).

Note that registering an actor using the DebugerServer.addGlobalActor/addTabActor API instantiates the actor lazily (when the first packet to it is received). I am not clear though on why you would want to remove the title and URL from the tab/app form. Is there a performance problem with it for apps?

Comment 16

[Alexandre Poirot [:ochameau]]

(In reply to Panos Astithas [:past] from comment #15)
> > Otherwise, one another thing I've been wanting is to be more lazy.
> > For example, it may be easier to create actors on demand.
> > So that listTabs ends up being the root actor "connect" command that offer a
> > way to fetch on-demand other live-data (tab titles/urls) and/or actors
> > (tabs, apps, chrome).
> 
> Note that registering an actor using the
> DebugerServer.addGlobalActor/addTabActor API instantiates the actor lazily
> (when the first packet to it is received). 

But we still load the actor script and all its dependencies. It would just be easier to load the actor script lazily as well. I worked around that by loading webapps actor dependencies in its constructor.

> I am not clear though on why you
> would want to remove the title and URL from the tab/app form. Is there a
> performance problem with it for apps?

Not a concrete issue. It's just that if you open the regular connect page and connect to a tabs, the Apps list will be useless. Same thing for the opposite action. If we craft an about:remote-apps page, we won't have any use of anything else but app actors and apps metadata.

I think that I'm just hijacking this bug to give you my overall feeback after having played with Debugger API. I'm really happy with the work being done in this bug. It definitely goes in the same direction I'd like to go!

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Panos Astithas [:past] from comment #14)
> Created attachment 755312 [details] [diff] [review]
> B2G and Fennec changes v3

Forgot to mention that I haven't finished the test yet. It's still the same thing you sent me.

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 753177 [details] [diff] [review]
Patch v2.1

Review of attachment 753177 [details] [diff] [review]:
-----------------------------------------------------------------

Let me state for the record that this patch has been reviewed and any changes I've requested have been included in the second patch.

::: toolkit/devtools/server/main.js
@@ +114,5 @@
>  
>      this.xpcInspector = Cc["@mozilla.org/jsinspector;1"].getService(Ci.nsIJSInspector);
>      this.initTransport(aAllowConnectionCallback);
>      this.addActors("resource://gre/modules/devtools/server/actors/script.js");
> +    this.addActors("resource://gre/modules/devtools/server/actors/root.js");

I was thinking that it would be cleaner for DebuggerServer.init to just load the root actor. That would be more inline with the expectations of projects like Marionette (which is a moot point now, but there may be others in the future).

@@ +177,5 @@
>    /**
>     * Install Firefox-specific actors.
>     */
>    addBrowserActors: function DS_addBrowserActors() {
> +    this.addActors("resource://gre/modules/devtools/server/actors/root.js");

Then we could have DebuggerServer.addBrowserActors load the script actors, which are arguably part of the browser. And also not load the root actor twice.

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: B2G and Fennec changes v4

Ignoring test-related things for the moment, this fixes all the issues I've encountered so far. Let me know what you think.

Comment 20

[Jim Blandy :jimb]

(In reply to Alexandre Poirot (:ochameau) from comment #13)
> Yes, that's what we do and will most likely end up doing, but as time goes,
> the listTabs will end up returning even more random things.

Yep. :(

> For example, I don't think we will provide tabs access in first iteration of
> b2g support. So that we will only return apps here and no tabs.
> Also we are using Tab actors "BrowserTabActor" for apps by giving an <iframe
> mozapp> instead of the <xul:browser>. It works as both have similar APIs,
> but again we are hijacking tabs stuff to make that work easily for apps.
> So far, I've only seen bad semantics, nothing really important but as I see
> many big refactoring in devtools... I suggested to also tweak it.

It seems to be that, in the end, B2G will want its own GaiaTabActor or something. BrowserTabActor is not so magical that B2G should go to great lengths to reuse its implementation; it would be better to have code that fits B2G's needs precisely.

ThreadActor seems to know more about TabActor than I would like; we should improve it as needed to make it cleaner to use in B2G. It should perhaps be parameterized (roughly the way RootActor was) to be more easily reused for chrome debugging, app debugging, and so on. The 'globalManager' stuff is a start, but could be improved.

> Otherwise, one another thing I've been wanting is to be more lazy.
> For example, it may be easier to create actors on demand.
> So that listTabs ends up being the root actor "connect" command that offer a
> way to fetch on-demand other live-data (tab titles/urls) and/or actors
> (tabs, apps, chrome).

Did you notice that the actors in actor pools can be constructor functions? That is meant to help produce actors lazily. See DebuggerServerConnection.prototype.onPacket.

Thank you for the explanation of B2G's app and tab process arrangements.

> Sounds reasonable. I just have many ideas in mind where listTabs looks wrong
> and I'll most likely suggest some tweaks in my App actor integration.
> For example, the app actor should be loaded and created on-demand.
> It will also be loaded once in the parent process and would not have any
> sense in any child.

The 'Debugger' instance needs to be in the same process as the debuggee; and the protocol is the best way we have at the moment to communicate with a Debugger instance in another process. If the app actor is in the parent, how will we get a ThreadActor dealing with the app's code?

Comment 21

[Jim Blandy :jimb]

Comment on attachment 755591 [details] [diff] [review]
B2G and Fennec changes v4

Review of attachment 755591 [details] [diff] [review]:
-----------------------------------------------------------------

This looks reasonable to me.

Comment 22

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: B2G and Fennec changes v5

Added tests that cover all the cases we've discussed.

Comment 23

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Combined patch

This is the consolidated patch. Asking Dave for a third pair of eyes on the devtools changes, and Mark and Fabrice for the Fennec and B2G changes. This refactoring has reduced the LOC in Fennec and B2G actors by a significant amount.

I'll push to try after the infra issues are resolved.

Comment 24

[Panos Astithas (he/him) [:past] (please ni?)]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=d3d41eda267a

Comment 25

[Panos Astithas (he/him) [:past] (please ni?)]

I should note that I've tested the latest version of the patch on Android, Unagi, desktop b2g and desktop Firefox.

Comment 26

[Panos Astithas (he/him) [:past] (please ni?)]

New try run since the previous one got caught in a repo reset:
https://tbpl.mozilla.org/?tree=Try&rev=1258dfaba07d

Comment 27

[[:fabrice] Fabrice Desré]

Comment on attachment 756099 [details] [diff] [review]
Combined patch

Review of attachment 756099 [details] [diff] [review]:
-----------------------------------------------------------------

I only looked at b2g/chrome/content/ files, and this worked well when I tested.

::: b2g/chrome/content/dbg-browser-actors.js
@@ +12,5 @@
>  /**
> + * Construct a root actor appropriate for use in a server running in B2G. The
> + * returned root actor:
> + * - respects the factories registered with DebuggerServer.addGlobalActor,
> + * - uses a GaiaTabList to supply tab actors,

Please rename everything called Gaia* to Content* for instance. We could use a different front end for b2g...

Comment 28

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 756099 [details] [diff] [review]
Combined patch

/mobile/android parts looked fine to me

Comment 29

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Combined patch with Gaia* renamed to Content*

Renamed Gaia* to Content*.

Comment 30

[Panos Astithas (he/him) [:past] (please ni?)]

Landed, backed out for wrong bug number and relanded:

https://hg.mozilla.org/integration/fx-team/rev/b0e571a21e22
https://hg.mozilla.org/integration/fx-team/rev/f2621bc17cd0
https://hg.mozilla.org/integration/fx-team/rev/295a11e1efef

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/295a11e1efef