1312515 - Lower priority of HTTP requests coming from tracking scripts

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[(no longer active)]

With bug 1312514, we can tell for example whether an XHR is coming from a tracking script.  With that information, we should be able to lower the priority of such HTTP requests.

Comment 1

[Masatoshi Kimura [:emk]]

Will <script> (without async/defer) or document.write no longer block loading for tracking scripts?

Comment 2

[u408661]

Honza - putting this on your radar, as it seems to fit in the context-driven priority project.

Comment 3

[Honza Bambas (:mayhemer)]

I know about this bug.  Thanks.

Comment 4

[(no longer active)]

(In reply to Masatoshi Kimura [:emk] from comment #1)
> Will <script> (without async/defer) or document.write no longer block
> loading for tracking scripts?

This bug should not affect script loading.  I think we only want to start doing this for XHR and fetch.

Comment 5

[(no longer active)]

With bug 1312514 being finished, now we should be able to do this.  But AFAICT there's not much to be done on the Necko side here.  I _think_ this can be done on the XHR side using the nsISupportsPriority API.  Does that sound correct, Honza?

Also, is it OK to also tie this behavior to the privacy.trackingprotection.lower_network_priority pref being added in bug 1170190?

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to :Ehsan Akhgari from comment #5)
> With bug 1312514 being finished, now we should be able to do this.  But
> AFAICT there's not much to be done on the Necko side here.  I _think_ this
> can be done on the XHR side using the nsISupportsPriority API.  Does that
> sound correct, Honza?

If the creator of the XHR knows it's coming from a tracking source, then yes, just lower the priority (and maybe fix it as 

> 
> Also, is it OK to also tie this behavior to the
> privacy.trackingprotection.lower_network_priority pref being added in bug
> 1170190?

I think yes, tho I would be more happy if that was not handled on two separate places (in the classifier and when creating XHR from a tracker).  But I think we can live with that.

Comment 7

[Kershaw Chang [:kershaw]]

Attachment: Part1: Lower the channel's priority

Summary:
 - Use |nsIDocument::IsScriptTracking| to find out if this XHR's creator is a tracking script.
 - If yes, Lower the channel's priority.

@baku, could you take a look at this patch? Thanks.

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8864469 [details] [diff] [review]
Part1: Lower the channel's priority

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

Looks good. But use Preferences::AddBoolVarCache.

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +2756,5 @@
>    nsCOMPtr<nsIStreamListener> listener = new net::nsStreamListenerWrapper(this);
>  
> +  // Check if this XHR is created from a tracking script.
> +  // If yes, lower the channel's priority.
> +  if (Preferences::GetBool("privacy.trackingprotection.lower_network_priority")) {

Use Preferences::AddBoolVarCache instead.

Comment 9

[Kershaw Chang [:kershaw]]

Attachment: Part1: Lower the channel's priority for XHR, v2

Summary:
 - Use Preferences::AddBoolVarCache

Comment 10

[Kershaw Chang [:kershaw]]

Attachment: Part2: Lower the channel's priority for Fetch

Summary:
 - Add |mIsTrackingFetch| in FetchDriver and set it in constructor.
 - Check if this is triggered in tracking script in FetchRequest.

Note that I am not sure if we want to also do the same for the xhr/fetch created in worker thread. If so, that means we have to track if a worker thread is created from a tracking script? Maybe we have to file another bug to discuss.

In bug 1312514, we also didn't handle the case if the timeout is from a worker created by a tracking script. What do you think, Ehsan?

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 8865823 [details] [diff] [review]
Part2: Lower the channel's priority for Fetch

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

File a bug for workers, yes.

::: dom/fetch/Fetch.cpp
@@ +279,5 @@
>        nsCOMPtr<nsIPrincipal> principal = proxy->GetWorkerPrivate()->GetPrincipal();
>        MOZ_ASSERT(principal);
>        nsCOMPtr<nsILoadGroup> loadGroup = proxy->GetWorkerPrivate()->GetLoadGroup();
>        MOZ_ASSERT(loadGroup);
> +      fetch = new FetchDriver(mRequest, principal, loadGroup, false);

add a comment about this 'false'.

Comment 12

[(no longer active)]

(In reply to Kershaw Chang [:kershaw] from comment #10)
> In bug 1312514, we also didn't handle the case if the timeout is from a
> worker created by a tracking script. What do you think, Ehsan?

The rationale for throttling the main-thread timeouts doesn't apply to worker threads, since timeouts running on worker threads can't jank foreground tabs.

Comment 13

[Kershaw Chang [:kershaw]]

Attachment: Part1: Lower the channel's priority for XHR, r=baku

Summary:
 - Rebase
 - Add IsLowerNetworkPriority in nsContentUtils to read preference "privacy.trackingprotection.lower_network_priority".

Comment 14

[Kershaw Chang [:kershaw]]

Attachment: Part2: Lower the channel's priority for Fetch, r=baku

Rebase.

Comment 15

[Kershaw Chang [:kershaw]]

Attachment: Part3: Test case

Summary:
 - A test case to test the priority of http channels created from XHR and Fetch

Test steps:
  1. Open a new window and load trackingRequest.html.
  2. In trackingRequest.html, load a tracker - trackingRequest.js.
  3. trackingRequest.js will send a XHR or a fetch request according to the received message.
  4. Register the observer "http-on-opening-request" and check if the priority is lowest.

Please have a look. Thanks.

Comment 16

[Andrea Marchesini [:baku]]

Comment on attachment 8869939 [details] [diff] [review]
Part3: Test case

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

::: toolkit/components/url-classifier/tests/mochitest/test_trackingprotection_bug1312515.html
@@ +19,5 @@
> +var Cc = SpecialPowers.Cc;
> +var Ci = SpecialPowers.Ci;
> +
> +var mainWindow = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                    .getInterface(Ci.nsIWebNavigation)

indentation here.

Comment 17

[Kershaw Chang [:kershaw]]

Attachment: Part3: Test case, r=baku

Fix review comments.

Thanks for the review.

Comment 18

[Kershaw Chang [:kershaw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=86fa2e4820e0306891842b97d7204378787dcf8d

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6de4c33aa5b
Part 1: Lower the channel's priority if this XHR is created from tracking script. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/45ee0803b090
Part 2: Lower the channel's priority if this Fetch  is created from tracking script. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6bfe95b76bb
Part 3: Test case. r=baku

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/a6de4c33aa5b
https://hg.mozilla.org/mozilla-central/rev/45ee0803b090
https://hg.mozilla.org/mozilla-central/rev/b6bfe95b76bb