Loading Preferences page resets user set process count value if the value happen to be the default

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Preferences
P1
critical
VERIFIED FIXED
28 days ago
23 days ago

People

(Reporter: timdream, Assigned: timdream)

Tracking

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

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

(2 attachments)

+++ This bug was initially created as a clone of Bug #1382649 +++

STR originally filed in bug 1382649 comment 33.

1. Set dom.ipc.processCount.web to 2 in Nightly
2. Go to Preferences, uncheck "use default performance"
3. Set the process count to 4 with the UI (4 is the default dom.ipc.processCount value in Nightly)
4. Reload the Perferences page

Expected:

1. process count set to 4

Actual:

1. process count flip to "2 (default)"

[Tracking Requested - why for this release]: Follow up fix to bug 1382649, which was incomplete.
Comment hidden (mozreview-request)
Unfortunately, I cannot verify my tests at home because of bug 1252976. Try run is scheduled on MozReview. I did manually verify the patch with STR above.
Assignee: nobody → timdream
Status: NEW → ASSIGNED
I think you linked to the wrong bug in comment #2? That bug is fixed.

Comment 4

27 days ago
mozreview-review
Comment on attachment 8890903 [details]
Bug 1384962 - Don't set process count for user on Preferences page load,

https://reviewboard.mozilla.org/r/162110/#review167590

::: commit-message-658cb:3
(Diff revision 1)
> +Bug 1384962 - Don't set process count for user on Preferences page load, r=jaws
> +
> +This is a follow-up to incompete fix in bug 1382649. In that bug, I attempted to

s/incompete/incomplete/

::: browser/components/preferences/in-content-new/main.js:1117
(Diff revision 1)
>          accelerationPref.value != accelerationPref.defaultValue) {
>        defaultPerformancePref.value = false;
>      }
>    },
>  
> -  updatePerformanceSettingsBox() {
> +  updatePerformanceSettingsBox(changed) {

Please use a object as the argument to the function so we can have "named parameters".

updatePerformanceSettingsBox({duringChangeEvent}) {
  ...
}

Then the calling code would be:
this.updatePerformanceSettingsBox({duringChangeEvent: true});
Attachment #8890903 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> I think you linked to the wrong bug in comment #2? That bug is fixed.

I was meant to link to bug 1384153 in comment 2, thanks for spotting that.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Created attachment 8891168 [details] [diff] [review]
beta.patch

Approval Request Comment
[Feature/Bug causing the regression]: Incomplete fix in bug 1382649
[User impact if declined]: On Beta, user will see their process count revert to value set by e10s rollout if they set the process count value to 1.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: See STR in comment 0 but set process count to 1 on step 3.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Localized. Only as risky as bug 1382649.
[String changes made/needed]: None.

Patch for beta strips the test case change in in-content-new since the feature is disabled on 55.
Attachment #8891168 - Flags: review+
Attachment #8891168 - Flags: approval-mozilla-beta?
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #0)

Noted to verify this bug on Beta branch, in step 3 of the STR in comment 0, you would need to set the process count to 1.

Comment 10

27 days ago
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9bba40accbf6
Don't set process count for user on Preferences page load, r=jaws
Backed out for frequently exceeding maximum run time for browser/components/preferences/in-content/tests/browser_performance.js:

https://hg.mozilla.org/integration/autoland/rev/09a412e3cc3b1a1fd9a1325e161d36b91018ef24

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9bba40accbf61384715d963df18e05227676591f&filter-searchStr=533f3b0d33f8841996a16249ba630067c1ca4719
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=118816229&repo=autoland
[task 2017-07-28T05:07:15.506613Z] 05:07:15     INFO - Entering test bound 
[task 2017-07-28T05:07:15.511240Z] 05:07:15     INFO - Buffered messages logged at 05:07:12
[task 2017-07-28T05:07:15.513203Z] 05:07:15     INFO - TEST-PASS | browser/components/preferences/in-content-new/tests/browser_performance.js | General pane was selected - 
[task 2017-07-28T05:07:15.515213Z] 05:07:15     INFO - TEST-PASS | browser/components/preferences/in-content-new/tests/browser_performance.js | performance settings section should be shown - 
[task 2017-07-28T05:07:15.519100Z] 05:07:15     INFO - TEST-PASS | browser/components/preferences/in-content-new/tests/browser_performance.js | pref value is false - 
[task 2017-07-28T05:07:15.525566Z] 05:07:15     INFO - TEST-PASS | browser/components/preferences/in-content-new/tests/browser_performance.js | checkbox should not be checked - 
[task 2017-07-28T05:07:15.527812Z] 05:07:15     INFO - Buffered messages logged at 05:07:13
[task 2017-07-28T05:07:15.530738Z] 05:07:15     INFO - Leaving test bound 
[task 2017-07-28T05:07:15.532618Z] 05:07:15     INFO - Buffered messages finished
[task 2017-07-28T05:07:15.534833Z] 05:07:15     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content-new/tests/browser_performance.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort. -
Flags: needinfo?(timdream)
Comment hidden (mozreview-request)
New test items moved to another file -- will ask for checked-in when Try confirms added tests works.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f085b2a8e26f01a53ec72d063095138f37f27449
Flags: needinfo?(timdream)
Keywords: checkin-needed

Comment 14

27 days ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/a4d0803e7690
Don't set process count for user on Preferences page load, r=jaws
Keywords: checkin-needed
status-firefox55: --- → unaffected
status-firefox56: --- → affected
This bug is not reproducible on latest Beta 55.0b13.
status-firefox55: unaffected → affected
Sorry, didn't mean to flip that. Tim, should we let this ride 56 given comment 15?
status-firefox55: affected → unaffected
Flags: needinfo?(timdream)

Comment 17

26 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a4d0803e7690
Status: ASSIGNED → RESOLVED
Last Resolved: 26 days ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Duplicate of this bug: 1385250
I am pretty sure it's reproducible... Did you check comment 9? The STR is slightly different, and it just a reduced steps from bug 1382649 comment 33. which you have reproduced.
Flags: needinfo?(timdream) → needinfo?(brindusa.tot)
Yes, you are right! With scenario from comment 9, it is reproducible on Beta 55.0b13. Sorry for the misunderstanding.
status-firefox55: unaffected → affected
Flags: needinfo?(brindusa.tot)
Comment on attachment 8891168 [details] [diff] [review]
beta.patch

followup fix for e10s-multi pref ui, beta55+

should be in 55.0 rc1
Attachment #8891168 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 22

24 days ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/f93965089c0a
status-firefox55: affected → fixed
Flags: in-testsuite+
Verified as fixed with latest Nightly 56.0a1, build ID 20170730100307 on Windows 10, Ubuntu 16.04 and Mac OS X 10.12.
status-firefox56: fixed → verified
tracking-firefox55: ? → -
Verified as fixed with the Release Candidate build 55.0, Build ID 20170731093223, on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
You need to log in before you can comment on or make changes to this bug.