Closed Bug 1525856 Opened 8 months ago Closed 8 months ago

Enable commented out tests for exceptions in browser_trackingUI_fingerprinters.js and browser_trackingUI_cryptominers.js

Categories

(Firefox :: Site Identity, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 67
Tracking Status
firefox67 --- fixed

People

(Reporter: johannh, Assigned: ehsan)

References

(Blocks 1 open bug)

Details

(Whiteboard: [anti-tracking])

Attachments

(1 file)

Some tests are commented out in these files because we need to fix bug 1525458 first.

Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a27b5825884b
Enable the disabled portions of browser_trackingUI_fingerprinters.js and browser_trackingUI_cryptominers.js now that we correctly deal with channels in the face of the content blocking allow list; r=johannh
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ef8953aa920d
Backed out changeset a27b5825884b as per eakhgari request. CLOSED TREE
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0507c090702b
Enable the disabled portions of browser_trackingUI_fingerprinters.js and browser_trackingUI_cryptominers.js now that we correctly deal with channels in the face of the content blocking allow list; r=johannh
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
Status: RESOLVED → REOPENED
Flags: needinfo?(ehsan)
Resolution: FIXED → ---
Target Milestone: Firefox 67 → ---

I don't understand this backout. Considering that this did not fail on integration, why did you not file a follow-up bug?

Flags: needinfo?(dluca)

I had to backout this revision because it was causing perma DT4 failures, like so : https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=513ac146d20192afc26d01dacaec1986f3e23b88&selectedJob=231348096

The issue went unnoticed for almost a day because the test for these failures is rare.

Flags: needinfo?(dluca)

How are the test failures in the devtools test related to the test changes made in this patch? I see zero evidence of any such connection. In fact, it is impossible for there to be any such connection, since the patch that landed here makes changes to tests that run in a different suite, so there is zero chance of the changes here to have made any impact whatsoever to the test that fails intermittently here.

You may ask yourself, could the failure in https://searchfox.org/mozilla-central/source/devtools/client/webconsole/test/mochitest/browser_webconsole_trackingprotection_errors.js have anything to do with bug 1525458 which also landed together with this patch? Looking at that test, the answer is no, since that test doesn't exercise the feature that this bug made changes to (url classifier notifications when the top-level page is on the content blocking allow list -- the test doesn't use the allow list at all.) So again no evidence for a connection there.

Looking at the log of the test, has this test been modified recently? Yes, it was modified just a couple of weeks ago in bug 1514824.

Looking at all of the available evidence, it seems like what happened was the test had "tracking protection" in its name, and it had an intermittent failure bug in it, and instead of a bug being filed for it and the author of the test being asked to look at it, the latest patch that made a change to an unrelated test with "tracking" in its name got backed out. Based on this, I'm going to reland my patch and ask you to file a bug on this test failure and talk to Nicolas about looking at it. Thanks!

Flags: needinfo?(ehsan)
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/846b0c8b048e
Enable the disabled portions of browser_trackingUI_fingerprinters.js and browser_trackingUI_cryptominers.js now that we correctly deal with channels in the face of the content blocking allow list; r=johannh

(In reply to :Ehsan Akhgari from comment #9)

How are the test failures in the devtools test related to the test changes made in this patch? I see zero evidence of any such connection. In fact, it is impossible for there to be any such connection, since the patch that landed here makes changes to tests that run in a different suite, so there is zero chance of the changes here to have made any impact whatsoever to the test that fails intermittently here.

This is an issue how the backout was performed, maybe the first revision was still in the clipboad when then end of the range which shall be backed out got pasted.

You may ask yourself, could the failure in https://searchfox.org/mozilla-central/source/devtools/client/webconsole/test/mochitest/browser_webconsole_trackingprotection_errors.js have anything to do with bug 1525458 which also landed together with this patch? Looking at that test, the answer is no, since that test doesn't exercise the feature that this bug made changes to (url classifier notifications when the top-level page is on the content blocking allow list -- the test doesn't use the allow list at all.) So again no evidence for a connection there.

When bug 1525458 landed for the first time, it caused this test to fail: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&tochange=66ba790db6ff5210b1429173732a5816b39849d2&fromchange=d7d6b249119a5c8a8b179407f801152f09acb0b3&searchStr=devtools%2Clinux%2Copt&selectedJob=231055048 That's the first failure when this test started to fail frequently.

Looking at the log of the test, has this test been modified recently? Yes, it was modified just a couple of weeks ago in bug 1514824.

Frequent fails (except for edge cases) aren't triggered by a change weeks after it landed. It was correct to look for a recent change.

Looking at all of the available evidence, it seems like what happened was the test had "tracking protection" in its name, and it had an intermittent failure bug in it, and instead of a bug being filed for it and the author of the test being asked to look at it, the latest patch that made a change to an unrelated test with "tracking" in its name got backed out. Based on this, I'm going to reland my patch and ask you to file a bug on this test failure and talk to Nicolas about looking at it. Thanks!

I will remind the sheriffs to talk to the developers if they believe to have found the change which causes a frequent failure.

Please be aware that the code sheriffs are not coding and thus are searching the culprits of failures by searching for test and folder names and doing backfills and retriggers.

(In reply to Sebastian Hengst [:aryx] (needinfo on intermittent or backout) from comment #11)

(In reply to :Ehsan Akhgari from comment #9)

How are the test failures in the devtools test related to the test changes made in this patch? I see zero evidence of any such connection. In fact, it is impossible for there to be any such connection, since the patch that landed here makes changes to tests that run in a different suite, so there is zero chance of the changes here to have made any impact whatsoever to the test that fails intermittently here.

This is an issue how the backout was performed, maybe the first revision was still in the clipboad when then end of the range which shall be backed out got pasted.

Not sure what you mean, sorry? I'm not too familiar with the backout procedure so I can't decode the sentence.

You may ask yourself, could the failure in https://searchfox.org/mozilla-central/source/devtools/client/webconsole/test/mochitest/browser_webconsole_trackingprotection_errors.js have anything to do with bug 1525458 which also landed together with this patch? Looking at that test, the answer is no, since that test doesn't exercise the feature that this bug made changes to (url classifier notifications when the top-level page is on the content blocking allow list -- the test doesn't use the allow list at all.) So again no evidence for a connection there.

When bug 1525458 landed for the first time, it caused this test to fail: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&tochange=66ba790db6ff5210b1429173732a5816b39849d2&fromchange=d7d6b249119a5c8a8b179407f801152f09acb0b3&searchStr=devtools%2Clinux%2Copt&selectedJob=231055048 That's the first failure when this test started to fail frequently.

Of course it did, that first landing had a typo (https://bugzilla.mozilla.org/show_bug.cgi?id=1525458#c15) which completely disabled our tracking protection feature, so that test plus any other test in the tree which examined the TP feature would fail. But if you look at the failure message, it is completely different than what this was backed out for, since the second landing of bug 1525458 didn't have the typo. :-)

Looking at the log of the test, has this test been modified recently? Yes, it was modified just a couple of weeks ago in bug 1514824.

Frequent fails (except for edge cases) aren't triggered by a change weeks after it landed. It was correct to look for a recent change.

You are right, perhaps my theory isn't a great one. But that just suggests the culprit is something else based on the fact that a test change in one test suite cannot break a test in another test suite.

I certainly agree that looking for a recent change was the correct thing to do, no debate there!

Looking at all of the available evidence, it seems like what happened was the test had "tracking protection" in its name, and it had an intermittent failure bug in it, and instead of a bug being filed for it and the author of the test being asked to look at it, the latest patch that made a change to an unrelated test with "tracking" in its name got backed out. Based on this, I'm going to reland my patch and ask you to file a bug on this test failure and talk to Nicolas about looking at it. Thanks!

I will remind the sheriffs to talk to the developers if they believe to have found the change which causes a frequent failure.

Please be aware that the code sheriffs are not coding and thus are searching the culprits of failures by searching for test and folder names and doing backfills and retriggers.

Very fair, and thanks! This is indeed tricky to figure out, and I was only explaining the reason why I was certain the backout patch was innocent to demonstrate how I think about the issue when I saw it, in the hopes that some of that would be useful to sheriffs in the future when making other future tricky calls of this nature.

But of course this isn't a pure science so there will be misidentified regressors and that's part of the process. :-) I do appreciate the hard effort that the sheriffs spend in keeping our trees healthy. In a long long past I also used to spend some time sheriffing trees myself and I would get things wrong quite a few times in similar situations, so I definitely understand how hard this is from the other side, and I guess the last thing I'll say here is that our sheriffs are the force that keeps our development ship afloat on a day to day basis, so kudos for all of the hard work and thanks a lot for listening to the occasional complaints from folks like myself, please don't forget that we really appreciate all your hard work!

Status: REOPENED → RESOLVED
Closed: 8 months ago8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67

@Nicolas can you please take a look at these failures and see if you can fix them? If you cannot find a fix then please let us know so that we should disable it. At first it looks that they are memory issues so a fix would be more preferable.

https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=windows%2C7%2Cpgo%2Cmochitests%2Cwith%2Ce10s%2Ctest-windows7-32-pgo%2Fopt-mochitest-devtools-chrome-e10s-7%2Cm-e10s%28dt7%29&fromchange=0c4c0810b93fa8fb8c8f1439b78ced96ba91450f&selectedJob=231892561

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=513ac146d20192afc26d01dacaec1986f3e23b88&selectedJob=231348096

It seems that these failures are jumping chunks, from dt4 and now it is on dt7 please see the above links.
This bug could also be related https://bugzilla.mozilla.org/show_bug.cgi?id=1514824

Thanks.

Flags: needinfo?(nchevobbe)

I'll investigate in Bug 1528503

Flags: needinfo?(nchevobbe)
You need to log in before you can comment on or make changes to this bug.