1321096 - Prevent loading TabActor in the parent process

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Alexandre Poirot [:ochameau]]

For now, we are loading a bunch of useless code and modules in the parent process as we load webbrowser.js. This module contains useful stuff to be loaded in the parent like the root actor setup function as well as BrowserTabActor and some other addon actors. But TabActor isn't as now that e10s is turned on, we should only use and load the TabActor in the child process. Unless we debug a tab in the parent process. But that should be an edge case.

Also, the opposite is true, we currently load the whole webbrowser.js in the child process whereas we, this time, only need TabActor.

I don't have any metric for this, but I'm expecting it to be positive regarding performance that I'm going to block bug 1320786.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1321096 - Extract TabActor into its own module to prevent loading it in the parent process.

Review commit: https://reviewboard.mozilla.org/r/96372/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96372/

Comment 2

[Alexandre Poirot [:ochameau]]

I wish we could keep the history of both part, but I imagine that's not possible?
Otherwise I tried to keep TabActor history as I think it has more value, but I failed here...

I tried to do: git mv webbrowser.js tabactor.js
but it looks like the convertion to mercurial lost that detail.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Hmm, not sure if you can tweak cinnabar's rename / copy detection to nudge it towards noticing the move when reporting to hg / mozreview...  In the past, this is the one case I've used hg for since I wasn't quite sure how to do it otherwise.

:glandium should know if cinnabar / publishing to mozreview has any dials for this.

Comment 4

[Mike Hommey [:glandium]]

The supported way to do this is to rename/move the file unchanged in a patch and leave the modifications to a subsequent patch.

The unsupported way to do this is to tweak https://github.com/glandium/git-cinnabar/blob/ab2387ecd420a0c30207bc11b7e4a227d105fb67/cinnabar/git.py#L460

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8815471 [details]
Bug 1321096 - Extract TabActor into its own module to prevent loading it in the parent process.

I'll try to address the copy story based on glandium tips,
but before that I need to know if you overall agree with this patch, the names, the precise way I split things?
I may also have to update docs like actor-hierarchy.md accordingly.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8815471 [details]
Bug 1321096 - Extract TabActor into its own module to prevent loading it in the parent process.

https://reviewboard.mozilla.org/r/96372/#review96844

Looks good at a high level.  Have you verified it works as expected (that `webbrowser.js` is only loaded in parent, and `tab.js` only in content)?

::: devtools/server/actors/chrome.js:10
(Diff revision 1)
>  "use strict";
>  
>  const { Ci } = require("chrome");
>  const Services = require("Services");
>  const { DebuggerServer } = require("../main");
> -const { getChildDocShells, TabActor } = require("./webbrowser");
> +const { getChildDocShells, TabActor } = require("./tabactor");

I guess this means we will load `tab.js` in the parent, but only after receiving a get process request for the parent?

::: devtools/server/actors/moz.build:62
(Diff revision 1)
>      'storage.js',
>      'string.js',
>      'styleeditor.js',
>      'styles.js',
>      'stylesheets.js',
> +    'tabactor.js',

Actor modules don't typically have the word `actor` in them, so it seems like this should be named `tab.js`.

Also, actor files are ignored by default in ESLint at the moment.  Add a new exception line to `.eslintignore` so the new file will be linted.

::: devtools/server/actors/webbrowser.js:9
(Diff revision 1)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> -/* global XPCNativeWrapper */
> +var { Ci } = require("chrome");

Since the point of the move is that this file keeps the parent process bits, while `tab.js` has the child process bits, you should add comments to the top of each file to describe this.

And yes, please also update `actor-hierarchy.md`.

::: devtools/shared/builtin-modules.js:182
(Diff revision 1)
>    ThreadSafeChromeUtils,
>    HeapSnapshot,
>  };
>  
>  defineLazyGetter(exports.modules, "Debugger", () => {
> +  dump(" ++ Debugger\n");

Seems like the wrong patch?

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 8815471 [details]
Bug 1321096 - Extract TabActor into its own module to prevent loading it in the parent process.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96372/diff/1-2/

Comment 8

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6)
> Comment on attachment 8815471 [details]
> Bug 1321096 - Extract TabActor into its own module to prevent loading it in
> the parent process.
> 
> https://reviewboard.mozilla.org/r/96372/#review96844
> 
> Looks good at a high level.  Have you verified it works as expected (that
> `webbrowser.js` is only loaded in parent, and `tab.js` only in content)?

Yes, tab.js is only loaded in the targeted process.
If the tab is parent it loads in parent,
if tab is oop, it loads only in the child.
If we open about:debugging it eventually loads in parent and/or child,
depending on the panel.

> 
> ::: devtools/server/actors/chrome.js:10
> (Diff revision 1)
> >  "use strict";
> >  
> >  const { Ci } = require("chrome");
> >  const Services = require("Services");
> >  const { DebuggerServer } = require("../main");
> > -const { getChildDocShells, TabActor } = require("./webbrowser");
> > +const { getChildDocShells, TabActor } = require("./tabactor");
> 
> I guess this means we will load `tab.js` in the parent, but only after
> receiving a get process request for the parent?

Yes, we can see that happening in about:debugging. In the worker panel which does a getProcess request.

> 
> ::: devtools/server/actors/moz.build:62
> (Diff revision 1)
> >      'storage.js',
> >      'string.js',
> >      'styleeditor.js',
> >      'styles.js',
> >      'stylesheets.js',
> > +    'tabactor.js',
> 
> Actor modules don't typically have the word `actor` in them, so it seems
> like this should be named `tab.js`.

Renamed.

> 
> Also, actor files are ignored by default in ESLint at the moment.  Add a new
> exception line to `.eslintignore` so the new file will be linted.

Added.

> 
> ::: devtools/server/actors/webbrowser.js:9
> (Diff revision 1)
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  "use strict";
> >  
> > -/* global XPCNativeWrapper */
> > +var { Ci } = require("chrome");
> 
> Since the point of the move is that this file keeps the parent process bits,
> while `tab.js` has the child process bits, you should add comments to the
> top of each file to describe this.

I added a comment in tab.js, but not in webbrowser.js as it is still loaded in both process.
DebuggerServer.addChildActors does that. This is another story that should be fixed in another bug.
child.js and content-server.jsm both call this method, child.js doesn't need webbrowser whereas content-server.jsm does. content-server.jsm is used for the Content Process Toolboxes which requires a root actor in the child.

> 
> And yes, please also update `actor-hierarchy.md`.
> 

Done, but I simply renamed webbrowser.js to tab.js in the relevant places.

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 8815471 [details]
Bug 1321096 - Extract TabActor into its own module to prevent loading it in the parent process.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96372/diff/2-3/

Comment 10

[Alexandre Poirot [:ochameau]]

\o/ Thanks glangium for the unofficial way, it worked perfectly with C1%!

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8815471 [details]
Bug 1321096 - Extract TabActor into its own module to prevent loading it in the parent process.

https://reviewboard.mozilla.org/r/96372/#review97534

Great, looks good to me.  Thanks for working on it!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b75ffccec23
Extract TabActor into its own module to prevent loading it in the parent process. r=jryans

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2b75ffccec23