Closed
Bug 1321096
Opened 8 years ago
Closed 8 years ago
Prevent loading TabActor in the parent process
Categories
(DevTools :: General, defect)
DevTools
General
Tracking
(firefox53 fixed)
RESOLVED
FIXED
Firefox 53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: ochameau, Assigned: ochameau)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
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 hidden (mozreview-request) |
Assignee | ||
Comment 2•8 years ago
|
||
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.
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.
Flags: needinfo?(mh+mozilla)
Comment 4•8 years ago
|
||
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
Flags: needinfo?(mh+mozilla)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → poirot.alex
Assignee | ||
Comment 5•8 years ago
|
||
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.
Attachment #8815471 -
Flags: feedback?(jryans)
Comment 6•8 years ago
|
||
mozreview-review |
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?
Attachment #8815471 -
Flags: feedback?(jryans) → feedback+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•8 years ago
|
||
(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 hidden (mozreview-request) |
Assignee | ||
Comment 10•8 years ago
|
||
\o/ Thanks glangium for the unofficial way, it worked perfectly with C1%!
Comment 11•8 years ago
|
||
mozreview-review |
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!
Attachment #8815471 -
Flags: review?(jryans) → review+
Comment 12•8 years ago
|
||
Pushed by apoirot@mozilla.com:
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•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•