1461534 - Honor user and add-on overrides in tracking annotations

Status: VERIFIED FIXED

(Toolkit :: Safe Browsing, enhancement, P1)

Description

[François Marier [:francois]]

We'll need to remove this early return in nsChannelClassifier::ShouldEnableTrackingProtectionInternal():
https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/netwerk/base/nsChannelClassifier.cpp#378-383

since there may be user overrides and add-ons may need to prevent their loads from being annotated as tracking.

We'll also need to avoid caching marking annotated channels as already classified:
https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/netwerk/base/nsChannelClassifier.cpp#706

There may be a performance impact to allowing users to override tracking annotations since we'll be doing permission manager lookups for every network request at BeginConnectContinue() time:
https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/netwerk/protocol/http/nsHttpChannel.cpp#6394
https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/netwerk/base/nsChannelClassifier.cpp#1203

Comment 1

[François Marier [:francois]]

Attachment: Bug 1461534 - Enable user/add-on overrides of tracking annotations.

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

Comment 2

[François Marier [:francois]]

Attachment: Bug 1461534 - Improve logging for the channel classifier.

This patch reduces the level of noise in MOZ_LOG messages by:

- splitting log messages into Warning, Info, and Debug levels
- truncating long URIs down to the first 128 bytes
- not mentioning suspended channels on NS_OK results

It also introduces new log messages for the outcome of a
match against the blacklist:

- dropping the channel priority (tracking annotations)
- cancelling the channel (tracking protection)
- no match (i.e. not a tracker)

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

Comment 3

[François Marier [:francois]]

Here are the steps to test my fix:

1. start Firefox (fresh profile) with MOZ_LOG="nsChannelClassifier:3"
2. go to https://cnn.com
3. look for evidence that trackers are de-prioritized on the terminal log:

    Setting PRIORITY_LOWEST for channel[0x7fb06add7050] (https://dt.adsafeprotected.com/)

4. set tracking protection to Always in about:preferences#privacy
5. reload https://cnn.com
6. click on shield in the URL bar and then "Disable For This Site"
7. set tracking protection back to "Only in private windows" in about:preferences#privacy
8. reload https://cnn.com
9. look to ensure there aren't any messages like this on the terminal:

    Setting PRIORITY_LOWEST for channel[0x7fb06add7050] (https://dt.adsafeprotected.com/)

10. instead, confirm that the terminal log includes messages like:

    User override on channel[0x7fb07a031050] (https://revee.outbrain.com/page/view) for https://www.cnn.com"

Comment 4

[Dimi Lee [:dimi]]

Comment on attachment 8989884 [details]
Bug 1461534 - Enable user/add-on overrides of tracking annotations.

https://reviewboard.mozilla.org/r/254876/#review262440

::: netwerk/base/nsChannelClassifier.cpp
(Diff revision 1)
> -    // Unlike full Tracking Protection, annotations don't block anything
> -    // so we don't need to take into account add-ons or user exceptions.
> -    if (aAnnotationsOnly) {
> -      *result = true;
> -      return NS_OK;
> -    }

Do we plan to use |aAnnotationsOnly| in the future?
If not, I think we can also remove it from parameter.

Comment 5

[Dimi Lee [:dimi]]

Comment on attachment 8989885 [details]
Bug 1461534 - Improve logging for the channel classifier.

https://reviewboard.mozilla.org/r/254878/#review262442

::: netwerk/base/nsChannelClassifier.cpp:1264
(Diff revision 1)
>    }
>  
>    nsCString trackingBlacklist =
>      CachedPrefs::GetInstance()->GetTrackingBlackList();
>    if (trackingBlacklist.IsEmpty()) {
> -    LOG(("nsChannelClassifier[%p]:CheckIsTrackerWithLocalTable blacklist is empty",
> +    LOG_WARN(("nsChannelClassifier[%p]:CheckIsTrackerWithLocalTable blacklist is empty",

whitepsace after [%p]:

Comment 6

[François Marier [:francois]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #4)
> Do we plan to use |aAnnotationsOnly| in the future?

Yes, in fact we'll be extending that feature :)

Comment 7

[François Marier [:francois]]

Comment on attachment 8989885 [details]
Bug 1461534 - Improve logging for the channel classifier.

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4818535bde9
Enable user/add-on overrides of tracking annotations. r=dimi
https://hg.mozilla.org/integration/autoland/rev/152002e28671
Improve logging for the channel classifier. r=dimi

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out 2 changesets (bug 1461534) for nsChannelClassifier.cpp related build bustages.

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

Backout link: https://hg.mozilla.org/integration/autoland/rev/6354a8dfce2fa6f5fd973e61daaf1e94a618c850

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

Comment 10

[François Marier [:francois]]

Comment on attachment 8989885 [details]
Bug 1461534 - Improve logging for the channel classifier.

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b7cb393a455
Enable user/add-on overrides of tracking annotations. r=dimi
https://hg.mozilla.org/integration/autoland/rev/58c256b60254
Improve logging for the channel classifier. r=dimi

Comment 12

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/4b7cb393a455
https://hg.mozilla.org/mozilla-central/rev/58c256b60254

Comment 13

[Timea Zsoldos [:tzsoldos], Desktop QA]

I can not reproduced this issue on nightly 2018.05.14, the log file not populated with messages.
I try to reproduce on another beta build 62.0b4 (2018.06.28) before the fix but the log file not populated.
I manage to verified it on 63.0b7 on Windows 10 x64, in log file I see this message:
  User override on channel[0000022201A47040] (https://revee.outbrain.com/page/view). This message is ok? The message from step 9 not there.

Comment 14

[François Marier [:francois]]

(In reply to Timea Zsoldos [:zstimi/tzsoldos], Desktop Release QA from comment #13)
> I manage to verified it on 63.0b7 on Windows 10 x64, in log file I see this
> message:
>   User override on channel[0000022201A47040]
> (https://revee.outbrain.com/page/view). This message is ok? The message from
> step 9 not there.

That looks good to me.

Comment 15

[Timea Zsoldos [:tzsoldos], Desktop QA]

I can confirm this issue is fixed, I verified using Firefox 63.0b11 on Ubuntu 18.04 x64, Windows  10 x64 and Mac OS X 10.13 and based on comment 14, removed the flag qe+ and mark this issue as verified-fixed.