Lower priority of HTTP requests coming from tracking scripts

RESOLVED FIXED in Firefox 55

Status

()

Core
Networking
P2
normal
RESOLVED FIXED
10 months ago
3 months ago

People

(Reporter: Ehsan, Assigned: kershaw)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(3 attachments, 4 obsolete attachments)

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.
See Also: → bug 1141814

Comment 1

10 months ago
Will <script> (without async/defer) or document.write no longer block loading for tracking scripts?
Honza - putting this on your radar, as it seems to fit in the context-driven priority project.
Flags: needinfo?(honzab.moz)
Whiteboard: [necko-next]
I know about this bug.  Thanks.
Flags: needinfo?(honzab.moz)
(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.
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?
(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.
Blocks: 1312741
No longer blocks: 1302270
Priority: -- → P2
Assignee: nobody → kechang
(Assignee)

Comment 7

4 months ago
Created attachment 8864469 [details] [diff] [review]
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.
Attachment #8864469 - Flags: review?(amarchesini)
(Assignee)

Updated

4 months ago
Status: NEW → ASSIGNED
Whiteboard: [necko-next] → [necko-active]
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.
Attachment #8864469 - Flags: review?(amarchesini) → review-
(Assignee)

Comment 9

3 months ago
Created attachment 8865819 [details] [diff] [review]
Part1: Lower the channel's priority for XHR, v2

Summary:
 - Use Preferences::AddBoolVarCache
Attachment #8864469 - Attachment is obsolete: true
Attachment #8865819 - Flags: review?(amarchesini)
Attachment #8865819 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 10

3 months ago
Created attachment 8865823 [details] [diff] [review]
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?
Flags: needinfo?(ehsan)
Attachment #8865823 - Flags: review?(amarchesini)
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'.
Attachment #8865823 - Flags: review?(amarchesini) → review+
(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.
Flags: needinfo?(ehsan)
(Assignee)

Comment 13

3 months ago
Created attachment 8869362 [details] [diff] [review]
Part1: Lower the channel's priority for XHR, r=baku

Summary:
 - Rebase
 - Add IsLowerNetworkPriority in nsContentUtils to read preference "privacy.trackingprotection.lower_network_priority".
Attachment #8865819 - Attachment is obsolete: true
Attachment #8865823 - Attachment is obsolete: true
Attachment #8869362 - Flags: review+
(Assignee)

Comment 14

3 months ago
Created attachment 8869363 [details] [diff] [review]
Part2: Lower the channel's priority for Fetch, r=baku

Rebase.
Attachment #8869363 - Flags: review+
(Assignee)

Comment 15

3 months ago
Created attachment 8869939 [details] [diff] [review]
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.
Attachment #8869939 - Flags: review?(amarchesini)
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.
Attachment #8869939 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 17

3 months ago
Created attachment 8869984 [details] [diff] [review]
Part3: Test case, r=baku

Fix review comments.

Thanks for the review.
Attachment #8869939 - Attachment is obsolete: true
Attachment #8869984 - Flags: review+
(Assignee)

Comment 18

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=86fa2e4820e0306891842b97d7204378787dcf8d
Keywords: checkin-needed

Comment 19

3 months ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 20

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a6de4c33aa5b
https://hg.mozilla.org/mozilla-central/rev/45ee0803b090
https://hg.mozilla.org/mozilla-central/rev/b6bfe95b76bb
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.