Honor user and add-on overrides in tracking annotations

VERIFIED FIXED in Firefox 63

Status

()

enhancement
P1
normal
VERIFIED FIXED
Last year
9 months ago

People

(Reporter: francois, Assigned: francois)

Tracking

(Blocks 1 bug)

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 verified)

Details

Attachments

(2 attachments)

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
Assignee

Updated

Last year
Priority: -- → P2
Assignee

Updated

Last year
Blocks: 1461921
No longer blocks: antitracking
Assignee

Updated

Last year
Assignee: nobody → francois
Blocks: FastBlock
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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"
Assignee

Updated

Last year
See Also: → 1473435

Comment 4

Last year
mozreview-review
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.
Attachment #8989884 - Flags: review?(dlee) → review+

Comment 5

Last year
mozreview-review
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]:
Attachment #8989885 - Flags: review?(dlee) → review+
Assignee

Comment 6

11 months ago
(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 hidden (mozreview-request)

Comment 8

11 months ago
Pushed by fmarier@mozilla.com:
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 hidden (mozreview-request)
Assignee

Updated

11 months ago
Flags: needinfo?(francois)

Comment 11

11 months ago
Pushed by fmarier@mozilla.com:
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

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4b7cb393a455
https://hg.mozilla.org/mozilla-central/rev/58c256b60254
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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.
Flags: needinfo?(francois)
Assignee

Comment 14

9 months ago
(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.
Flags: needinfo?(francois)
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Contact: francois
QA Contact: francois
You need to log in before you can comment on or make changes to this bug.