Closed Bug 1141814 Opened 5 years ago Closed 3 years ago

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

Categories

(Toolkit :: Safe Browsing, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: cpeterson, Assigned: kershaw)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 11 obsolete files)

12.24 KB, patch
kershaw
: review+
Details | Diff | Splinter Review
12.36 KB, patch
kershaw
: review+
Details | Diff | Splinter Review
4.28 KB, patch
kershaw
: review+
Details | Diff | Splinter Review
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.
Clearing the dependency on bug 1029886, because this has no effect on tracking.
No longer depends on: 1029886
(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.
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
Component: Networking: HTTP → DOM: Security
Blocks: 1141182
Component: DOM: Security → Safe Browsing
Product: Core → Toolkit
Priority: -- → P5
Depends on: 1170190
Priority: P5 → P3
Blocks: QuantumDOM
Depends on: 1309653
See Also: → 1312515
Chris, why exactly is bug 1309653 blocking this one?
Blocks: CDP
Flags: needinfo?(cpeterson)
Summary: Lower priority of HTTP requests for resources on the Tracking Protection list → Lower priority of HTTP requests for resources on the Tracking Protection list and mark them as LOAD_BACKGROUND
(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.
No longer depends on: 1309653
Flags: needinfo?(cpeterson)
Kershaw, is this something you could work on?
Assignee: nobody → kechang
You will also need to figure out how much this is blocked by bug 1170190.
(In reply to Honza Bambas (:mayhemer) from comment #6)
> Kershaw, is this something you could work on?

Yes, I think I can do this.
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.
Flags: needinfo?(honzab.moz)
(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.
(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.
Flags: needinfo?(honzab.moz)
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.
Attached patch Part1: Enable TP list updates (obsolete) — Splinter Review
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
Attachment #8809416 - Flags: feedback?(francois)
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|)
Attachment #8809423 - Flags: feedback?(honzab.moz)
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
Attachment #8809423 - Flags: feedback?(honzab.moz) → feedback+
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.
Attachment #8809416 - Flags: feedback?(francois) → feedback+
(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 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.
(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
Flags: needinfo?(honzab.moz)
(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.
Flags: needinfo?(honzab.moz)
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?
Flags: needinfo?(kechang)
(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.
Flags: needinfo?(kechang)
Summary: Lower priority of HTTP requests for resources on the Tracking Protection list and mark them as LOAD_BACKGROUND → Lower priority of HTTP requests for resources on the Tracking Protection list
Summary:
Change the names based on the previous feedback.
Attachment #8809416 - Attachment is obsolete: true
Attachment #8811695 - Flags: review?(francois)
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
Attachment #8809423 - Attachment is obsolete: true
Attachment #8811698 - Flags: review?(honzab.moz)
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 :)
Attachment #8811698 - Flags: review?(honzab.moz) → review+
(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.
Flags: needinfo?(honzab.moz)
(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.
(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.
Flags: needinfo?(honzab.moz)
(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.
Attached patch Part3: test case (obsolete) — Splinter Review
Francois, could you please also take a look at this test case?

Thanks.
Attachment #8812092 - Flags: review?(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"
Attachment #8811695 - Flags: review?(francois) → review+
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.
Attachment #8812092 - Flags: review?(francois)
Attached patch Part3: test case- v2 (obsolete) — Splinter Review
Thanks for the previous review.

I've modified the patch based on your comment. Please take a look. Thanks.
Attachment #8812092 - Attachment is obsolete: true
Attachment #8813026 - Flags: review?(francois)
Attached patch Part3: test case - v2 (obsolete) — Splinter Review
Update the patch to remove a redundant file.

Sorry for the spam.
Attachment #8813026 - Attachment is obsolete: true
Attachment #8813026 - Flags: review?(francois)
Attachment #8813059 - Flags: review?(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.
Attachment #8813059 - Flags: review?(francois) → review+
(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.
Carry reviewer's name.
Attachment #8811695 - Attachment is obsolete: true
Attachment #8811698 - Attachment is obsolete: true
Attachment #8813059 - Attachment is obsolete: true
Attachment #8813964 - Flags: review+
Attached patch Part3: test case, r=francois (obsolete) — Splinter Review
Carry reviewer's name.
Attachment #8813966 - Flags: review+
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
Backout by cbook@mozilla.com:
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
(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
Flags: needinfo?(kechang) → needinfo?(honzab.moz)
Depends on: 1320067
I think this has been discussed on IRC.  The result was (IIRC) to introduce a new notification.
Flags: needinfo?(honzab.moz)
(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.
Attached patch Part3: Unit test (obsolete) — Splinter Review
Honza, could you take a look at this patch?

Or should I ask Francois to review?
Attachment #8813966 - Attachment is obsolete: true
Attachment #8815556 - Flags: review?(honzab.moz)
(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?
Flags: needinfo?(kechang)
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
Attachment #8815556 - Flags: review?(honzab.moz) → review+
(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.
Flags: needinfo?(kechang)
Depends on: 1321486
Rebase.
Attachment #8813964 - Attachment is obsolete: true
Attachment #8813965 - Attachment is obsolete: true
Attachment #8815556 - Attachment is obsolete: true
Attachment #8816012 - Flags: review+
Carry reviewer's name.
Attachment #8816014 - Flags: review+
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
Kershaw, is this still dependent on bug 1170190?  If not, can you please remove the dep?  Thanks.
Flags: needinfo?(kechang)
No longer depends on: 1170190
Flags: needinfo?(kechang)
Depends on: 1324053
See Also: → 1352618
You need to log in before you can comment on or make changes to this bug.