1141814 - Lower priority of HTTP requests for resources on the Tracking Protection list

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P3)

Description

[Chris Peterson [:cpeterson]]

When Tracking Protection is disabled, we could still use the Tracking Protection list to lower the priority of those HTTP requests to nsISupportsPriority::PRIORITY_LOWEST.

Patrick says:

to the extent that TP resources are separate origins than other resources it wouldn't actually turn into much of a practical difference. Different origins are basically run in parallel right now and the prioritizations apply within the origin.

There are some exceptions to this when different origins are carried on the same connection - and we will see more of that in the H2/CDN world. And generally doing more with priority information is an evolving area of interest, so marking TP channels as low priority at least creates the meta information to do the right thing when the rest of the stack has more creative things to do.

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

Clearing the dependency on bug 1029886, because this has no effect on tracking.

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to Chris Peterson [:cpeterson] from comment #0)
> When Tracking Protection is disabled, we could still use the Tracking
> Protection list

This requires that the TP list be updated for every user regardless whether TP is enforced. Either one of bug 1134284 or bug 1138979 would cause this as a side-effect.

Comment 3

[Patrick McManus [:mcmanus]]

the change here is pretty much in the TP code (setpriority() where we currently cancel cancel()), so I'll change the component to reflect that

Comment 4

[Honza Bambas (:mayhemer)]

Chris, why exactly is bug 1309653 blocking this one?

Comment 5

[Chris Peterson [:cpeterson]]

(In reply to Honza Bambas (:mayhemer) from comment #4)
> Chris, why exactly is bug 1309653 blocking this one?

It doesn't need to block. Bug 1309653 used to be called "Deprioritize network loads in background pages", but it looks like the scope of that bug changed.

Comment 6

[Honza Bambas (:mayhemer)]

Kershaw, is this something you could work on?

Comment 7

[Honza Bambas (:mayhemer)]

You will also need to figure out how much this is blocked by bug 1170190.

Comment 8

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #6)
> Kershaw, is this something you could work on?

Yes, I think I can do this.

Comment 9

[Kershaw Chang [:kershaw]]

I think this bug could need two steps to complete:

1. Figure out how to classify URLs when TP is disabled.
   Modify nsChannelClassifier and nsUrlClassifierDBService to make them work without TP being enabled.
   This might be the most difficult one, since I am not sure how much complicated it is. 

2. Based on the classified result to adjust the channel's priority and mark LOAD_BACKGROUND.
   This seems easier compared to step 1. Maybe just call |SetPriority| and directly change |mLoadFlags| is enough?

Honza, could you have a look at my rough plan and point out if I overlook something? Thanks.

Comment 10

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #7)
> You will also need to figure out how much this is blocked by bug 1170190.

It seems that those discussions in bug 1170190 are quite high level and the scope is not clear to me.
I think we can just start with this bug.

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Kershaw Chang [:kershaw] from comment #9)
> I think this bug could need two steps to complete:
> 
> 1. Figure out how to classify URLs when TP is disabled.
>    Modify nsChannelClassifier and nsUrlClassifierDBService to make them work
> without TP being enabled.
>    This might be the most difficult one, since I am not sure how much
> complicated it is. 

Hmm.. could you just directly use the same code as we use here?

https://dxr.mozilla.org/mozilla-central/rev/3e73fd638e687a4d7f46613586e5156b8e2af846/netwerk/protocol/http/nsHttpChannel.cpp#5891

I mean, independently on whether classification is enforced or allowed for the load.

If so, I think this makes few things easier.  If not, I'll start looking and asking.

> 
> 2. Based on the classified result to adjust the channel's priority and mark
> LOAD_BACKGROUND.
>    This seems easier compared to step 1. Maybe just call |SetPriority| and
> directly change |mLoadFlags| is enough?

Yes, it's should be exactly that easy, nothing more needed.  Just do it before you go out to network.

> 
> Honza, could you have a look at my rough plan and point out if I overlook
> something? Thanks.

If you find it easy to find out if the URL is on the tracking list, I think this can be done in a single bug and patch.

Comment 12

[François Marier [:francois]]

First of all, if you need help with this, I'm happy to assist and you can also chat with Dimi, Henry and Thomas in Taipei who are all working on the URL Classifier at the moment.

(In reply to Kershaw Chang [:kershaw] from comment #9)
> 1. Figure out how to classify URLs when TP is disabled.
>    Modify nsChannelClassifier and nsUrlClassifierDBService to make them work
> without TP being enabled.

Given that bug 1312515 (or bug 1312514) and bug 1170190 will all need this, we could split this up in a separate dependent bug. Up to you.

We're going to need two things:

(a) Enable list updates when TP is disabled.
(b) Annotate channels used to load a tracking resource. 

Currently we look at the two TP prefs:

https://dxr.mozilla.org/mozilla-central/rev/3e73fd638e687a4d7f46613586e5156b8e2af846/toolkit/components/url-classifier/SafeBrowsing.jsm#182

to determine whether or not to update the list:

https://dxr.mozilla.org/mozilla-central/rev/3e73fd638e687a4d7f46613586e5156b8e2af846/toolkit/components/url-classifier/SafeBrowsing.jsm#302-315

We could add a new pref here like "privacy.trackingprotection.annotate_channels" or something like that. If it's off, then we won't update the list or annotate the channels and so none of the perf features will do anything.

I'm not exactly sure at which point we'll want to annotate the channels, but I suspect we could refactor the existing tracking protection code to flag the channel as a tracking resource. Then any other piece of code could look at that flag and decide to deprioritize, suspend or cancel the channel as appropriate.

(In reply to Kershaw Chang [:kershaw] from comment #10)
> (In reply to Honza Bambas (:mayhemer) from comment #7)
> > You will also need to figure out how much this is blocked by bug 1170190.
> 
> It seems that those discussions in bug 1170190 are quite high level and the
> scope is not clear to me.
> I think we can just start with this bug.

Had bug 1170190 been resolved first, you would have had most of #1 already done. If you do that work however, that bug is no longer blocking you.

Comment 13

[Kershaw Chang [:kershaw]]

Attachment: Part1: Enable TP list updates

Summary:
- As described in comment #12, this patch adds a new pref to control TP list updates when the TP is disabled. 
- Remove |mCheckTracking| in nsUrlClassifierDBService, since it's not used.

Hi Francois,

Could you have a look at this patch?

It seems that nsIURIClassifier::classify already depends on the parameter aTrackingProtectionEnabled [1] to decide whether to classify the URI against the TP list. So, I think we can just use nsIURIClassifier::classify to annotate channels if TP is disabled.

[1] http://searchfox.org/mozilla-central/rev/846adaea6ccd1428780ed47d0d94da18478acdbb/netwerk/base/nsIURIClassifier.idl#56

Comment 14

[Kershaw Chang [:kershaw]]

Attachment: Part2: Lower the priority of the channel loading tracking resource

Summary:
- Save the result of |ShouldEnableTrackingProtection|. Since this function is called in twice in [1] and [2], which are in the same call path.
- Lower the priority of the channel and set new load flags when TP is disabled.

Honza, Could you please have a look and provide some feedback? Thanks. 

[1] http://searchfox.org/mozilla-central/rev/846adaea6ccd1428780ed47d0d94da18478acdbb/netwerk/protocol/http/nsHttpChannel.cpp#5883
[2] http://searchfox.org/mozilla-central/rev/846adaea6ccd1428780ed47d0d94da18478acdbb/netwerk/base/nsChannelClassifier.cpp#344 (via |nsChannelClassifier::Start|)

Comment 15

[Honza Bambas (:mayhemer)]

Comment on attachment 8809423 [details] [diff] [review]
Part2: Lower the priority of the channel loading tracking resource

Review of attachment 8809423 [details] [diff] [review]:
-----------------------------------------------------------------

looks good !

::: netwerk/base/nsChannelClassifier.cpp
@@ +680,5 @@
>               this, errorName.get()));
>        }
>        MarkEntryClassified(aErrorCode);
>  
> +      MOZ_ASSERT(mTrackingProtectionEnabled);

I only don't follow this assertion

Comment 16

[François Marier [:francois]]

Comment on attachment 8809416 [details] [diff] [review]
Part1: Enable TP list updates

Review of attachment 8809416 [details] [diff] [review]:
-----------------------------------------------------------------

I suggested different names inline, but this is the right approach.

::: modules/libpref/init/all.js
@@ +1221,5 @@
>  pref("privacy.trackingprotection.enabled",  false);
>  // Enforce tracking protection in Private Browsing mode
>  pref("privacy.trackingprotection.pbmode.enabled",  true);
> +// Enforce annotating channels in all modes
> +pref("privacy.trackingprotection.annotate_channels.enabled",  false);

Now that I think of it, the `enabled` part is a little redundant. We could just use `privacy.trackingprotection.annotate_channels`.

::: toolkit/components/url-classifier/SafeBrowsing.jsm
@@ +119,5 @@
> +  phishingEnabled:    false,
> +  malwareEnabled:     false,
> +  trackingEnabled:    false,
> +  blockedEnabled:     false,
> +  annotatingEnabled:  false,

Maybe `trackingAnnotations` instead of `annotatingEnabled` so that it's clear that we annotating based on the TP list?

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ -98,5 @@
>  
>  #define CHECK_PHISHING_PREF     "browser.safebrowsing.phishing.enabled"
>  #define CHECK_PHISHING_DEFAULT  false
>  
> -#define CHECK_TRACKING_PREF     "privacy.trackingprotection.enabled"

I'm surprised we're not using these, but if it compiles and the tests pass, I guess it's true.

Comment 17

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #15)
> Comment on attachment 8809423 [details] [diff] [review]
> Part2: Lower the priority of the channel loading tracking resource
> 
> Review of attachment 8809423 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks good !
> 
> ::: netwerk/base/nsChannelClassifier.cpp
> @@ +680,5 @@
> >               this, errorName.get()));
> >        }
> >        MarkEntryClassified(aErrorCode);
> >  
> > +      MOZ_ASSERT(mTrackingProtectionEnabled);
> 
> I only don't follow this assertion

At this point that |mTrackingProtectionEnabled| should contain a value, so add an assertion here to make sure that |ShouldEnableTrackingProtection| has been called before.

Comment 18

[Kershaw Chang [:kershaw]]

Comment on attachment 8809423 [details] [diff] [review]
Part2: Lower the priority of the channel loading tracking resource

Review of attachment 8809423 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsChannelClassifier.cpp
@@ +697,5 @@
> +        }
> +        nsLoadFlags newLoadFlags;
> +        mChannel->GetLoadFlags(&newLoadFlags);
> +        newLoadFlags |= nsIChannel::LOAD_BACKGROUND;
> +        mChannel->SetLoadFlags(newLoadFlags);

It seems that only marking the load flag with LOAD_BACKGROUND is not enough, we should also update |mForegroundCount| in this channel's loadgroup.

Comment 19

[Kershaw Chang [:kershaw]]

(In reply to Kershaw Chang [:kershaw] from comment #18)
> Comment on attachment 8809423 [details] [diff] [review]
> Part2: Lower the priority of the channel loading tracking resource
> 
> Review of attachment 8809423 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/nsChannelClassifier.cpp
> @@ +697,5 @@
> > +        }
> > +        nsLoadFlags newLoadFlags;
> > +        mChannel->GetLoadFlags(&newLoadFlags);
> > +        newLoadFlags |= nsIChannel::LOAD_BACKGROUND;
> > +        mChannel->SetLoadFlags(newLoadFlags);
> 
> It seems that only marking the load flag with LOAD_BACKGROUND is not enough,
> we should also update |mForegroundCount| in this channel's loadgroup.

Honza,

I was wondering is it really worth to mark the channel as LOAD_BACKGROUND? If yes, it means that we might need to add new function in nsILoadGroup to update |mForegroundCount|. Moreover, it could cause that |onStopRequest| is not always called after |onStartRequest|, since it is not called when the load flags has LOAD_BACKGROUND [1].


[1] http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/netwerk/base/nsLoadGroup.cpp#623

Comment 20

[Honza Bambas (:mayhemer)]

(In reply to Kershaw Chang [:kershaw] from comment #19)
> (In reply to Kershaw Chang [:kershaw] from comment #18)
> > Comment on attachment 8809423 [details] [diff] [review]
> > Part2: Lower the priority of the channel loading tracking resource
> > 
> > Review of attachment 8809423 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: netwerk/base/nsChannelClassifier.cpp
> > @@ +697,5 @@
> > > +        }
> > > +        nsLoadFlags newLoadFlags;
> > > +        mChannel->GetLoadFlags(&newLoadFlags);
> > > +        newLoadFlags |= nsIChannel::LOAD_BACKGROUND;
> > > +        mChannel->SetLoadFlags(newLoadFlags);
> > 
> > It seems that only marking the load flag with LOAD_BACKGROUND is not enough,
> > we should also update |mForegroundCount| in this channel's loadgroup.
> 
> Honza,
> 
> I was wondering is it really worth to mark the channel as LOAD_BACKGROUND?
> If yes, it means that we might need to add new function in nsILoadGroup to
> update |mForegroundCount|. Moreover, it could cause that |onStopRequest| is
> not always called after |onStartRequest|, since it is not called when the
> load flags has LOAD_BACKGROUND [1].
> 
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 4b6cab91f93c73ae591dafaea40fd5704b41810e/netwerk/base/nsLoadGroup.cpp#623

And what about:
- remove the channel from it's load group (if there is one)
- change the flag
- add the channel again to the same load group

Assert is all happens on the main thread!  Load groups are not thread safe.

Comment 21

[Honza Bambas (:mayhemer)]

Hmm... but what about the child process... right... 

Maybe file a new bug and let's solve the background flag there.  If we only lower the priority (in a patch for this bug), I think we have 80% of the requirement fulfilled here.

Makes sense?

Comment 22

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #21)
> Hmm... but what about the child process... right... 
> 
> Maybe file a new bug and let's solve the background flag there.  If we only
> lower the priority (in a patch for this bug), I think we have 80% of the
> requirement fulfilled here.
> 
> Makes sense?

Sure. I'll file a new bug once this bug has fixed.

Comment 23

[Kershaw Chang [:kershaw]]

Attachment: Part1: Enable TP list updates - v2

Summary:
Change the names based on the previous feedback.

Comment 24

[Kershaw Chang [:kershaw]]

Attachment: Part2: Lower the priority of the channel loading tracking resource - v2

Summary:
Remove the setting the load flags part. Only set the priority.

In addition, I found it's difficult for me to create a test case, since it's not easy to get a request's priority in js code. I've tried to use nsIWebProgressListener to monitor priority changes, but I can't get the latest value after setting to PRIORITY_LOWEST. Maybe we have to notify nsIWebProgressListener of the priority changes like what we did in [1].

Or do you think we can land this patch without having a test case?

[1] http://searchfox.org/mozilla-central/rev/62db1c9021cfbde9fa5e6e9601de16c21f4c7ce4/netwerk/base/nsChannelClassifier.cpp#554-567

Comment 25

[Honza Bambas (:mayhemer)]

Comment on attachment 8811698 [details] [diff] [review]
Part2: Lower the priority of the channel loading tracking resource - v2

Review of attachment 8811698 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!  Looks good to me.

r=honzab

::: netwerk/base/nsChannelClassifier.cpp
@@ +357,5 @@
> +      trackingProtectionEnabled = mTrackingProtectionEnabled.value();
> +    }
> +
> +    bool annotateChannelEnabled =
> +      Preferences::GetBool("privacy.trackingprotection.annotate_channels");

nit: since this is on a critical path, having a cache for the preference seems wiser.  (Preferences::AddBoolVarCache)

@@ +680,5 @@
>               this, errorName.get()));
>        }
>        MarkEntryClassified(aErrorCode);
>  
> +      MOZ_ASSERT(mTrackingProtectionEnabled);

add a comment to this assertion why it's here and what exactly it asserts

@@ +682,5 @@
>        MarkEntryClassified(aErrorCode);
>  
> +      MOZ_ASSERT(mTrackingProtectionEnabled);
> +      if (aErrorCode == NS_ERROR_TRACKING_URI &&
> +          !mTrackingProtectionEnabled.valueOr(false)) {

this is probably why I didn't (and still don't) understand the assertion.  Just before this condition you assert mTrackingProtectionEnabled is set a value.  But here you branch when it's set false or not keeping a value.

There is probably some mistake here, either in the assertion or in the condition (should only be && !mTrackingProtectionEnabled.value()?)  Or you want a fallback path for release build when this would break?  Or I need more coffee today?  Not sure :)

Comment 26

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #25)
> Comment on attachment 8811698 [details] [diff] [review]
> Part2: Lower the priority of the channel loading tracking resource - v2
> 
> Review of attachment 8811698 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks!  Looks good to me.
> 
> r=honzab
> 
> ::: netwerk/base/nsChannelClassifier.cpp
> @@ +357,5 @@
> > +      trackingProtectionEnabled = mTrackingProtectionEnabled.value();
> > +    }
> > +
> > +    bool annotateChannelEnabled =
> > +      Preferences::GetBool("privacy.trackingprotection.annotate_channels");
> 
> nit: since this is on a critical path, having a cache for the preference
> seems wiser.  (Preferences::AddBoolVarCache)
> 
> @@ +680,5 @@
> >               this, errorName.get()));
> >        }
> >        MarkEntryClassified(aErrorCode);
> >  
> > +      MOZ_ASSERT(mTrackingProtectionEnabled);
> 
> add a comment to this assertion why it's here and what exactly it asserts
> 
> @@ +682,5 @@
> >        MarkEntryClassified(aErrorCode);
> >  
> > +      MOZ_ASSERT(mTrackingProtectionEnabled);
> > +      if (aErrorCode == NS_ERROR_TRACKING_URI &&
> > +          !mTrackingProtectionEnabled.valueOr(false)) {
> 
> this is probably why I didn't (and still don't) understand the assertion. 
> Just before this condition you assert mTrackingProtectionEnabled is set a
> value.  But here you branch when it's set false or not keeping a value.
> 
> There is probably some mistake here, either in the assertion or in the
> condition (should only be && !mTrackingProtectionEnabled.value()?)  Or you
> want a fallback path for release build when this would break?  Or I need
> more coffee today?  Not sure :)

Yes, I want a fallback path for release build. Not sure what to do is correct here.
Do you think |!mTrackingProtectionEnabled.value()| is ok and we don't have to worry about release build?

BTW, could you please also provide some feedback about test case in comment#24? Thanks.

Comment 27

[Honza Bambas (:mayhemer)]

(In reply to Kershaw Chang [:kershaw] from comment #26)
> Yes, I want a fallback path 

Yes, that's a good rational for me.  Just add a comment to the code and you are done.
 
> BTW, could you please also provide some feedback about test case in
> comment#24? Thanks.

Will do shortly.

Comment 28

[Honza Bambas (:mayhemer)]

(In reply to Kershaw Chang [:kershaw] from comment #24)
> In addition, I found it's difficult for me to create a test case, since it's
> not easy to get a request's priority in js code. 

If you just want to check the priority have been set to some expected value, you may use either on-modify-request or on-examine-response (the former would be more appropriate but the latter would do too) to inspect the channel directly (GetPriority).

> I've tried to use
> nsIWebProgressListener to monitor priority changes, but I can't get the
> latest value after setting to PRIORITY_LOWEST. Maybe we have to notify
> nsIWebProgressListener of the priority changes like what we did in [1].

I would not bother.

> 
> Or do you think we can land this patch without having a test case?

If the test would become too complicated, manual testing (in your discretion before landing) for now is probably enough.  Prioritization is generally a hard thing to test.

Comment 29

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #28)
> (In reply to Kershaw Chang [:kershaw] from comment #24)
> > In addition, I found it's difficult for me to create a test case, since it's
> > not easy to get a request's priority in js code. 
> 
> If you just want to check the priority have been set to some expected value,
> you may use either on-modify-request or on-examine-response (the former
> would be more appropriate but the latter would do too) to inspect the
> channel directly (GetPriority).
> 

Thanks! "on-examine-response" is exactly what I need.

Comment 30

[Kershaw Chang [:kershaw]]

Attachment: Part3: test case

Francois, could you please also take a look at this test case?

Thanks.

Comment 31

[François Marier [:francois]]

Comment on attachment 8811695 [details] [diff] [review]
Part1: Enable TP list updates - v2

Review of attachment 8811695 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpref/init/all.js
@@ +1222,5 @@
>  // Enforce tracking protection in all modes
>  pref("privacy.trackingprotection.enabled",  false);
>  // Enforce tracking protection in Private Browsing mode
>  pref("privacy.trackingprotection.pbmode.enabled",  true);
> +// Enforce annotating channels based on tracking protection list in all modes

nit: I would suggest just "Annotate channels based on the tracking protection list in all modes"

Comment 32

[François Marier [:francois]]

Comment on attachment 8812092 [details] [diff] [review]
Part3: test case

Review of attachment 8812092 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine. I've suggested two small tweaks which would expand the scope of the test.

Also, if you want, you could test the case where `privacy.trackingprotection.annotate_channels = false` and see that the priority isn't changed there.

::: toolkit/components/url-classifier/tests/mochitest/test_trackingprotection_annotateChannels.html
@@ +40,5 @@
> +}
> +
> +var trackerPriorityMap = {
> +  "evil.js": Ci.nsISupportsPriority.PRIORITY_LOWEST,
> +  "evil.css": Ci.nsISupportsPriority.PRIORITY_LOWEST,

It would be great if you could also add a filename here that is expected to be the default priority. That way this test case would ensure two things:

1. we make the tracking elements PRIORITY_LOWEST
2. we don't touch the priority of non-tracking elements

@@ +77,5 @@
> +
> +SpecialPowers.pushPrefEnv(
> +  {"set" : [["urlclassifier.trackingTable", "test-track-simple"],
> +            ["privacy.trackingprotection.enabled", false],
> +            ["privacy.trackingprotection.pbmode.enabled", true],

I would suggest setting this one to false so that we also test that the list is available when only the channel annotation is enabled.

Comment 33

[Kershaw Chang [:kershaw]]

Attachment: Part3: test case- v2

Thanks for the previous review.

I've modified the patch based on your comment. Please take a look. Thanks.

Comment 34

[Kershaw Chang [:kershaw]]

Attachment: Part3: test case - v2

Update the patch to remove a redundant file.

Sorry for the spam.

Comment 35

[François Marier [:francois]]

Comment on attachment 8813059 [details] [diff] [review]
Part3: test case - v2

Review of attachment 8813059 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/url-classifier/tests/mochitest/test_trackingprotection_annotateChannels.html
@@ +90,5 @@
> +SpecialPowers.pushPrefEnv(
> +  {"set" : [["urlclassifier.trackingTable", "test-track-simple"],
> +            ["privacy.trackingprotection.enabled", false],
> +            ["privacy.trackingprotection.pbmode.enabled", false],
> +            ["channelclassifier.allowlist_example", true]]},

Maybe we need to set "privacy.trackingprotection.annotate_channels = false" explicitly, in case we default to "true" in the future.

Comment 36

[Kershaw Chang [:kershaw]]

(In reply to François Marier [:francois] from comment #35)
> Comment on attachment 8813059 [details] [diff] [review]
> Part3: test case - v2
> 
> Review of attachment 8813059 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> toolkit/components/url-classifier/tests/mochitest/
> test_trackingprotection_annotateChannels.html
> @@ +90,5 @@
> > +SpecialPowers.pushPrefEnv(
> > +  {"set" : [["urlclassifier.trackingTable", "test-track-simple"],
> > +            ["privacy.trackingprotection.enabled", false],
> > +            ["privacy.trackingprotection.pbmode.enabled", false],
> > +            ["channelclassifier.allowlist_example", true]]},
> 
> Maybe we need to set "privacy.trackingprotection.annotate_channels = false"
> explicitly, in case we default to "true" in the future.

Nice catch!

Thanks for the review.

Comment 37

[Kershaw Chang [:kershaw]]

Attachment: Part1: Enable TP list updates, r=francois

Carry reviewer's name.

Comment 38

[Kershaw Chang [:kershaw]]

Attachment: Part2: Lower the priority of the channel loading tracking resource, r=honzab

Carry reviewer's name.

Comment 39

[Kershaw Chang [:kershaw]]

Attachment: Part3: test case, r=francois

Carry reviewer's name.

Comment 40

[Kershaw Chang [:kershaw]]

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

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/504e1e32c8b8
Part1: Enable to update TP list if TP is disabled, r=francois
https://hg.mozilla.org/integration/mozilla-inbound/rev/76a1099ef0b4
Part2: Lower the priority of channel loading tracking resource, r=honzab
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c26d0bd467b
Part3: Test case, r=francois

Comment 42

[Carsten Book [:Tomcat]]

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=39798545&repo=mozilla-inbound

Comment 43

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/43b2fafc68f4
Backed out changeset 6c26d0bd467b 
https://hg.mozilla.org/integration/mozilla-inbound/rev/91924feee6b0
Backed out changeset 76a1099ef0b4 
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e68f80c7826
Backed out changeset 504e1e32c8b8 for test failures in own tests

Comment 44

[Kershaw Chang [:kershaw]]

(In reply to Carsten Book [:Tomcat] from comment #42)
> backed out for test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=39798545&repo=mozilla-
> inbound

I believe this is due to that the channel's priority could be changed in other places, like [1].
Actually, "on-examine-response" could be notified after [1], so we cannot always get the correct priority at that time. Not to mention that "on-examine-response" is a little late. We should really check the priority before calling |SetupTransaction|.

Honza, do you think is it worth to add another observer for the test purpose in this bug?


[1] http://searchfox.org/mozilla-central/rev/b4e6fa3527436bdbcd9dd2e1982382fe423431f3/netwerk/base/nsLoadGroup.cpp#617

Comment 45

[Honza Bambas (:mayhemer)]

I think this has been discussed on IRC.  The result was (IIRC) to introduce a new notification.

Comment 46

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #45)
> I think this has been discussed on IRC.  The result was (IIRC) to introduce
> a new notification.

After adding a new notification, I am still not able to get the priority value I want. I also found that the priority had been set by NetworkPrioritizer.jsm and there no way to guarantee when the priority would be changed.
So, I think the best way to test this is by xpcselltest without the interference from NetworkPrioritizer.

Comment 47

[Kershaw Chang [:kershaw]]

Attachment: Part3: Unit test

Honza, could you take a look at this patch?

Or should I ask Francois to review?

Comment 48

[Kershaw Chang [:kershaw]]

Test result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc3dac3dd5051deaa9a4ff5c4117f572484e3ed6&selectedJob=32060504

Comment 49

[Honza Bambas (:mayhemer)]

(In reply to Kershaw Chang [:kershaw] from comment #46)
> (In reply to Honza Bambas (:mayhemer) from comment #45)
> > I think this has been discussed on IRC.  The result was (IIRC) to introduce
> > a new notification.
> 
> After adding a new notification, I am still not able to get the priority
> value I want. I also found that the priority had been set by
> NetworkPrioritizer.jsm 

Ups.  Then your test did the right job and _actually found a problem!_  

How exactly does NetworkPrioritizer.jsm plays with the already set priorities?  Does it destroy the priority you are trying to set in the classifier?  If so, we probably have to fix NetworkPrioritizer.jsm and not the tests to mask this.

> and there no way to guarantee when the priority would
> be changed.
> So, I think the best way to test this is by xpcselltest without the
> interference from NetworkPrioritizer.

To make a very basic test yes an xpcshell test is a good thing to have.  

To test how it behaves when being part of a navigation when all the surrounding mechanisms are involved (like during a normal use of the browser) we need ALSO the previous test (a mochitest).

Or am I missing something?

Comment 50

[Honza Bambas (:mayhemer)]

Comment on attachment 8815556 [details] [diff] [review]
Part3: Unit test

Review of attachment 8815556 [details] [diff] [review]:
-----------------------------------------------------------------

roughly looked only, looks good.   if works reliably, land.

r=honzab

::: netwerk/test/unit/test_trackingProtection_annotateChannels.js
@@ +1,1 @@
> +

remove this blank like

add a comment with steps what this test is performing so one doesn't need to study the code to know

Comment 51

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #49)
> (In reply to Kershaw Chang [:kershaw] from comment #46)
> > (In reply to Honza Bambas (:mayhemer) from comment #45)
> > > I think this has been discussed on IRC.  The result was (IIRC) to introduce
> > > a new notification.
> > 
> > After adding a new notification, I am still not able to get the priority
> > value I want. I also found that the priority had been set by
> > NetworkPrioritizer.jsm 
> 
> Ups.  Then your test did the right job and _actually found a problem!_  
> 
> How exactly does NetworkPrioritizer.jsm plays with the already set
> priorities?  Does it destroy the priority you are trying to set in the
> classifier?  If so, we probably have to fix NetworkPrioritizer.jsm and not
> the tests to mask this.
> 
Yes, NetworkPrioritizer could affect the priority which is already set in the classifier. We still have a potential issue here. Let me file another bug to address this.

Comment 52

[Kershaw Chang [:kershaw]]

Attachment: Part1: Enable TP list updates, r=francois

Rebase.

Comment 53

[Kershaw Chang [:kershaw]]

Attachment: Part2: Lower the priority of the channel loading tracking resource, r=honzab

Rebase

Comment 54

[Kershaw Chang [:kershaw]]

Attachment: Part3: Unit test, r=honzab

Carry reviewer's name.

Comment 55

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc772f9c5fb4
Part 1: Enable to update TP list if TP is disabled. r=francois
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb18b507a912
Part 2: Lower the priority of channel loading tracking resource. r=honzab
https://hg.mozilla.org/integration/mozilla-inbound/rev/33de2e2e2cf3
Part 3: Unit test. r=honzab

Comment 56

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fc772f9c5fb4
https://hg.mozilla.org/mozilla-central/rev/fb18b507a912
https://hg.mozilla.org/mozilla-central/rev/33de2e2e2cf3

Comment 57

[Honza Bambas (:mayhemer)]

Kershaw, is this still dependent on bug 1170190?  If not, can you please remove the dep?  Thanks.