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

Comment 21

[Andrew Creskey [:acreskey]]

This has been nightly-only since it landed ~7 years ago, as far as I can tell.
In bug 1915186 we found some cases were this logic was lowering the priority of key resources that were not tracking scripts and so disabling this for the moment.