Lower priority of HTTP requests coming from tracking scripts

RESOLVED FIXED in Firefox 55

Status

()

P2
normal
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: Ehsan, Assigned: kershaw)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
See Also: → bug 1141814
Will <script> (without async/defer) or document.write no longer block loading for tracking scripts?

Comment 2

2 years ago
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)
(Reporter)

Comment 4

2 years ago
(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.
(Reporter)

Comment 5

2 years ago
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

2 years 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

2 years 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

2 years 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

2 years 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+
(Reporter)

Comment 12

2 years ago
(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

2 years 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

2 years 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

2 years 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

2 years 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+

Comment 19

2 years 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

2 years 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: 2 years 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.