1461515 - Allow tracking annotations to use a different list than tracking protection

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P1)

Description

[François Marier [:francois]]

Here's a sketch of what this involves:

- add new urlclassifier.trackingAnnotationTable
- cache the value of that pref in the channel classifier
- check against that list in [[https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/netwerk/base/nsChannelClassifier.cpp#1189][nsChannelClassifier::CheckIsTrackerWithLocalTable()]]
- define a new NS_ERROR_TRACKING_ANNOTATION_URI error code
- rename mExpectWhitelistResult flag to mExpectEntityListResult in [[https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/netwerk/base/nsChannelClassifier.cpp#901][TrackingURICallback]]
- rename OnWhitelistResult() to OnEntityListResult()
- distinguish between NS_ERROR_TRACKING_URI and NS_ERROR_TRACKING_ANNOTATION_URI in [[https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/netwerk/base/nsChannelClassifier.cpp#1018][OnTrackerFound]]()

- add the new pref to SafeBrowsing.jsm
- gate the new lists on [[https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/toolkit/components/url-classifier/SafeBrowsing.jsm#429-435][trackingAnnotations]]

Comment 1

[François Marier [:francois]]

Attachment: Bug 1461515 - Split tracking annotations from tracking protection. r?dimi

This makes it possible to use different lists for tracking protection
and for the features that rely on tracking annotations.

Comment 2

[François Marier [:francois]]

Attachment: Bug 1461515 - Fix and expand tracking annotation test. r?dimi

Here's a summary of things that were wrong about this test:

1. It was setting urlclassifier.trackingTable only to be overwritten
   later by addTestTrackers().
2. It was using an http event which fires before the classification has
   been done.
3. It didn't disable tailing, which interferes with lowering the priority of
   XHRs.
4. It was not testing that non-annotated or whitelisted resources would not
   have their priority lowered.

I added more test cases both to ensure that the correct list
(urlclassifier.trackingAnnotationTable) is used but also to ensure that
whitelisted or non-blacklisted URLs preserve the normal priority (point #4 above).

I found that XHRs do not get their priority lowered because of this flag:

  https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/netwerk/base/nsChannelClassifier.cpp#221

which gets set here:

  https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/dom/xhr/XMLHttpRequestMainThread.cpp#2548

and so I had to disable tailing in the test (point #3 above).

There was also a problem where the test was resetting the prefs too early
because we were not actually waiting for the classification to finish.

We would wait for the following event: http-on-opening-request

  https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/netwerk/protocol/http/nsIHttpProtocolHandler.idl#85

whereas maybe a more appropriate one would be http-on-before-connect:

  https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/netwerk/protocol/http/nsIHttpProtocolHandler.idl#103

since that is triggerred after annotations (point #2 above):

  https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/netwerk/protocol/http/nsHttpChannel.cpp#6614

Comment 3

[François Marier [:francois]]

Attachment: Bug 1461515 - Make TP test fail if it uses the wrong list. r?dimi

I added a test case in one of the tracking protection tests which will
fail if the annotation list gets pulled into the TP ones.

I also removed unnecessary prefs that were being set in the test.

Comment 4

[François Marier [:francois]]

Try is looking good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=08c7dbb7504ec0cf1fe95427688aa9252109c5b7

Comment 5

[Phabricator Automation]

Comment on attachment 8995675 [details]
Bug 1461515 - Split tracking annotations from tracking protection. r?dimi

Dimi Lee[:dimi][:dlee] has approved the revision.

https://phabricator.services.mozilla.com/D2484

Comment 6

[Phabricator Automation]

Comment on attachment 8995676 [details]
Bug 1461515 - Fix and expand tracking annotation test. r?dimi

Dimi Lee[:dimi][:dlee] has approved the revision.

https://phabricator.services.mozilla.com/D2485

Comment 7

[Phabricator Automation]

Comment on attachment 8995677 [details]
Bug 1461515 - Make TP test fail if it uses the wrong list. r?dimi

Dimi Lee[:dimi][:dlee] has approved the revision.

https://phabricator.services.mozilla.com/D2486

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3715363d32cb
Split tracking annotations from tracking protection. r=dimi

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/70ffd2c01061
Fix and expand tracking annotation test. r=dimi

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6148cbf8e14c
Make TP test fail if it uses the wrong list. r=dimi

Comment 11

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/3715363d32cb
https://hg.mozilla.org/mozilla-central/rev/70ffd2c01061
https://hg.mozilla.org/mozilla-central/rev/6148cbf8e14c