If Content process limit is default (1) and dom.ipc.processCount.web=4, then 5 Firefox processes are opened

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Preferences
P1
critical
VERIFIED FIXED
a month ago
22 days ago

People

(Reporter: brindusat, Assigned: timdream)

Tracking

55 Branch
Firefox 56
All
Windows 10
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox55 verified, firefox56 verified)

Details

(Whiteboard: [photon-preference][e10s-multi:?])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

Created attachment 8888285 [details]
ContentProcessLimit1(default)But5Processes.jpg

[Affected versions]: 
Beta 55.0b10

[Affected platforms]:
Platforms: Windows 10 x 64, Ubuntu 16.04 and Mac OS X 10.10

[Preconditions]: make sure e10s is enabled and dom.ipc.processCount.web=4:
1. Open Beta with a new profile.
2. Go to about:config and search for "dom.ipc.processCount.web". If this is not 4 repeat steps 1 and 2 until the value of dom.ipc.processCount.web is 4.
3. In about:config search for "browser.tabs.remote.autostart". If it is not true, set it to true and restart the browser.

[Steps to reproduce]:
1. Open Beta with the profile prepared in preconditions
2. Go to about:preferences#general at Performance section
3. Uncheck the "Use recommended performance settings". -> Content process limit is set to 1(default).
4. Open 5 tabs and verify in Process Explorer the number of processes opened for Firefox.


[Expected result]:
The expected maximum processes will be 1 more than the values set, in our case should have 2 processes.

[Actual result]:
The number of processes opened is 5.

[Note]:
If the user modifies the "Content process limit" to other value than the default one, then the correct number of processes is opened.
status-firefox55: --- → affected
Whiteboard: [photon-preference]

Comment 1

a month ago
One for gpu process, the other for tab content processes. total=5 processes. no?
I guess so, but I am not sure. I attached a screenshot to the bugs, does this help you?

Comment 3

a month ago
> [Expected result]:
> The expected maximum processes will be 1 more than the values set, in our case should have 2 processes.

Noooo, If user has not set dom.ipc.processCount (default content process limit), firefox will read pref dom.ipc.processCount.web=4 so you get 4 content processes, not 2.

> [Note]:
> If the user modifies the "Content process limit" to other value than the default one, then the correct number of processes is opened.

Indeed, If user has set dom.ipc.processCount, firefox respect their decision and won't read *.web pref.
Created attachment 8888318 [details]
About_Config_dom.ipc.processCount.jpg

In about:config both dom.ipc.processCount and dom.ipc.processCount.web are set(see attachment):
dom.ipc.processCount=1
dom.ipc.processCount.web=4

My concern is that in about:preferences for the Content process limit is 1, but actually the value 4 of the dom.ipc.processCount.web is taken into consideration.

Comment 5

29 days ago
> [Preconditions]: make sure e10s is enabled and dom.ipc.processCount.web=4:
> 1. Open Beta with a new profile.
> 2. Go to about:config and search for "dom.ipc.processCount.web". If this is
> not 4 repeat steps 1 and 2 until the value of dom.ipc.processCount.web is 4.
I'm wondering why repeat steps 1 and 2 could make `dom.ipc.processCount.web` change to 4 from 1. Is that a bug or something?
Maybe the simple solution here is ensure `dom.ipc.processCount` and `dom.ipc.processCount.web` are the same value when creating the new profile.

And I would say this might not be a Photon (Preferences) issue since the only thing Preferences do is creating an UI to set `dom.ipc.processCount` value and we already could reproduce the issue in current beta release.

Updated

29 days ago
Whiteboard: [photon-preference] → [photon-preference][triage]
If I read the code correctly here

http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/toolkit/xre/nsAppRunner.cpp#5183-5198

The problem lies on the fact that dom.ipc.processCount=1 is not a user defined value on Beta, so it goes on and decide to use the number set in |dom.ipc.processCount.web| instead. 

I wonder if we should patch this and make it respect |browser.preferences.defaultPerformanceSettings.enabled| which maps to "Use recommended performance settings" in about:preferences.

Blake, the code was check-in in bug 1349363. Do you think the above is the right diagnostic and also the right approach?

(Please be aware that we are talking about a Fx55 feature here, currently in beta)
Flags: needinfo?(mrbkap)
Whiteboard: [photon-preference][triage] → [photon-preference][triage][e10s-multi:?]
Another issue is how we mark default value in the UI: we simply read the default value of the pref |dom.ipc.processCount|, while the "default" can change on Beta with the multi-e10s roll out.

http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/browser/components/preferences/in-content-new/main.js#1137

We should probably fix that in another bug.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Given that this looks strightforward, I submitted a patch to do exactly comment 6 and another to do comment 7. Let me know if this is the right way to address this bug.
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

29 days ago
mozreview-review
Comment on attachment 8888638 [details]
Bug 1382649 - Take the process count value set by e10s rollout as the default in the about:preferences UI,

https://reviewboard.mozilla.org/r/159658/#review165058

Looks good to me. Thank you.
Attachment #8888638 - Flags: review?(evan) → review+

Updated

29 days ago
Flags: qe-verify+
Priority: -- → P1
QA Contact: hani.yacoub
Whiteboard: [photon-preference][triage][e10s-multi:?] → [photon-preference][e10s-multi:?]

Comment 14

28 days ago
mozreview-review
Comment on attachment 8888637 [details]
Bug 1382649 - Unset dom.ipc.processCount pref value should be taken as a user opt-in value if browser.preferences.defaultPerformanceSettings.enabled is false.

https://reviewboard.mozilla.org/r/159656/#review165240

I have a couple of whitespace nits. This looks like a decent approach. I was playing with the idea of setting "dom.ipc.processCount"'s default to 0 on Beta and Release, which I believe would also fix this bug (it also required patching GetMaxWebProcessCount to make sure we return at least 1), but that might have only been clearer in my mind.

::: browser/extensions/e10srollout/bootstrap.js:262
(Diff revision 2)
>  
>  function optedIn() {
>    let e10s = Preferences.get(PREF_E10S_OPTED_IN, false) ||
>               Preferences.get(PREF_E10S_FORCE_ENABLED, false);
> -  let multi = Preferences.isSet(PREF_E10S_PROCESSCOUNT);
> +  let multi = Preferences.isSet(PREF_E10S_PROCESSCOUNT) ||
> +    !Preferences.get(PREF_USE_DEFAULT_PERF_SETTINGS, true);

Nit: fix the indentation for `multi` to look like `e10s`.

::: toolkit/xre/nsAppRunner.cpp:5193
(Diff revision 2)
> -  // cohort.
> -  if (Preferences::HasUserValue(optInPref)) {
> +    Preferences::GetBool(useDefaultPerformanceSettings, true);
> +
> +  // If the user has set dom.ipc.processCount, or if they have opt out of 
> +  // default performances settings from about:preferences, respect their 
> +  // decision regardless of add-ons that might affect their experience or 
> +  // experiment cohort.

Nit: nuke the trailing whitespace at the ends of these lines.
Attachment #8888637 - Flags: review?(mrbkap) → review+

Comment 15

28 days ago
mozreview-review
Comment on attachment 8888638 [details]
Bug 1382649 - Take the process count value set by e10s rollout as the default in the about:preferences UI,

https://reviewboard.mozilla.org/r/159658/#review165334
Attachment #8888638 - Flags: review?(mrbkap) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Flags: needinfo?(mrbkap)
status-firefox56: --- → affected
Severity: major → critical

Comment 19

25 days ago
mozreview-review
Comment on attachment 8888638 [details]
Bug 1382649 - Take the process count value set by e10s rollout as the default in the about:preferences UI,

https://reviewboard.mozilla.org/r/159658/#review165922
Attachment #8888638 - Flags: review?(jaws) → review+
Keywords: checkin-needed
Try push to beta: https://hg.mozilla.org/try/pushloghtml?changeset=6da6591b7c6d1c313fdf5b6b10275faa9ac6e871
Sorry, https://treeherder.mozilla.org/#/jobs?repo=try&revision=6da6591b7c6d1c313fdf5b6b10275faa9ac6e871
Comment on attachment 8888638 [details]
Bug 1382649 - Take the process count value set by e10s rollout as the default in the about:preferences UI,

Approval Request Comment
[Feature/Bug causing the regression]: Feature in bug 1364070 
[User impact if declined]: User will see incorrect default content process count; set to the incorrect default will be ignored and the real connent process count default (set by e10s rollout add-on) will still apply.
[Is this code covered by automated tests?]: No. This cannot be tested fully on Nightly since e10s rollout add-on does not apply on the channel.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: QE can manually simulate comment 0 on Nightly by setting |dom.ipc.processCount.web| to anything other than 4, and verify that # of web content process observed in will be that number in Process Explorer, and the default process count in Preferences UI is that number.
[List of other uplifts needed for the feature/fix]: Two patches should be uplifted together.
[Is the change risky?]: Maybe not.
[Why is the change risky/not risky?]: The change is fairly localized and only involves reading a pref.
[String changes made/needed]:
Attachment #8888638 - Flags: approval-mozilla-beta?
Attachment #8888637 - Flags: approval-mozilla-beta?

Comment 23

25 days ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ce12a37d6926
Unset dom.ipc.processCount pref value should be taken as a user opt-in value if browser.preferences.defaultPerformanceSettings.enabled is false. r=mrbkap
https://hg.mozilla.org/integration/autoland/rev/e2089c0f89cd
Take the process count value set by e10s rollout as the default in the about:preferences UI, r=evanxd,jaws,mrbkap
Keywords: checkin-needed

Comment 24

25 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ce12a37d6926
https://hg.mozilla.org/mozilla-central/rev/e2089c0f89cd
Status: ASSIGNED → RESOLVED
Last Resolved: 25 days ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Comment on attachment 8888637 [details]
Bug 1382649 - Unset dom.ipc.processCount pref value should be taken as a user opt-in value if browser.preferences.defaultPerformanceSettings.enabled is false.

fix setting content process count through about:preferences, beta55+
Attachment #8888637 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8888638 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 26

24 days ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/78829e0f0362
https://hg.mozilla.org/releases/mozilla-beta/rev/162941857ff4

Should we have bumped the e10srollout version number too?
status-firefox55: affected → fixed
(In reply to Ryan VanderMeulen [:RyanVM] from comment #26)
> Should we have bumped the e10srollout version number too?

I don't know if we deploy e10s rollout out of the train. mrbkap?
Flags: needinfo?(mrbkap)
Verified as fixed on latest Nightly 56.0a1 on Windows 10 x64 and Mac OS X 10.12.  After setting |dom.ipc.processCount.web| to anything other than 4, the # of web content processes from preferences is updated to that number and the number of processes in Process Explorer for web pages is the same.

On Ubuntu 16.04, with latest Nightly build 56.0a1, Build ID 20170726100241, this is NOT working. The number of processes in preferences is not updated - remains the default value 4, no matter what value is set for dom.ipc.processCount.web. Also, the number of processes opened for Nightly is 4.
Tim, is this the expected behavior for Ubuntu?
Flags: needinfo?(timdream)
Please double confirm that the Nightly version tested does contain the patch of this bug. That's not the intended behavior. I will investigate tomorrow.
Flags: needinfo?(timdream)
Verified again this morning, and now it is working on Ubuntu too. But, while verifying the fix, I observe the following scenario:

1. Start Nightly and set dom.ipc.processCount.web=2
2. Restart Browser. -> In Preferences no of Content Processes is 2(default)
3. Update Content process limit to 3
4. Restart browser -> In Preferences no of Content Processes is 3 (Correct)
5. Update Content process limit to 4
6. Restart browser -> In Preferences no of Content Processes is 2(default)

Is this correct? Shouldn't be 4 the no of Content Processes?
Flags: needinfo?(timdream)
I wonder if the safer thing to do for 55 would be to not expose that setting in about:preferences, given the seemingly complex interactions between different prefs?
(In reply to Brindusa Tot[:brindusat] from comment #30)
> Verified again this morning, and now it is working on Ubuntu too. But, while
> verifying the fix, I observe the following scenario:
> 
> 1. Start Nightly and set dom.ipc.processCount.web=2
> 2. Restart Browser. -> In Preferences no of Content Processes is 2(default)
> 3. Update Content process limit to 3
> 4. Restart browser -> In Preferences no of Content Processes is 3 (Correct)
> 5. Update Content process limit to 4
> 6. Restart browser -> In Preferences no of Content Processes is 2(default)
> 
> Is this correct? Shouldn't be 4 the no of Content Processes?

This is correct. We will label the number set in `dom.ipc.processCount.web` as the default.

(In reply to Julien Cristau [:jcristau] from comment #31)
> I wonder if the safer thing to do for 55 would be to not expose that setting
> in about:preferences, given the seemingly complex interactions between
> different prefs?

It's not complex unless we found something new again. With this patch the feature should be fine for 55.
Flags: needinfo?(timdream)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #32)
> (In reply to Brindusa Tot[:brindusat] from comment #30)
> > Verified again this morning, and now it is working on Ubuntu too. But, while
> > verifying the fix, I observe the following scenario:
> > 
> > 1. Start Nightly and set dom.ipc.processCount.web=2
> > 2. Restart Browser. -> In Preferences no of Content Processes is 2(default)
> > 3. Update Content process limit to 3
> > 4. Restart browser -> In Preferences no of Content Processes is 3 (Correct)
> > 5. Update Content process limit to 4
> > 6. Restart browser -> In Preferences no of Content Processes is 2(default)
> > 
> > Is this correct? Shouldn't be 4 the no of Content Processes?
> 
> This is correct. We will label the number set in `dom.ipc.processCount.web`
> as the default.

I understand that default value is set by `dom.ipc.processCount.web`, but in this scenario user chose to set a different value for the Content process limit, in this case 4 and after a restart, the Content process limit is set back to 2(default).
Please note that this is reproducible only if the limit set by the user for Content process is 4 (this was the default one on Nightly before adding the 'dom.ipc.processCount.web' config parameter). If the user sets other value for Content process limit, like 5 (but not 4) after restart same value remains for Content process limit, in this example 5.

Please let me know if this is clear.
Flags: needinfo?(timdream)
You are right. Let me file a bug to fix this.
Flags: needinfo?(timdream)
Blocks: 1384962
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #27)
> I don't know if we deploy e10s rollout out of the train. mrbkap?

We do, but I'm going to be bumping the e10srollout version number in bug 1384985, so I'll take care of it then.
Flags: needinfo?(mrbkap)
Tested the fix on the latest Beta 55.0b13, observe the following scenario:

1. Open Beta with a new profile.
2. Go to about:config and search for "dom.ipc.processCount.web". If this is not 4 repeat steps 1 and 2 until the value of dom.ipc.processCount.web is 4.
3. In about:config search for "browser.tabs.remote.autostart". If it is not true, set it to true and restart the browser.

[Steps to reproduce]:
1. Open Beta with a new profile.
2. Go to about:config and search for "dom.ipc.processCount.web". If it is not 4, set it to 4.
3. Go to about:preferences#general at Performance section
4. Uncheck the "Use recommended performance settings". -> Content process limit is set to 4(default).
5. Update the content process to 2.
6. Restart browser.

Expected results:
In Preferences page, the Content process limit should be 2 and default value should remain 4(default).

Actual results:
In Preferences page, the Content process limit is 2, but the default value updated to 1(default).
in about:config, the "dom.ipc.processCount.web"  parameter has disappeared.

Is this correct? The parameter "dom.ipc.processCount.web" should be deleted after one restart?
Flags: needinfo?(timdream)
(In reply to Brindusa Tot[:brindusat] from comment #36)
> 2. Go to about:config and search for "dom.ipc.processCount.web". If it is
> not 4, set it to 4.
>
> Is this correct? The parameter "dom.ipc.processCount.web" should be deleted
> after one restart?

Looks like e10s rollout add-on will reset the pref if it didn't classify the profile to be eligible to enable multi-e10s.

http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/browser/extensions/e10srollout/bootstrap.js#175

This can be indeterministic since e10s rollout because user can be put into test groups simply by chance.

http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/browser/extensions/e10srollout/bootstrap.js#241
Flags: needinfo?(timdream)
The only problem I see here is that the labeling for default value is changed from 4(default) to 1(default) after one restart, but the actual value of the Content process limit remains the one that is set by the user.
I logged bug 1385250 for the labeling issue.

Also, for the issue reported in comment 33, was logged bug 1384962.

Considering that issues discovered while verifying this bug will be tracked in other bugs, I will mark this bug verified as fixed on Latest Beta 55.0b13. The bug was verified on Windows 10x 64, Ubuntu 16.04 and Mac OS X 10.12.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
status-firefox56: fixed → verified
You need to log in before you can comment on or make changes to this bug.