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

Status: VERIFIED FIXED

Description

[Tim Guan-tin Chien [:timdream] (please needinfo; no longer work for MoCo)]

+++ 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 1

[Tim Guan-tin Chien [:timdream] (please needinfo; no longer work for MoCo)]

Attachment: Bug 1384962 - Don't set process count for user on Preferences page load,

This is a follow-up to incompete fix in bug 1382649. In that bug, I attempted to
set the process count value to the e10s rollout value, when the user unchecks
"Use default performance settings". The same function updatePerformanceSettingsBox()
is actually run during page load, resulting the user set value being changed to
e10s rollout value if the user decided to use the default process count.

Tests are written for both patches.

Review commit: https://reviewboard.mozilla.org/r/162110/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162110/

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo; no longer work for MoCo)]

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.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

I think you linked to the wrong bug in comment #2? That bug is fixed.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

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});

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo; no longer work for MoCo)]

(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 6

[Tim Guan-tin Chien [:timdream] (please needinfo; no longer work for MoCo)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162110/diff/1-2/

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo; no longer work for MoCo)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162110/diff/2-3/

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo; no longer work for MoCo)]

Attachment: 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.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo; no longer work for MoCo)]

(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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9bba40accbf6
Don't set process count for user on Preferences page load, r=jaws

Comment 11

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

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. -

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo; no longer work for MoCo)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162110/diff/3-4/

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo; no longer work for MoCo)]

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a4d0803e7690
Don't set process count for user on Preferences page load, r=jaws

Comment 15

[Brindusa Tot[:brindusat]]

This bug is not reproducible on latest Beta 55.0b13.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Sorry, didn't mean to flip that. Tim, should we let this ride 56 given comment 15?

Comment 17

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/a4d0803e7690

Comment 19

[Tim Guan-tin Chien [:timdream] (please needinfo; no longer work for MoCo)]

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.

Comment 20

[Brindusa Tot[:brindusat]]

Yes, you are right! With scenario from comment 9, it is reproducible on Beta 55.0b13. Sorry for the misunderstanding.

Comment 21

[Julien Cristau [:jcristau]]

Comment on attachment 8891168 [details] [diff] [review]
beta.patch

followup fix for e10s-multi pref ui, beta55+

should be in 55.0 rc1

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f93965089c0a

Comment 23

[Brindusa Tot[:brindusat]]

Verified as fixed with latest Nightly 56.0a1, build ID 20170730100307 on Windows 10, Ubuntu 16.04 and Mac OS X 10.12.

Comment 24

[Brindusa Tot[:brindusat]]

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.