1360846 - Permaorange in browser_performance.js when Gecko 55 merges to beta on 2017-06-12

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Phil Ringnalda (:philor)]

Apparently at least one, if not more, of the things that browser_performance.js tests changes when we hit beta, so when we do it's going to fail like https://treeherder.mozilla.org/logviewer.html#?job_id=95479171&repo=try (ignore the first two failures, those are bug 1358381).

You should be able to test locally by just editing /config/milestone.txt and changing the 55.0a1 to 55.0

Comment 1

[Phil Ringnalda (:philor)]

[Tracking Requested - why for this release]: merge bustage, closed tree, delayed b1.

Comment 2

[Marcia Knous [:marcia]]

Tracking 55+ for this permaorange.

Comment 3

[Evan Tseng [:evanxd]]

Looks like this is a timing issue, the tests were running before the prefs were set. This changes[1] should fix this. The treeherder tasks[2] are running now. Let's wait for the result.

[1]: https://hg.mozilla.org/try/rev/8f9b58faf880a6193fcccf3c0f8a002616da6f48
[2]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=babd6b57444c693648f49c40ac0386fa53358800

Comment 4

[Evan Tseng [:evanxd]]

Attachment: Backed out changeset b8c879365f51

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

Comment 5

[Evan Tseng [:evanxd]]

Comment on attachment 8863608 [details]
Backed out changeset b8c879365f51

https://reviewboard.mozilla.org/r/135390/#review138312

::: browser/components/preferences/in-content/tests/browser_performance.js:1
(Diff revision 1)
> -SpecialPowers.pushPrefEnv({set: [
> +add_task(function*() {

Ensure the prefs are set before run the test. We do same thing at [1]. And the `browser_performance.js` test are stable now. It is passed over 10 times continually in the treeherder task[2].

[1]: http://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/tests/browser_search_within_preferences.js#18-21
[2]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=babd6b57444c693648f49c40ac0386fa53358800&selectedJob=95822775

Comment 6

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

Comment on attachment 8863608 [details]
Backed out changeset b8c879365f51

https://reviewboard.mozilla.org/r/135390/#review138506

Yes, good find! Thanks!

Comment 7

[Evan Tseng [:evanxd]]

Thanks for reviewing, Jared. Let's land the fix.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8c879365f51
Ensure prefs are set before run the browser_performance.js tests, r=jaws

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 10

[Phil Ringnalda (:philor)]

Unfortunately, it didn't work, https://treeherder.mozilla.org/logviewer.html#?job_id=96460567&repo=try

It's not intermittent, you shouldn't need ten runs to tell you anything, only one. But it's also not on the trunk, it's on the trunk as though it had already been merged to beta. If your try push isn't changing config/milestone.txt to 55.0, and rebuilding with it changed, then it isn't testing the conditions under which this permaorange happens.

Comment 11

[Evan Tseng [:evanxd]]

(In reply to Phil Ringnalda (:philor) from comment #10)
> Unfortunately, it didn't work,
> https://treeherder.mozilla.org/logviewer.html#?job_id=96460567&repo=try
> 
> It's not intermittent, you shouldn't need ten runs to tell you anything,
> only one. But it's also not on the trunk, it's on the trunk as though it had
> already been merged to beta. If your try push isn't changing
> config/milestone.txt to 55.0, and rebuilding with it changed, then it isn't
> testing the conditions under which this permaorange happens.

Thanks for explaination, Phil.

Changed `config/milestone.txt` to 55.0 and pushed a new try[1] to debug.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4260b712d9224b3a79840a0ea45d2d7c0ba6d41c

Comment 12

[Evan Tseng [:evanxd]]

Hi Phil,

I cannot reproduce the test failures locally or on treeherder by changing `config/milestone.txt` to `55.0`.

On the treeherder[1], the mochitest jobs always said `application timed out after 330 seconds with no output`, and same thing is in local.

Do you know what is the problem for this? Or we also need to change other files?

Thanks.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4260b712d9224b3a79840a0ea45d2d7c0ba6d41c

Comment 13

[Phil Ringnalda (:philor)]

I've never used --artifact on try, but that would certainly be my first suspicion, that trying to do artifact builds while changing that file which changes several defines which change a great deal of behavior is completely broken. The full set of changes I push to try on top of is https://hg.mozilla.org/try/rev/7f6fa826360bc5c8b2f3c734ff73e08805107a61 and https://hg.mozilla.org/try/rev/94e25caa7eaed140e128799fff388f7bdc0aa2fc but I'd be surprised if you do need them, rather than just needing a full build.

Comment 14

[Evan Tseng [:evanxd]]

(In reply to Phil Ringnalda (:philor) from comment #13)
> I've never used --artifact on try, but that would certainly be my first
> suspicion, that trying to do artifact builds while changing that file which
> changes several defines which change a great deal of behavior is completely
> broken. The full set of changes I push to try on top of is
> https://hg.mozilla.org/try/rev/7f6fa826360bc5c8b2f3c734ff73e08805107a61 and
> https://hg.mozilla.org/try/rev/94e25caa7eaed140e128799fff388f7bdc0aa2fc but
> I'd be surprised if you do need them, rather than just needing a full build.

Thanks, Phil. The explanation helps a lot.

Finally, I found the root cause. My assumption of about:config prefs is not correct. I updated the test, and it could be passed on 55.0 locally.

The below two treeherder pages are for the patches in 55.0 and 55.0a1.

55.0a1: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bdf83cee5882945e388199fc51f213d4a6ff5c2
55.0: https://treeherder.mozilla.org/#/jobs?repo=try&revision=24f7ee41b98e93a52f6a57a057ac759e57bc8c20

I'll backout the previous commit[1] and add the commit fixed the test failures, then send review request later.

[1]: https://hg.mozilla.org/mozilla-central/rev/b8c879365f51

Comment 15

[Evan Tseng [:evanxd]]

The change (on Comment 14) cannot work on the try server, but it works locally. I found out it can be passed if we only run the `browser_performance.js` test. But it will be failed if we run all Preferences test files (`browser/components/preferences/in-content/tests/`). So looks like somehow tests influence each other. I will continue to find out the root cause of why they influence each other.

Comment 16

[Evan Tseng [:evanxd]]

Attachment: backout-bug-1360846.patch

Hi Phil,

Since I found out some clues (Comment 15) fix the issue, could you help back out the patch[1] we've landed previously? And the attachment is the back out patch. Then we could land correct fixing patch into our code base.

Thanks.

[1]: https://hg.mozilla.org/mozilla-central/rev/b8c879365f51

Comment 17

[Evan Tseng [:evanxd]]

I think I finally fix this. The patch[1] can make the test be passed when run all Preferences test files. Let's wait for the treeherder tasks[2], [3] and see if it is also fixed on treeherder.

[1]: https://hg.mozilla.org/try/rev/6d23cb328472eff2115e68889f67565c25aa416e
[2]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddf2ddeccb18108852dde8c80b7e0d2120478659
[3]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae2b88ac641aa9f625804ba7ada16374abb024f7

Comment 18

[Evan Tseng [:evanxd]]

Nice, the test[1] is passed on Windows Platform on treeherder. I'll send review request later.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae2b88ac641aa9f625804ba7ada16374abb024f7&selectedJob=97315012

Comment 19

[Evan Tseng [:evanxd]]

Need to back out the patch[1] before I send the review request.

[1]: https://bug1360846.bmoattachments.org/attachment.cgi?id=8864864

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Request for backout (comment 9).

Evan, you can submit mozreview request w/o waiting for backout by explicitly push a set of commits (for example, |git mozreview push HEAD...HEAD^|).

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b86664d108a4f04495c4569b8c46fa4857ac0a0

Comment 22

[Evan Tseng [:evanxd]]

Attachment: Bug 1360846 - Make the prefs restore to default value before run the tests.

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

Comment 23

[Evan Tseng [:evanxd]]

Comment on attachment 8863608 [details]
Backed out changeset b8c879365f51

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

Comment 24

[Evan Tseng [:evanxd]]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #20)
> Request for backout (comment 9).
> 
> Evan, you can submit mozreview request w/o waiting for backout by explicitly
> push a set of commits (for example, |git mozreview push HEAD...HEAD^|).

Thanks for the tip, Tim. I sent the review request.

Comment 25

[Evan Tseng [:evanxd]]

Hi Jared,

I updated the `browser_performance.js` test to let it be passed on 55.0 and 55.0a1. And the test is already passed, see it on treeherder tasks.

55.0a1: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddf2ddeccb18108852dde8c80b7e0d2120478659
55.0: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae2b88ac641aa9f625804ba7ada16374abb024f7

Could you help review the patch? Thanks.

Comment 26

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

Comment on attachment 8865605 [details]
Bug 1360846 - Make the prefs restore to default value before run the tests.

https://reviewboard.mozilla.org/r/137230/#review140600

::: browser/components/preferences/in-content/tests/browser_performance.js:2
(Diff revision 1)
> -  ["browser.preferences.defaultPerformanceSettings.enabled", true],
> -  ["dom.ipc.processCount", 4],
> -  ["layers.acceleration.disabled", false],
> +Services.prefs.clearUserPref("layers.acceleration.disabled");
> +Services.prefs.clearUserPref("dom.ipc.processCount");
> +const DEFAULT_HW_ACCEL_PREF = Services.prefs.getBoolPref("layers.acceleration.disabled");

Please continue to use pushPrefEnv for this so that the pref values (if they are modified) will go back to their modified values after this test.

You can use pushPrefEnv to set these to their default value. Just get the default pref branch and use the default values when calling pushPrefEnv.

Use http://searchfox.org/mozilla-central/rev/224cc663d54085994a4871ef464b7662e0721e83/modules/libpref/nsIPrefService.idl#121 to get the default values.

Comment 27

[Evan Tseng [:evanxd]]

Updated the patch[1] for review comments. Will send review request again after the 55.0[2] and 55.0a1[3] treeherder tasks are good.

[1]: https://hg.mozilla.org/try/rev/57ea94044dccf9992c30629229aec21c6a90a2fd
[2]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=67d483a3a8c550ecf74f7cc20121a4b1e7ed954f
[3]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=15d833d2f78eef3cf7986c6eaba1296eca1d9cd8

Comment 28

[Evan Tseng [:evanxd]]

Comment on attachment 8865605 [details]
Bug 1360846 - Make the prefs restore to default value before run the tests.

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

Comment 29

[Evan Tseng [:evanxd]]

Comment on attachment 8865605 [details]
Bug 1360846 - Make the prefs restore to default value before run the tests.

https://reviewboard.mozilla.org/r/137230/#review140600

> Please continue to use pushPrefEnv for this so that the pref values (if they are modified) will go back to their modified values after this test.
> 
> You can use pushPrefEnv to set these to their default value. Just get the default pref branch and use the default values when calling pushPrefEnv.
> 
> Use http://searchfox.org/mozilla-central/rev/224cc663d54085994a4871ef464b7662e0721e83/modules/libpref/nsIPrefService.idl#121 to get the default values.

Thanks Jared. Learned. Updated the patch for the comment.

Comment 30

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

Comment on attachment 8865605 [details]
Bug 1360846 - Make the prefs restore to default value before run the tests.

https://reviewboard.mozilla.org/r/137230/#review140690

r=me with the following two changes.

::: browser/components/preferences/in-content/tests/browser_performance.js:35
(Diff revision 2)
>    ok(!useRecommendedPerformanceSettings.checked, "checkbox should not be checked after clicking on checkbox");
>  
>    let allowHWAccel = doc.querySelector("#allowHWAccel");
> -  is(Services.prefs.getBoolPref("layers.acceleration.disabled"), false,
> -    "pref value should be false before clicking on checkbox");
> +  is(Services.prefs.getBoolPref("layers.acceleration.disabled"), DEFAULT_HW_ACCEL_PREF,
> +    "pref value should be the default value before clicking on checkbox");
>    ok(allowHWAccel.checked, "checkbox should be checked");

This will fail on systems where layers.acceleration.disabled is by default set to `true`. Please change this to:
> is(allowHWAccel.checked, !DEFAULT_HW_ACCEL_PREF, "checkbox should show the invert of the default value");

::: config/milestone.txt:13
(Diff revision 2)
>  # Referenced by milestone.py.
>  # Hopefully I'll be able to automate replacement of *all*
>  # hardcoded milestones in the tree from these two files.
>  #--------------------------------------------------------
>  
> -55.0a1
> +55.0

This change needs to be reverted.

Comment 31

[Evan Tseng [:evanxd]]

Comment on attachment 8865605 [details]
Bug 1360846 - Make the prefs restore to default value before run the tests.

https://reviewboard.mozilla.org/r/137230/#review140690

> This will fail on systems where layers.acceleration.disabled is by default set to `true`. Please change this to:
> > is(allowHWAccel.checked, !DEFAULT_HW_ACCEL_PREF, "checkbox should show the invert of the default value");

Good point. Let's update it.

> This change needs to be reverted.

Sure.

Comment 32

[Evan Tseng [:evanxd]]

Updated patch for the review comments and got r+. Let's the patch after the 55.0[1] and 55.0a1[2] treeherder tasks are all good.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eca816518346ffeef163b0ec018312a8fbd626a7
[2]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=39b4d94002e7a7ef677c6e16950660fdb45cd528

Comment 33

[Evan Tseng [:evanxd]]

Comment on attachment 8865605 [details]
Bug 1360846 - Make the prefs restore to default value before run the tests.

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

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

The merged backout: https://hg.mozilla.org/mozilla-central/rev/8b86664d108a

Comment 35

[Evan Tseng [:evanxd]]

The treeherder tasks for 55.0 and 55.0a1 are good (Comment 32). Let's land patch.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e7840392d2e
Make the prefs restore to default value before run the tests. r=jaws

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8e7840392d2e