Closed Bug 1384962 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Preferences, defect, P1)

Product:
Firefox
Component:
Preferences
Version:
55 Branch
Platform:
All
Windows 10
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox55 - verified
firefox56 --- verified

People

(Reporter: timdream, Assigned: timdream)

References

Details

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

Attachments

(2 files)

Bug 1384962 - Don't set process count for user on Preferences page load,
59 bytes, text/x-review-board-request
jaws
: review+
Details
beta.patch
12.08 KB, patch
timdream
: review+
jcristau
: approval-mozilla-beta+
Details | Diff | Splinter Review 
+++ 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.
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 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.
Attached patch beta.patch — — Splinter Review 
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.
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)
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)
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
This bug is not reproducible on latest Beta 55.0b13.
Sorry, didn't mean to flip that. Tim, should we let this ride 56 given comment 15?
status-firefox55: affected → unaffected
Flags: needinfo?(timdream)
https://hg.mozilla.org/mozilla-central/rev/a4d0803e7690
Status: ASSIGNED → RESOLVED
Closed: 4 years 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+
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
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
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.