Closed Bug 1334716 Opened 7 years ago Closed 7 years ago

Make process selection pluggable

Categories

(Core :: DOM: Content Processes, defect)

Product:
Core
Component:
DOM: Content Processes
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Iteration:
54.3 - Mar 6
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: mrbkap, Assigned: mrbkap)

References

Details

(Whiteboard: [e10s-multi:+] )

Attachments

(1 file)

Bug 1334716 - Make process selection a service and implementable in JS.
59 bytes, text/x-review-board-request
gkrizsanits
: review+
Details 
I thought we had a bug for this, but I can't find it. I have a patch that moves process selection into JS and allows for overriding via a contract ID.

What I have right now is a sketch that would be fleshed out. Comments are definitely welcome.
Comment on attachment 8831367 [details]
Bug 1334716 - Make process selection a service and implementable in JS.

https://reviewboard.mozilla.org/r/107922/#review109246

I like this oatch in general, but I'm not sure I get the big picture completly, so let's talk this over a bit more (here or real time which ever works the best for you)

::: dom/base/ProcessSelector.js:20
(Diff revision 1)
> +function RoundRobinSelector() {
> +}
> +
> +RoundRobinSelector.prototype = {
> +  classID:          Components.ID("{c616fcfd-9737-41f1-aa74-cee72a38f91b}"),
> +  QueryInterface:   XPCOMUtils.generateQI([Ci.nsIContentProcessProvider]),

How will this be used from WebExtensions? Do we know what kind of API do we want to expose for this? Or we just want to use this internally from system add-ons?

::: dom/base/ProcessSelector.js:25
(Diff revision 1)
> +  QueryInterface:   XPCOMUtils.generateQI([Ci.nsIContentProcessProvider]),
> +
> +  provideProcess(aType, /*aInitialURI, */aOpener, aProcesses, aCount) {
> +    let maxContentParents = -1;
> +    try {
> +      maxContentParents = Services.prefs.getIntPref(PREF_BRANCH + aType);

I did a bit of refactoring around this area under bug 1324428. I extracted the function that selects the process, and there are some utility functions for getting the max number of processes from a type, etc.

Do you want to rebase this patch on that, or should I do the rebase? (It's an r+ed patch but I have problems with the test so could not landed it yet)

::: dom/base/ProcessSelector.js:40
(Diff revision 1)
> +
> +    if (aCount < maxContentParents) {
> +      return Ci.nsIContentProcessProvider.NEW_PROCESS;
> +    }
> +
> +    let startIdx = Math.floor(Math.random() * maxContentParents);

I don't think this is Round Robin, it's just RandomSelector. Round Robin should store a last index and continue the rotation from there without anything random involved, no? Actually I was promised I look into that in bug 1333799. Feel free to steal it though :)

::: dom/interfaces/base/nsIContentProcess.idl:13
(Diff revision 1)
> +interface nsIURI;
> +
> +[scriptable, builtinclass, uuid(456f58be-29dd-4973-885b-95aece1c9a8a)]
> +interface nsIContentProcessInfo : nsISupports
> +{
> +  /// Is this content process alive?

Any reason for the /// here and below?

::: dom/interfaces/base/nsIContentProcess.idl:23
(Diff revision 1)
> +  readonly attribute int32_t processId;
> +
> +  /// This content process's opener.
> +  readonly attribute nsIContentProcessInfo opener;
> +
> +  // TODO fill me in with tab children/memory/what else?

What's the planned relationship between this struct and the message manager that connects the parent to the content process? Also, I'm not sure where will this be exposed (I might be looking over something in the patch)

Also, shutdown() would be nice for testing I think...
Comment on attachment 8831367 [details]
Bug 1334716 - Make process selection a service and implementable in JS.

https://reviewboard.mozilla.org/r/107922/#review109246

> How will this be used from WebExtensions? Do we know what kind of API do we want to expose for this? Or we just want to use this internally from system add-ons?

I gave up for the moment on adding this to web extensions since I heard that we're not planning on using them for the internal addon stuff. Please correct me if I'm wrong and I'll start working on the webextension API. When I started looking into it a while back, I thought it might be easier to plug into it from JS anyway, so I might end up sitting that API on top of the a JS-implemented version using the contractid.

> I did a bit of refactoring around this area under bug 1324428. I extracted the function that selects the process, and there are some utility functions for getting the max number of processes from a type, etc.
> 
> Do you want to rebase this patch on that, or should I do the rebase? (It's an r+ed patch but I have problems with the test so could not landed it yet)

I'll rebase on top of your patch.

> I don't think this is Round Robin, it's just RandomSelector. Round Robin should store a last index and continue the rotation from there without anything random involved, no? Actually I was promised I look into that in bug 1333799. Feel free to steal it though :)

I'll rename this (and add a round robin selector -- I'm taking the other bug now).

> Any reason for the /// here and below?

It used to be a javadoc comment, but I don't know if anybody even recognizes it anymore. I was lazy when I was prototyping. I'll move these over to the better-known `/** ... */` style comments.

> What's the planned relationship between this struct and the message manager that connects the parent to the content process? Also, I'm not sure where will this be exposed (I might be looking over something in the patch)
> 
> Also, shutdown() would be nice for testing I think...

Currently, this is unrelated to the message manager. It should be easy to add an attribute pointing to it, though. The only API added in this patch allows JS code to select an existing process for a new tab (or for a tab remoteness change, I think) or to ask for a new process to be spun up. As things stand, we actually need to add more information about e.g. any potential pre-allocated processes that may or may not be ready, as that might affect the decision. If we decide we like this approach, we'd probably also add APIs for controlling pre-allocation of processes, the ability to shut down processes and more; then, we could write JS services to handle process allocation and shutdown (which would be pluggable).

Does that make sense?
(In reply to Blake Kaplan (:mrbkap) from comment #3)
> I gave up for the moment on adding this to web extensions since I heard that
> we're not planning on using them for the internal addon stuff. Please
> correct me if I'm wrong and I'll start working on the webextension API. When
> I started looking into it a while back, I thought it might be easier to plug
> into it from JS anyway, so I might end up sitting that API on top of the a
> JS-implemented version using the contractid.

For the internal add-ons I don't think we will use Web Extensions, but I would not
mind add-on authors experimenting with this a bit on their own. It's not clear
what strategy will we use in the long run, and who knows, we might get some nice
ideas from there for free :) Also, I would think that this is a fun API to play with.
Anyway, this should be an optional follow-up work with someone from the add-on team
involved, and possibly doing the work as well on top of this patch, when it becomes
a priority for them.

> I'll rebase on top of your patch.

Great, thanks, I'll get back to it today and try to land it then.

> Currently, this is unrelated to the message manager. It should be easy to
> add an attribute pointing to it, though. The only API added in this patch
> allows JS code to select an existing process for a new tab (or for a tab
> remoteness change, I think) or to ask for a new process to be spun up. As
> things stand, we actually need to add more information about e.g. any
> potential pre-allocated processes that may or may not be ready, as that
> might affect the decision. If we decide we like this approach, we'd probably
> also add APIs for controlling pre-allocation of processes, the ability to
> shut down processes and more; then, we could write JS services to handle
> process allocation and shutdown (which would be pluggable).
> 
> Does that make sense?

In my mind on the parent side the message manager sort of acts like a handle to the content process (initialProcessData, loadProcessScript), and this object will become another handle to the same thing (it is really not under the hood but from JS it feels like it). I think once we make CPI more accessible from JS, very soon someone will ask the question how to find the related message manager to it, or the other way around, and currently that's not trivial to me. But that is not a concern for now and will be easy to fix anyway. Everything else makes perfect sense to me.
Assignee: nobody → mrbkap
Comment on attachment 8831367 [details]
Bug 1334716 - Make process selection a service and implementable in JS.

Cancelling the review flag for now until the rebased version.
Attachment #8831367 - Flags: review?(gkrizsanits)
Passing the initial URL is going to have to happen in a followup bug. Right now, we create the frameloader (and the remote process) way before we load the URL. I don't think that's required (though maybe checking in with the new service on top-level navigation might be the easiest way to fix it), so I'm going to attach a patch with Gabor's comments addressed and file a followup for that.
Comment on attachment 8831367 [details]
Bug 1334716 - Make process selection a service and implementable in JS.

https://reviewboard.mozilla.org/r/107922/#review113634

Thanks for rebasing it! I think there is a small bug in GetNewOrUsedBrowserProcess, other than that it looks great to me.

::: dom/interfaces/base/nsIContentProcess.idl:36
(Diff revision 3)
> +   * The process manager for this ContentParent (so a process message manager
> +   * as opposed to a frame message manager.
> +   */
> +  readonly attribute nsIMessageSender messageManager;
> +
> +  // TODO fill me in with tab children/memory/what else?

I think this comment can go.

::: dom/ipc/ContentParent.cpp:505
(Diff revision 3)
> +  if (!mContentParent) {
> +    return NS_ERROR_NOT_INITIALIZED;
> +  }
> +
> +  if (ContentParent* opener = mContentParent->Opener()) {
> +    NS_IF_ADDREF(*aInfo = opener->ScriptableHelper());

I probably would have used some kind of ref pointer and forget here to do the addref, but I kind of like this version better... So what's the current policy about using NS macros or XPCOM smart pointers in these cases?

::: dom/ipc/ContentParent.cpp:793
(Diff revision 3)
> +    // If there was a problem with the JS chooser, fall back to a random
> +    // selection.
> +    NS_WARNING("nsIContentProcessProvider failed to return a process");
> +    p = RandomSelect(contentParents, aOpener, maxContentParents);
>    }

Value of |p| after set here seems to be ignored after this line. Shouldn't the |if (0 <= index ...| branch above be moved after the closure of this 'else' branch?

::: dom/ipc/ContentParent.cpp:1373
(Diff revision 3)
> +  if (mScriptableHelper) {
> +    static_cast<ScriptableCPInfo*>(mScriptableHelper.get())->ProcessDied();
> +    mScriptableHelper = nullptr;
> +  }
> +

I'm not sure that ShutDownProcess is always called... (see NotifyTabDestroying). Maybe MarkAsDead would be a better place for this? Anyway, your assert in the destructor will tell I guess...
Attachment #8831367 - Flags: review?(gkrizsanits) → review+
Whiteboard: [e10s-multi:?]
Comment on attachment 8831367 [details]
Bug 1334716 - Make process selection a service and implementable in JS.

https://reviewboard.mozilla.org/r/107922/#review114736
Blocks: e10s-multi-aurora
Whiteboard: [e10s-multi:?] → [e10s-multi:+]
https://treeherder.mozilla.org/#/jobs?repo=try&revision=12ee26110ca8ebba52a65115df57568858c4aeb5

Gabor, would you mind re-looking at the most recent rebased patch?
Flags: needinfo?(gkrizsanits)
(In reply to Blake Kaplan (:mrbkap) from comment #12)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=12ee26110ca8ebba52a65115df57568858c4aeb5
> 
> Gabor, would you mind re-looking at the most recent rebased patch?

Uh... not sure what happened with that try run.

Anyway, I think we should also check if we reached the max process count before defaulting to the random selector (the C++ version of the random selector does not seem to return early if the max process count has not been reached yet)

I don't see why you prefer goto, over just putting the block in the middle section of GetNewOrUsedBrowserProcess (process reusing bits) in the else branch of aRemoteType.EqualsLiteral(LARGE_ALLOCATION_REMOTE_TYPE).

// If the provider returned an existing ContentParent, us that one.
missing 'e' in 'use'
Flags: needinfo?(gkrizsanits)
Lgtm.
Flags: needinfo?(gkrizsanits)
Pushed by mrbkap@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b30e076b9d15
Make process selection a service and implementable in JS. r=krizsa
Backed out for failing e.g. toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_tab_teardown.html on Linux with e10s enabled:

https://hg.mozilla.org/integration/autoland/rev/1801a4ccf60358919e2c39e121477042b8edb151

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b30e076b9d1562078772c877c4bad77f349079bb&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log (please also check the other M-e10s 10 failures): https://treeherder.mozilla.org/logviewer.html#?job_id=80047156&repo=autoland

[task 2017-02-24T19:31:18.846071Z] 19:31:18     INFO - Buffered messages logged at 19:31:04
[task 2017-02-24T19:31:18.848317Z] 19:31:18     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_tab_teardown.html | ExtensionContext after closing tab 
[task 2017-02-24T19:31:18.850877Z] 19:31:18     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_tab_teardown.html | unload tab's ExtensionContext 
[task 2017-02-24T19:31:18.854534Z] 19:31:18     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_tab_teardown.html | ExtensionContext URL at closing tab should be tab URL 
[task 2017-02-24T19:31:18.856565Z] 19:31:18     INFO - SpawnTask.js | Leaving test test_extension_page_tabs_create_reload_and_close
[task 2017-02-24T19:31:18.858720Z] 19:31:18     INFO - SpawnTask.js | Entering test test_extension_page_window_open_reload_and_close
[task 2017-02-24T19:31:18.861946Z] 19:31:18     INFO - Extension loaded
[task 2017-02-24T19:31:18.869516Z] 19:31:18     INFO - Buffered messages finished
[task 2017-02-24T19:31:18.876520Z] 19:31:18     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_tab_teardown.html | ExtensionContext change for opening a tab - got +0, expected 1
[task 2017-02-24T19:31:18.879388Z] 19:31:18     INFO -     SimpleTest.is@SimpleTest/SimpleTest.js:310:5
[task 2017-02-24T19:31:18.881160Z] 19:31:18     INFO -     runTabReloadAndCloseTest@toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_tab_teardown.html:32:3
[task 2017-02-24T19:31:18.883322Z] 19:31:18     INFO -     test_extension_page_window_open_reload_and_close@toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_tab_teardown.html:145:10
[task 2017-02-24T19:31:18.888322Z] 19:31:18     INFO -     onFulfilled@SimpleTest/SpawnTask.js:69:15
[task 2017-02-24T19:31:18.889988Z] 19:31:18     INFO -     Async*next@SimpleTest/SpawnTask.js:104:45
[task 2017-02-24T19:31:18.896478Z] 19:31:18     INFO -     onFulfilled@SimpleTest/SpawnTask.js:73:7
[task 2017-02-24T19:31:18.898674Z] 19:31:18     INFO -     promise callback*next@SimpleTest/SpawnTask.js:104:45
[task 2017-02-24T19:31:18.900596Z] 19:31:18     INFO -     onFulfilled@SimpleTest/SpawnTask.js:73:7
[task 2017-02-24T19:31:18.903637Z] 19:31:18     INFO -     Async*next@SimpleTest/SpawnTask.js:104:45
[task 2017-02-24T19:31:18.906429Z] 19:31:18     INFO -     onFulfilled@SimpleTest/SpawnTask.js:73:7
[task 2017-02-24T19:31:18.908257Z] 19:31:18     INFO -     promise callback*next@SimpleTest/SpawnTask.js:104:45
[task 2017-02-24T19:31:18.910176Z] 19:31:18     INFO -     onFulfilled@SimpleTest/SpawnTask.js:73:7
[task 2017-02-24T19:31:18.916228Z] 19:31:18     INFO -     promise callback*next@SimpleTest/SpawnTask.js:104:45
[task 2017-02-24T19:31:18.919018Z] 19:31:18     INFO -     onFulfilled@SimpleTest/SpawnTask.js:73:7
[task 2017-02-24T19:31:18.920842Z] 19:31:18     INFO -     co/<@SimpleTest/SpawnTask.js:58:5
[task 2017-02-24T19:31:18.922881Z] 19:31:18     INFO -     co@SimpleTest/SpawnTask.js:54:10
[task 2017-02-24T19:31:18.924823Z] 19:31:18     INFO -     toPromise@SimpleTest/SpawnTask.js:122:60
[task 2017-02-24T19:31:18.927193Z] 19:31:18     INFO -     next@SimpleTest/SpawnTask.js:103:19
[task 2017-02-24T19:31:18.929234Z] 19:31:18     INFO -     onFulfilled@SimpleTest/SpawnTask.js:73:7
[task 2017-02-24T19:31:18.933048Z] 19:31:18     INFO -     promise callback*next@SimpleTest/SpawnTask.js:104:45
[task 2017-02-24T19:31:18.934898Z] 19:31:18     INFO -     onFulfilled@SimpleTest/SpawnTask.js:73:7
[task 2017-02-24T19:31:18.936957Z] 19:31:18     INFO -     co/<@SimpleTest/SpawnTask.js:58:5
[task 2017-02-24T19:31:18.942816Z] 19:31:18     INFO -     co@SimpleTest/SpawnTask.js:54:10
[task 2017-02-24T19:31:18.944565Z] 19:31:18     INFO -     add_task/<@SimpleTest/SpawnTask.js:270:9
[task 2017-02-24T19:31:18.946291Z] 19:31:18     INFO -     setTimeout handler*SimpleTest_setTimeoutShim@SimpleTest/SimpleTest.js:672:12
[task 2017-02-24T19:31:18.949192Z] 19:31:18     INFO -     add_task@SimpleTest/SpawnTask.js:269:7
[task 2017-02-24T19:31:18.951109Z] 19:31:18     INFO -     @toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_tab_teardown.html:66:1
Flags: needinfo?(mrbkap)
Pushed by mrbkap@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59c96f5f613f
Make process selection a service and implementable in JS. r=krizsa
Flags: needinfo?(mrbkap)
https://hg.mozilla.org/mozilla-central/rev/59c96f5f613f
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Iteration: --- → 54.3 - Mar 6
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: