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

Authored by francois on Fri, Jul 27, 10:47 PM.

Details

Summary

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

Diff Detail

Repository
rMOZILLACENTRAL mozilla-central
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
francois created this revision.Fri, Jul 27, 10:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Jul 27, 10:47 PM
phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".Fri, Jul 27, 10:48 PM
phab-bot changed the edit policy from "Custom Policy" to "All Users".
phab-bot removed a project: Restricted Project.
francois requested review of this revision.Fri, Jul 27, 10:48 PM
francois updated this revision to Diff 6146.Mon, Jul 30, 9:31 PM
This comment was removed by francois.
dimi accepted this revision.Wed, Aug 1, 11:51 AM
This revision is now accepted and ready to land.Wed, Aug 1, 11:51 AM
This revision was automatically updated to reflect the committed changes.