1451307 - Tracking Protection fails to report blocked requests made in iframes

Status: REOPENED

(Firefox :: Protections UI, defect, P3)

Description

[Kestrel]

Attachment: missing-tp-doorhanger.png

Tracking Protection should work in extension pages just like normal pages, showing an icon in the location bar with the option to disable protection. It is important that it does not break things without feedback or recourse.

For example the "Tab for a Cause" extension replaces the New Tab page with a page that shows ads when signed on. When visited directly it allows disabling TP but when shown in the New Tab page there is no warning and no option.

https://addons.mozilla.org/en-US/firefox/addon/tab-for-a-cause/

https://tab.gladly.io/newtab/

Comment 1

[Kris Maglione [:kmag]]

Johann, Andrew suggested you might have an opinion on how to prioritize this. It doesn't seem like a major concern to me.

Comment 2

[Johann Hofmann [:johannh]]

Yeah, it is a valid bug but not a critical issue IMO. I'm torn between P3 and P5. It would be great if someone could find the time to look into this but I have no idea who that could be.

Comment 3

[François Marier [:francois]]

This is a symptom of a larger problem: we don't run the URL classifier on about:* pages.

Comment 4

[Johann Hofmann [:johannh]]

(In reply to François Marier [:francois] from comment #3)
> This is a symptom of a larger problem: we don't run the URL classifier on
> about:* pages.

I'm not sure what you mean, the WebExtension page isn't an about: page. After spending some time on this I think this is actually a general TP problem with iframes. The Tab for a Cause extension uses a full-window iframe on its new tab page.

nsChannelClassifier::SetBlockedContent is the place where we regularly set TP flags, but in the case of the Tab for a Cause extension the function consistently exited early at https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/netwerk/base/nsChannelClassifier.cpp#851. The document URI here is of course moz-extension://... while the channel load principal has the URI of the iframe. This looks like all iframe requests are categorized as "this isn't the same document anymore" and discarded before we can show the UI (though at least we still block them).

I did a very cheap test on http://joo.crater.uberspace.de/tp/ and you will notice the same issue (open devtools console): the request to the FB tracker is blocked but there's no explicit console warning about TP and no shield.

Comment 5

[François Marier [:francois]]

(In reply to Johann Hofmann [:johannh] from comment #4)
> (In reply to François Marier [:francois] from comment #3)
> > This is a symptom of a larger problem: we don't run the URL classifier on
> > about:* pages.
> 
> I'm not sure what you mean, the WebExtension page isn't an about: page.
> After spending some time on this I think this is actually a general TP
> problem with iframes. The Tab for a Cause extension uses a full-window
> iframe on its new tab page.

Sorry, I misinterpreted what the bug was about. Looking at the screenshot I thought it was the difference between our blocking trackers when they're on an HTTP URL and not blocking them when they're served from about:newtab.

> I did a very cheap test on http://joo.crater.uberspace.de/tp/ and you will
> notice the same issue (open devtools console): the request to the FB tracker
> is blocked but there's no explicit console warning about TP and no shield.

You're right, this is a bigger problem in the channel classifier.

Comment 6

[Johann Hofmann [:johannh]]

I can look into this...

Comment 7

[Johann Hofmann [:johannh]]

Attachment: Bug 1451307 - Part 1 - Consider iframes when comparing loading URIs in nsChannelClassifier::SetBlockedContent.

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

Comment 8

[Johann Hofmann [:johannh]]

Attachment: Bug 1451307 - Part 2 - Move trackingUI tests out of b/b/c/test/general into their own subdirectory.

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

Comment 9

[Johann Hofmann [:johannh]]

Attachment: Bug 1451307 - Part 3 - Add a test for showing the TP UI for iframes and unloaded documents.

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

Comment 10

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=da9b09c9f35ca9b318d9983bd4186491ad06253b

Comment 11

[Johann Hofmann [:johannh]]

These patches are a bit... exploratory. I don't actually know that much about channel classifier or tracking protection but I think this should work.

It would be great if someone could answer my question marked with XXX in patch 1.

Comment 12

[:Gijs (he/him)]

Comment on attachment 8969430 [details]
Bug 1451307 - Part 2 - Move trackingUI tests out of b/b/c/test/general into their own subdirectory.

https://reviewboard.mozilla.org/r/238190/#review243918

I'll do a detailed review tomorrow, but do you think it would be OK to put these in the 'siteIdentity' directory instead? That is kind of the UI element that this is about... and while I'm glad things are moving out of general/ it'd also be good if we didn't end up with 50 different subdirectories each with only a few tests in...

Comment 13

[Johann Hofmann [:johannh]]

(In reply to :Gijs (he/him) from comment #12)
> Comment on attachment 8969430 [details]
> Bug 1451307 - Part 2 - Move trackingUI tests out of b/b/c/test/general into
> their own subdirectory.
> 
> https://reviewboard.mozilla.org/r/238190/#review243918
> 
> I'll do a detailed review tomorrow, but do you think it would be OK to put
> these in the 'siteIdentity' directory instead? That is kind of the UI
> element that this is about... and while I'm glad things are moving out of
> general/ it'd also be good if we didn't end up with 50 different
> subdirectories each with only a few tests in...

Hm, it could be in siteIdentity, though I think trackingUI makes for a pretty decent subset of tests. It's 9 tests so I wouldn't call it a few (in addition to the 19 tests of siteIdentity that's a bunch already). So, personally I wouldn't move it to siteIdentity, but I really don't feel strongly about this either.

Comment 14

[:Gijs (he/him)]

Comment on attachment 8969430 [details]
Bug 1451307 - Part 2 - Move trackingUI tests out of b/b/c/test/general into their own subdirectory.

https://reviewboard.mozilla.org/r/238190/#review244142

I don't see anything wrong here, but the trypush is unhappy with the tour tests (and has some osx debug timeouts for the non-tour trackingUI tests which might be intermittent, but would be good to check - there's just about enough that it looks dodgy). Maybe because of the other csets or something? Or maybe because some other test in bbc/t/general makes some changes that causes the tests to pass more reliably there. Or something.

Also eslint semicolon issues in browser/base/content/test/trackingUI/trackingPage_onunload.html (in the other cset).

Comment 15

[Honza Bambas (:mayhemer)]

Comment on attachment 8969429 [details]
Bug 1451307 - Part 1 - Consider iframes when comparing loading URIs in nsChannelClassifier::SetBlockedContent.

https://reviewboard.mozilla.org/r/238188/#review244170

lgtm, thanks

Comment 16

[François Marier [:francois]]

Comment on attachment 8969429 [details]
Bug 1451307 - Part 1 - Consider iframes when comparing loading URIs in nsChannelClassifier::SetBlockedContent.

https://reviewboard.mozilla.org/r/238188/#review244334

The approach looks good to me. I noted two small things inline.

::: netwerk/base/nsChannelClassifier.cpp:857
(Diff revision 1)
>    // This event might come after the user has navigated to another page.
>    // To prevent showing the TrackingProtection UI on the wrong page, we need to
>    // check that the loading URI for the channel is the same as the URI currently
> -  // loaded in the document.
> -  if (!SameLoadingURI(doc, channel)) {
> +  // loaded in the document that is associated with the channel (which may be
> +  // different than the top-level document, so we avoid ignoring all iframes).
> +  nsCOMPtr<nsPIDOMWindowOuter> win = GetWindowForChannel(channel);

`GetWindowForChannel()` could return `nullptr` so we should check for null here before we start using that pointer.

::: netwerk/base/nsChannelClassifier.cpp:866
(Diff revision 1)
> +  // No frame document? This can happen when the frame itself is from
> +  // a tracker URL. Walk back the parent tree...
> +  while (!frameDoc && win != topWin) {
> +    win = win->GetParent();
> +    frameDoc = win->GetExtantDoc();
> +  }

Are we guaranteed to either get a `frameDoc` or to get right up to the `topWin`?

Is it possible that this could fail and turn into an infinite loop?

Comment 17

[François Marier [:francois]]

Comment on attachment 8969431 [details]
Bug 1451307 - Part 3 - Add a test for showing the TP UI for iframes and unloaded documents.

https://reviewboard.mozilla.org/r/238192/#review244340

::: browser/base/content/test/trackingUI/browser_trackingUI_iframe.js:1
(Diff revision 1)
> +const URL = getRootDirectory(gTestPath).replace("chrome://mochitests/content", "http://example.org") + "trackingPage_iframe.html";

Please add the public domain license header from https://www.mozilla.org/en-US/MPL/headers/

::: browser/base/content/test/trackingUI/browser_trackingUI_unloaded_documents.js:1
(Diff revision 1)
> +const URL = getRootDirectory(gTestPath).replace("chrome://mochitests/content", "http://example.org") + "trackingPage_onunload.html";

Again, the public domain header from https://www.mozilla.org/en-US/MPL/headers/.

::: browser/base/content/test/trackingUI/trackingPage_iframe.html:2
(Diff revision 1)
> +<!DOCTYPE HTML>
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public

I don't particularly care, but we normally use the public domain header in tests.

Comment 18

[Johann Hofmann [:johannh]]

Comment on attachment 8969429 [details]
Bug 1451307 - Part 1 - Consider iframes when comparing loading URIs in nsChannelClassifier::SetBlockedContent.

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

Comment 19

[Johann Hofmann [:johannh]]

Comment on attachment 8969430 [details]
Bug 1451307 - Part 2 - Move trackingUI tests out of b/b/c/test/general into their own subdirectory.

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

Comment 20

[Johann Hofmann [:johannh]]

Comment on attachment 8969431 [details]
Bug 1451307 - Part 3 - Add a test for showing the TP UI for iframes and unloaded documents.

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

Comment 21

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cfa4fe80ccf70f3be11e82155fc12251bdec158

Comment 22

[Johann Hofmann [:johannh]]

Comment on attachment 8969429 [details]
Bug 1451307 - Part 1 - Consider iframes when comparing loading URIs in nsChannelClassifier::SetBlockedContent.

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

Comment 23

[Johann Hofmann [:johannh]]

Comment on attachment 8969430 [details]
Bug 1451307 - Part 2 - Move trackingUI tests out of b/b/c/test/general into their own subdirectory.

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

Comment 24

[Johann Hofmann [:johannh]]

Comment on attachment 8969431 [details]
Bug 1451307 - Part 3 - Add a test for showing the TP UI for iframes and unloaded documents.

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

Comment 25

[Johann Hofmann [:johannh]]

Comment on attachment 8969431 [details]
Bug 1451307 - Part 3 - Add a test for showing the TP UI for iframes and unloaded documents.

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

Comment 26

[Johann Hofmann [:johannh]]

Comment on attachment 8969429 [details]
Bug 1451307 - Part 1 - Consider iframes when comparing loading URIs in nsChannelClassifier::SetBlockedContent.

Honza, there's a bit that changed in this patch now, would you like to take another quick look?

I realized that for iframe document loads we need to always consider the parent document URI (and it doesn't really make sense to walk up the parent chain in that case).

Comment 27

[Honza Bambas (:mayhemer)]

Comment on attachment 8969429 [details]
Bug 1451307 - Part 1 - Consider iframes when comparing loading URIs in nsChannelClassifier::SetBlockedContent.

https://reviewboard.mozilla.org/r/238188/#review244992

Makes much more sense now!  Thanks.

::: netwerk/base/nsChannelClassifier.cpp:869
(Diff revision 3)
> +  // contains a tracking page, the window associated with the channel belongs
> +  // to that iframe, not its loading parent. In this case we actually want to
> +  // compare load URIs with the parent frame, so we step up one level.
> +  nsLoadFlags loadFlags = 0;
> +  channel->GetLoadFlags(&loadFlags);
> +  bool isDocumentLoad = loadFlags & nsIChannel::LOAD_DOCUMENT_URI;

should we use

https://searchfox.org/mozilla-central/rev/8f06c1b9a080b84435a2906e420fe102e1ed780b/netwerk/base/nsIChannel.idl#358

?

::: netwerk/base/nsChannelClassifier.cpp:884
(Diff revision 3)
> +    return NS_OK;
> +  }
> +
> +  // Get the root docshell for updating the security UI.
> +  nsCOMPtr<nsPIDOMWindowOuter> topWin = win->GetScriptableTop();
> +  nsCOMPtr<nsIDocShell> docShell = topWin->GetDocShell();

topDocShell?

Comment 28

[François Marier [:francois]]

Comment on attachment 8969429 [details]
Bug 1451307 - Part 1 - Consider iframes when comparing loading URIs in nsChannelClassifier::SetBlockedContent.

https://reviewboard.mozilla.org/r/238188/#review245048

Comment 29

[Johann Hofmann [:johannh]]

Comment on attachment 8969429 [details]
Bug 1451307 - Part 1 - Consider iframes when comparing loading URIs in nsChannelClassifier::SetBlockedContent.

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

Comment 30

[Johann Hofmann [:johannh]]

Comment on attachment 8969430 [details]
Bug 1451307 - Part 2 - Move trackingUI tests out of b/b/c/test/general into their own subdirectory.

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

Comment 31

[Johann Hofmann [:johannh]]

Comment on attachment 8969431 [details]
Bug 1451307 - Part 3 - Add a test for showing the TP UI for iframes and unloaded documents.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238192/diff/4-5/

Comment 32

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0c363f450ca91826b50a32499d80915e1fe7837

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f89d94d45ba
Part 1 - Consider iframes when comparing loading URIs in nsChannelClassifier::SetBlockedContent. r=francois,mayhemer
https://hg.mozilla.org/integration/autoland/rev/13e58d5887d8
Part 2 - Move trackingUI tests out of b/b/c/test/general into their own subdirectory. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/be0a9b78c224
Part 3 - Add a test for showing the TP UI for iframes and unloaded documents. r=francois

Comment 34

[Dorel Luca [:dluca]]

Backed out 3 changesets (bug 1451307) for Mochitest failure on toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=175570115&repo=autoland&lineNumber=2760

INFO -  611 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should not import bad css
[task 2018-04-25T19:10:27.183Z] 19:10:27     INFO -  Buffered messages finished
[task 2018-04-25T19:10:27.184Z] 19:10:27     INFO -  612 INFO TEST-UNEXPECTED-FAIL | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Gethash request is not triggered.
[task 2018-04-25T19:10:27.184Z] 19:10:27     INFO -      testUpdate/<@toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html:161:7
[task 2018-04-25T19:10:29.714Z] 19:10:29     INFO -  613 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should not load bad javascript
[task 2018-04-25T19:10:29.714Z] 19:10:29     INFO -  614 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should load clean css
[task 2018-04-25T19:10:29.715Z] 19:10:29     INFO -  615 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should not load bad css
[task 2018-04-25T19:10:29.716Z] 19:10:29     INFO -  616 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should not import bad css
[task 2018-04-25T19:10:29.717Z] 19:10:29     INFO -  617 INFO TEST-UNEXPECTED-FAIL | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Gethash request is not triggered.
[task 2018-04-25T19:10:29.717Z] 19:10:29     INFO -      testUpdateNotClearCompletions/<@toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html:177:7
[task 2018-04-25T19:10:32.253Z] 19:10:32     INFO -  618 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should not load bad javascript
[task 2018-04-25T19:10:32.253Z] 19:10:32     INFO -  619 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should load clean css
[task 2018-04-25T19:10:32.253Z] 19:10:32     INFO -  620 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should not load bad css
[task 2018-04-25T19:10:32.254Z] 19:10:32     INFO -  621 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should not import bad css
[task 2018-04-25T19:10:32.254Z] 19:10:32     INFO -  622 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Gethash request is not triggered.
[task 2018-04-25T19:10:34.591Z] 19:10:34     INFO -  623 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should not load bad javascript
[task 2018-04-25T19:10:34.592Z] 19:10:34     INFO -  624 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should load clean css
[task 2018-04-25T19:10:34.593Z] 19:10:34     INFO -  625 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should not load bad css
[task 2018-04-25T19:10:34.593Z] 19:10:34     INFO -  626 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should not import bad css
[task 2018-04-25T19:10:34.593Z] 19:10:34     INFO -  627 INFO TEST-UNEXPECTED-FAIL | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Gethash request is not triggered.
[task 2018-04-25T19:10:34.594Z] 19:10:34     INFO -      testUpdateCompletionsAfterReload/<@toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html:198:7
[task 2018-04-25T19:10:40.170Z] 19:10:40     INFO -  628 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should not load bad javascript

Push with the failure:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=be0a9b78c2242fcc4eb2545d7fe5b58fecd250e7

Backout:
https://hg.mozilla.org/integration/autoland/rev/76f35d0ecaa690917347f2b0f97180b05e7272bb

Comment 35

[François Marier [:francois]]

(In reply to Dorel Luca [:dluca] from comment #34)
> Backed out 3 changesets (bug 1451307) for Mochitest failure on
> toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html.

That test already fails intermittently on Android (see bug 1456944):
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1456944

Comment 36

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb399145506f3d8311044bd93a80e342ef57b61d

Comment 37

[Johann Hofmann [:johannh]]

Ok, at this point I'm going to file a follow-up bug for the Android test failure. I have spent way too much time on trying to get this to fail locally (and trying to get gdb running with fennec) and I didn't succeed. Note that the failures on automation aren't perma-fails either, it's more like frequent intermittents (https://treeherder.mozilla.org/#/jobs?repo=try&revision=d02d92ae0472a4b946a34dbea5ed00c74c3fad32&selectedJob=176130882).

I don't see how my changes would make this test fail (only on automation, mind you) except that we might have changed some slight perf characteristics which surfaced in the dead slow debug android automation runs. I'm very skeptical that this has any real-world effects.

Comment 38

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f8a711f4ce2b3bfd43e40e73099a40b9bc102af

Comment 39

[Johann Hofmann [:johannh]]

Attachment: Bug 1451307 - Part 4 - Disable test_bug1254766.html and test_cachemiss.html on Android.

This is tracked in https://bugzilla.mozilla.org/show_bug.cgi?id=1458635

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

Comment 40

[Johann Hofmann [:johannh]]

Comment on attachment 8969429 [details]
Bug 1451307 - Part 1 - Consider iframes when comparing loading URIs in nsChannelClassifier::SetBlockedContent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238188/diff/4-5/

Comment 41

[Johann Hofmann [:johannh]]

Comment on attachment 8969430 [details]
Bug 1451307 - Part 2 - Move trackingUI tests out of b/b/c/test/general into their own subdirectory.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238190/diff/4-5/

Comment 42

[Johann Hofmann [:johannh]]

Comment on attachment 8969431 [details]
Bug 1451307 - Part 3 - Add a test for showing the TP UI for iframes and unloaded documents.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238192/diff/5-6/

Comment 43

[François Marier [:francois]]

Comment on attachment 8972645 [details]
Bug 1451307 - Part 4 - Disable test_bug1254766.html and test_cachemiss.html on Android.

https://reviewboard.mozilla.org/r/241198/#review247714

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9f8135caee4f
Part 1 - Consider iframes when comparing loading URIs in nsChannelClassifier::SetBlockedContent. r=francois,mayhemer
https://hg.mozilla.org/integration/autoland/rev/7adc755a4229
Part 2 - Move trackingUI tests out of b/b/c/test/general into their own subdirectory. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/12ea607734b1
Part 3 - Add a test for showing the TP UI for iframes and unloaded documents. r=francois
https://hg.mozilla.org/integration/autoland/rev/9e0246c2f62f
Part 4 - Disable test_bug1254766.html on Android. r=francois

Comment 45

[Andreea Pavel [:apavel]]

Backed out for failing android at toolkit/components/url-classifier/tests/mochitest/test_cachemiss.html 

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9e0246c2f62f9c1e97fa9b194357e01787907213

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=177180482&repo=autoland

Backout: https://hg.mozilla.org/integration/autoland/rev/791af1639cdda1681f029efd07215783ddc6aa67

Comment 46

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=35886eb580d4934c371d8cd3989b404bb2afd8b4

Comment 47

[Tanvi Vyas[:tanvi]]

The metabug this blocks is not resolved duplicate.  Johann, do we need to make this blocking something else?

Comment 48

[Johann Hofmann [:johannh]]

I've looked into this for quite some time now and can't figure out the deal with the Android failures on infra. It's really hard to debug Fennec locally, but nigh impossible on try.

At some point I resorted to the good old binary search comment-out code method and found that this part of the code seems to cause the failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d89691bb5375e2f4f02238d6f173a61c78a6d53

However, various attempts at narrowing down the cause failed so far.

I think this bug should really be fixed, especially with our renewed focus on tracking protection and the TP UI. We're now starting to report trackers by default, even with TP turned off, and that should really include tracking iframes. With more people turning on TP by default (and us potentially turning on some form of TP by default for everyone), it's crucial to give users the ability to unbreak all websites.

Hence, I'm going to disable test_cachemiss.html on Android as well and post my findings in bug 1458635.

Comment 49

[Johann Hofmann [:johannh]]

Comment on attachment 8969429 [details]
Bug 1451307 - Part 1 - Consider iframes when comparing loading URIs in nsChannelClassifier::SetBlockedContent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238188/diff/5-6/

Comment 50

[Johann Hofmann [:johannh]]

Comment on attachment 8969430 [details]
Bug 1451307 - Part 2 - Move trackingUI tests out of b/b/c/test/general into their own subdirectory.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238190/diff/5-6/

Comment 51

[Johann Hofmann [:johannh]]

Comment on attachment 8969431 [details]
Bug 1451307 - Part 3 - Add a test for showing the TP UI for iframes and unloaded documents.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238192/diff/6-7/

Comment 52

[Johann Hofmann [:johannh]]

Comment on attachment 8972645 [details]
Bug 1451307 - Part 4 - Disable test_bug1254766.html and test_cachemiss.html on Android.

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

Comment 53

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ab6c8ee6c86ec7e6e149dcf477455b3fb18b978

Comment 54

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e8d1b667eb51f2cf7dd24125353b9c7154215b6

Comment 56

[Johann Hofmann [:johannh]]

This was fixed somewhere down the road during all the anti-tracking work we did, plus the new protections popup makes this issue somewhat irrelevant because you can always turn off protections for a site.

Comment 57

[Kestrel]

The Bug 1538048 Comment 0 test case still fails though, the TP icon and panel reports no trackers detected despite blocking vimeo. The user needs to be informed that content is being blocked.

Comment 58

[Johann Hofmann [:johannh]]

(In reply to Kestrel from comment #57)

The Bug 1538048 Comment 0 test case still fails though, the TP icon and panel reports no trackers detected despite blocking vimeo. The user needs to be informed that content is being blocked.

On Desktop Nightly this seems to work fine for me, can you describe the issue?

Comment 59

[Kestrel]

The Vimeo player is not currently blocked by the default Level 1 block list, you need the Level 2 block list for this specific test case (hidden behind browser.contentblocking.customBlockList.preferences.ui.enabled). Latest Nightly fails to inform that Vimeo is being blocked in the test case.

Comment 60

[Johann Hofmann [:johannh]]

Ah, right, for the Level 2 list this is still a bug, which makes this less of a priority I'd argue.

Comment 61

[Kestrel]

Which blocklist the test case uses shouldn't make any difference, items on the Level 1 blocklist are affected in the same way. Although Level 1 items are less critical, it is important that all blocking is transparent to the user. The Level 2 blocklist has been exposed in the UI going back to FF57, hiding it in FF70 does not downgrade existing users to the Level 1 blocklist.

The "Tab for a Cause" extension is still affected with the Level 1 blocklist. When it is used for the new tab page, TP detects Social Media Trackers and Fingerprinters but the Tracking Content category is empty. Loading the page directly outside of an iframe and Tracking Content has 13 items. If these numbers are used for the TP Report then it may be significantly under-reporting.

Here's a test case in the wild:

1. Install uBlock Origin (prevents TP from detecting other trackers).
2. Go to https://reddit.com and sign up from the main page (not from https://reddit.com/register which does not use an iframe).
3. Skip email step.
4. Enter username and password.
5. A captcha should appear but is blocked by TP Level 2 blocklist.

Expected:

TP shield icon turns purple indicating content is blocked, clicking on the shield shows google.com is being blocked.

Actual:

TP shield remains gray, clicking it on shows "No trackers known to Nightly were detected on this page". There is no indication a captcha should appear or that anything is being blocked, the Sign Up button simply doesn't respond.

Comment 62

[:Gijs (he/him)]

Silently breaking reddit signup sounds like something that should be more than P5 - Johann?

Comment 63

[Johann Hofmann [:johannh]]

(In reply to :Gijs (he/him) from comment #62)

Silently breaking reddit signup sounds like something that should be more than P5 - Johann?

This is with the Level 2 blocklist, a ton of stuff is broken with that and we're not even really supporting that setting anymore. I can see that the issue still exists with L1 though it is definitely less pronounced. So, sure, we can bump this to P3.