Closed
Bug 1382649
Opened 7 years ago
Closed 7 years ago
If Content process limit is default (1) and dom.ipc.processCount.web=4, then 5 Firefox processes are opened
Categories
(Firefox :: Settings UI, defect, P1)
Tracking
()
VERIFIED
FIXED
Firefox 56
People
(Reporter: btot, Assigned: timdream)
References
Details
(Whiteboard: [photon-preference][e10s-multi:?])
Attachments
(4 files)
393.80 KB,
image/jpeg
|
Details | |
47.14 KB,
image/jpeg
|
Details | |
59 bytes,
text/x-review-board-request
|
mrbkap
:
review+
jcristau
:
approval-mozilla-beta+
|
Details |
59 bytes,
text/x-review-board-request
|
evanxd
:
review+
jaws
:
review+
mrbkap
:
review+
jcristau
:
approval-mozilla-beta+
|
Details |
[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.
Reporter | ||
Updated•7 years ago
|
status-firefox55:
--- → affected
Whiteboard: [photon-preference]
Comment 1•7 years ago
|
||
One for gpu process, the other for tab content processes. total=5 processes. no?
Reporter | ||
Comment 2•7 years ago
|
||
I guess so, but I am not sure. I attached a screenshot to the bugs, does this help you?
Comment 3•7 years 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.
Reporter | ||
Comment 4•7 years ago
|
||
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•7 years 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•7 years ago
|
Whiteboard: [photon-preference] → [photon-preference][triage]
Assignee | ||
Comment 6•7 years ago
|
||
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:?]
Assignee | ||
Comment 7•7 years ago
|
||
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) |
Assignee | ||
Comment 10•7 years ago
|
||
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•7 years 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•7 years ago
|
Flags: qe-verify+
Priority: -- → P1
QA Contact: hani.yacoub
Whiteboard: [photon-preference][triage][e10s-multi:?] → [photon-preference][e10s-multi:?]
Comment 14•7 years 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•7 years 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) |
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(mrbkap)
Reporter | ||
Updated•7 years ago
|
status-firefox56:
--- → affected
Reporter | ||
Updated•7 years ago
|
Severity: major → critical
Comment 19•7 years 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+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 20•7 years ago
|
||
Try push to beta: https://hg.mozilla.org/try/pushloghtml?changeset=6da6591b7c6d1c313fdf5b6b10275faa9ac6e871
Assignee | ||
Comment 21•7 years ago
|
||
Sorry, https://treeherder.mozilla.org/#/jobs?repo=try&revision=6da6591b7c6d1c313fdf5b6b10275faa9ac6e871
Assignee | ||
Comment 22•7 years ago
|
||
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?
Assignee | ||
Updated•7 years ago
|
Attachment #8888637 -
Flags: approval-mozilla-beta?
Comment 23•7 years 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ce12a37d6926 https://hg.mozilla.org/mozilla-central/rev/e2089c0f89cd
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Comment 25•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8888638 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 26•7 years ago
|
||
bugherder uplift |
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?
Assignee | ||
Comment 27•7 years ago
|
||
(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)
Reporter | ||
Comment 28•7 years ago
|
||
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)
Assignee | ||
Comment 29•7 years ago
|
||
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)
Reporter | ||
Comment 30•7 years ago
|
||
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)
Comment 31•7 years ago
|
||
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?
Assignee | ||
Comment 32•7 years ago
|
||
(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)
Reporter | ||
Comment 33•7 years ago
|
||
(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)
Assignee | ||
Comment 34•7 years ago
|
||
You are right. Let me file a bug to fix this.
Flags: needinfo?(timdream)
Comment 35•7 years ago
|
||
(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)
Reporter | ||
Comment 36•7 years ago
|
||
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)
Assignee | ||
Comment 37•7 years ago
|
||
(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)
Reporter | ||
Comment 38•7 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•