Closed Bug 1493682 Opened 6 years ago Closed 6 years ago

Perma multiple failures in toolkit/components/antitracking/test/browser/browser_blockingCookies.js when Gecko 64 merges to Beta on 2018-10-15

Categories

(Firefox :: Protections UI, defect, P1)

Product:
Firefox
Component:
Protections UI
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 64
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 + fixed
firefox64 + verified

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

(Keywords: regression)

Attachments

(3 files)

Bug 1493682 - Part 1: Introduce two new prefs for controlling whether the content blocking allow list would be honoured
46 bytes, text/x-phabricator-request
baku
: review+
pascalc
: approval-mozilla-beta+
Details | Review
Bug 1493682 - Part 2: Fix the image cache tests
46 bytes, text/x-phabricator-request
baku
: review+
Details | Review
Bug 1493682 - Part 3: Add tests for the new prefs
46 bytes, text/x-phabricator-request
baku
: review+
Details | Review
From bug 1493148 comment 5: There is a failure missing from the first comment: https://treeherder.mozilla.org/logviewer.html#?job_id=200985482&repo=try&lineNumber=4223 13:29:08 INFO - TEST-START | toolkit/components/antitracking/test/browser/browser_blockingCookies.js 13:29:08 INFO - TEST-INFO | started process screenshot 13:29:08 INFO - TEST-INFO | screenshot: exit 0 13:29:08 INFO - Buffered messages logged at 13:29:08 13:29:08 INFO - Entering test bound 13:29:08 INFO - Starting blocking cookieBehavior (4) and blocking contentBlocking and contentBlocking UI and contentBlocking third-party cookies UI without allow list test Set/Get Cookies running in a normal window with iframe sandbox set to null 13:29:08 INFO - Creating a new tab 13:29:08 INFO - Creating a 3rd party content 13:29:08 INFO - Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "http://example.net/browser/toolkit/components/antitracking/test/browser/page.html" line: 0}] 13:29:08 INFO - Sending code to the 3rd party content 13:29:08 INFO - TEST-PASS | toolkit/components/antitracking/test/browser/browser_blockingCookies.js | No cookies for me - true == true - 13:29:08 INFO - Buffered messages finished 13:29:08 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/antitracking/test/browser/browser_blockingCookies.js | No cookies for me - false == true - 13:29:08 INFO - Stack trace: 13:29:08 INFO - resource://testing-common/content-task.js line 59 > eval:msg:22 13:29:08 INFO - Not taking screenshot here: see the one that was previously logged 13:29:08 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/antitracking/test/browser/browser_blockingCookies.js | We should not have cookies - false == true - 13:29:08 INFO - Stack trace: 13:29:08 INFO - resource://testing-common/content-task.js line 59 > eval:msg:22 13:29:08 INFO - Not taking screenshot here: see the one that was previously logged 13:29:08 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/antitracking/test/browser/browser_blockingCookies.js | We should not have cookies - false == true - 13:29:08 INFO - Stack trace: 13:29:08 INFO - resource://testing-common/content-task.js line 59 > eval:msg:22 13:29:08 INFO - Not taking screenshot here: see the one that was previously logged 13:29:08 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/antitracking/test/browser/browser_blockingCookies.js | Still no cookies for me - false == true - 13:29:08 INFO - Stack trace: 13:29:08 INFO - resource://testing-common/content-task.js line 59 > eval:msg:22 13:29:08 INFO - Not taking screenshot here: see the one that was previously logged 13:29:08 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/antitracking/test/browser/browser_blockingCookies.js | Checking cookie blocking notifications - Got false, expected true 13:29:08 INFO - Stack trace: 13:29:08 INFO - chrome://mochikit/content/browser-test.js:test_is:1304 13:29:08 INFO - chrome://mochitests/content/browser/toolkit/components/antitracking/test/browser/head.js:_createTask/<:519 13:29:08 INFO - Removing the tab 13:29:08 INFO - Leaving test bound 13:29:08 INFO - Entering test bound 13:29:08 INFO - Cleaning up. 13:29:08 INFO - Leaving test bound 13:29:08 INFO - Entering test bound 13:29:08 INFO - Starting non-blocking cookieBehavior (0) and blocking contentBlocking and contentBlocking UI and contentBlocking third-party cookies UI without allow list test Set/Get Cookies running in a normal window with iframe sandbox set to null 13:29:09 INFO - GECKO(1700) | JavaScript error: resource://gre/modules/WebProgressChild.jsm, line 58: TypeError: this.mm.content is null; can't access its "document" property 13:29:09 INFO - Console message: [JavaScript Error: "TypeError: this.mm.content is null; can't access its "document" property" {file: "resource://gre/modules/WebProgressChild.jsm" line: 58}] 13:29:09 INFO - Creating a new tab 13:29:09 INFO - Creating a 3rd party content 13:29:09 INFO - Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "http://example.net/browser/toolkit/components/antitracking/test/browser/page.html" line: 0}] 13:29:09 INFO - Sending code to the 3rd party content 13:29:09 INFO - TEST-PASS | toolkit/components/antitracking/test/browser/browser_blockingCookies.js | No cookies for me - true == true - 13:29:09 INFO - TEST-PASS | toolkit/components/antitracking/test/browser/browser_blockingCookies.js | We should not have cookies - true == true - 13:29:09 INFO - TEST-PASS | toolkit/components/antitracking/test/browser/browser_blockingCookies.js | Some cookies for me - true == true - 13:29:09 INFO - TEST-PASS | toolkit/components/antitracking/test/browser/browser_blockingCookies.js | Some cookies for me - true == true - 13:29:09 INFO - TEST-PASS | toolkit/components/antitracking/test/browser/browser_blockingCookies.js | We should have cookies - true == true - 13:29:09 INFO - TEST-PASS | toolkit/components/antitracking/test/browser/browser_blockingCookies.js | Some Cookies for me - true == true - 13:29:09 INFO - TEST-PASS | toolkit/components/antitracking/test/browser/browser_blockingCookies.js | Checking cookie blocking notifications - 13:29:09 INFO - Removing the tab
I think the fix to bug 1491061 was incorrect, based on these test failures. :-( The biggest thing that goes wrong there is that setting browser.contentblocking.ui.enabled to true will disable content blocking. Such a pref hell we have ended up in. Will think of a new approach...
Blocks: 1491061
Tracking for 63 as this is needed for bug 1491061 which got uplifted.
status-firefox63: --- → affected
tracking-firefox63: --- → +
I *think* the right thing to do might be to check browser.contentblocking.rejecttrackers.ui.enabled only at the time we are doing the allow list checks. Coincidentally, that might help fix bug 1493357 without the need for a separate patch there too. Testing a fix right now...
Status: NEW → ASSIGNED
Priority: -- → P1
One slight modification to what I said in comment 3 is that the pref shouldn't be checked when the allow list is being checked for tracking *protection* but it should be checked for tracking *annotations*, since third-party cookie blocking can depend on tracking annotations. This means that if we just use browser.contentblocking.rejecttrackers.ui.enabled, turning third-party cookies UI off in beta would turn off annotations for pages that are on the allow list! Therefore I will introduce two whole new prefs here, so that we can turn the allow list checks for annotations off separately from our storage checks. It is a bit complex but looking at the patches will hopefully make things clearer. This means that if we decide to turn off the third-party cookies UI on beta, we will need to turn browser.contentblocking.rejecttrackers.ui.enabled and browser.contentblocking.allowlist.storage.enabled to false.
The first line which this patch is fixing was clobbering the object that the tests were setting up, so all these tests were testing was the blocking callback of the default runTest() path before this patch. Depends on D6747
The image cache tests didn't follow the previous naming convention in order to make it clearer how the tests are set up using the naming conventions of the files. These tests should be unified into a single file soon. Depends on D6748
Comment on attachment 9011668 [details] Bug 1493682 - Part 3: Add tests for the new prefs Andrea Marchesini [:baku] has approved the revision.
Attachment #9011668 - Flags: review+
Comment on attachment 9011667 [details] Bug 1493682 - Part 2: Fix the image cache tests Andrea Marchesini [:baku] has approved the revision.
Attachment #9011667 - Flags: review+
Comment on attachment 9011666 [details] Bug 1493682 - Part 1: Introduce two new prefs for controlling whether the content blocking allow list would be honoured Andrea Marchesini [:baku] has approved the revision.
Attachment #9011666 - Flags: review+
Pushed by eakhgari@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2b82edb23402 Part 1: Introduce two new prefs for controlling whether the content blocking allow list would be honoured r=baku https://hg.mozilla.org/integration/autoland/rev/f84e7a76ff41 Part 2: Fix the image cache tests r=baku https://hg.mozilla.org/integration/autoland/rev/5bcb9295f09c Part 3: Add tests for the new prefs r=baku
Comment on attachment 9011666 [details] Bug 1493682 - Part 1: Introduce two new prefs for controlling whether the content blocking allow list would be honoured Approval Request Comment [Feature/Bug causing the regression]: This is the rest of the fix for bug 1491061. See https://bugzilla.mozilla.org/show_bug.cgi?id=1491061#c22. [User impact if declined]: See above. [Is this code covered by automated tests?]: Yes. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: No. [Why is the change risky/not risky?]: Code is preffed off by default. [String changes made/needed]: None.
Attachment #9011666 - Flags: approval-mozilla-beta?
Depends on: 1494145
See Also: → 1494145
Comment on attachment 9011666 [details] Bug 1493682 - Part 1: Introduce two new prefs for controlling whether the content blocking allow list would be honoured Tracked for 63 and a P1, uplift approved for 63 beta 10, thanks.
Attachment #9011666 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Backed out from Beta per bug 1491061.
status-firefox63: fixed → affected
Ehsan, is there any progress here?
Flags: needinfo?(ehsan)
(In reply to Andreea Pavel [:apavel] from comment #18) > Ehsan, is there any progress here? See bug 1494737 comment 4. Once that gets approved I'll reland. Hopefully today before going on PTO. If not will post rebased patches on bugs so that you can land them while I'm away (but I really hope to be able to land them myself since there is a large stack of patches to uplift...)
Flags: needinfo?(ehsan)
QA Contact: francois
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: