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

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Preferences
P1
normal
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: philor, Assigned: evanxd)

Tracking

({regression})

55 Branch
Firefox 55
regression
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55+ fixed)

Details

(Whiteboard: [photon-preference] )

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
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
(Reporter)

Updated

a year ago
Flags: needinfo?(evan)
(Reporter)

Comment 1

a year ago
[Tracking Requested - why for this release]: merge bustage, closed tree, delayed b1.
tracking-firefox55: --- → ?
Whiteboard: [photon-preference]
Target Milestone: --- → Firefox 55
Tracking 55+ for this permaorange.
tracking-firefox55: ? → +
(Assignee)

Comment 3

a year ago
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
Assignee: nobody → evan
Flags: needinfo?(evan)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

a year ago
mozreview-review
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

Updated

a year ago
Status: NEW → ASSIGNED

Comment 6

a year ago
mozreview-review
Comment on attachment 8863608 [details]
Backed out changeset b8c879365f51

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

Yes, good find! Thanks!
Attachment #8863608 - Flags: review?(jaws) → review+
(Assignee)

Comment 7

a year ago
Thanks for reviewing, Jared. Let's land the fix.
Keywords: checkin-needed

Comment 8

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b8c879365f51
Ensure prefs are set before run the browser_performance.js tests, r=jaws
Keywords: checkin-needed

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b8c879365f51
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
(Reporter)

Comment 10

a year ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

a year ago
Priority: -- → P1
(Assignee)

Comment 11

a year ago
(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
(Assignee)

Comment 12

a year ago
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
Flags: needinfo?(philringnalda)
(Reporter)

Comment 13

a year ago
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.
Flags: needinfo?(philringnalda)
(Assignee)

Comment 14

a year ago
(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
(Assignee)

Comment 15

a year ago
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.
(Assignee)

Comment 16

a year ago
Created attachment 8864864 [details] [diff] [review]
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
Flags: needinfo?(philringnalda)
(Assignee)

Comment 17

a year ago
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
(Assignee)

Comment 18

a year ago
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
(Assignee)

Updated

a year ago
Attachment #8863608 - Attachment is obsolete: true
(Assignee)

Comment 19

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

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

Updated

a year ago
Status: REOPENED → ASSIGNED
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^|).
Flags: needinfo?(evan)
Keywords: checkin-needed
Backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b86664d108a4f04495c4569b8c46fa4857ac0a0
status-firefox55: fixed → affected
Flags: needinfo?(philringnalda)
Keywords: checkin-needed
Target Milestone: Firefox 55 → ---
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8865605 - Flags: review?(jaws)
(Assignee)

Comment 24

a year ago
(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.
Flags: needinfo?(evan)
(Assignee)

Comment 25

a year ago
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

a year ago
mozreview-review
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.
Attachment #8865605 - Flags: review?(jaws) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 29

a year ago
mozreview-review-reply
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

a year ago
mozreview-review
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.
Attachment #8865605 - Flags: review?(jaws) → review+
(Assignee)

Comment 31

a year ago
mozreview-review-reply
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.
(Assignee)

Comment 32

a year ago
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 hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8863608 - Attachment is obsolete: true
(Assignee)

Comment 35

a year ago
The treeherder tasks for 55.0 and 55.0a1 are good (Comment 32). Let's land patch.
Keywords: checkin-needed

Comment 36

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e7840392d2e
Make the prefs restore to default value before run the tests. r=jaws
Keywords: checkin-needed

Comment 37

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8e7840392d2e
Status: ASSIGNED → RESOLVED
Last Resolved: a year agoa year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

9 months ago
Flags: qe-verify?
Flags: qe-verify? → qe-verify-
You need to log in before you can comment on or make changes to this bug.