Closed Bug 1312515 Opened 8 years ago Closed 7 years ago

Lower priority of HTTP requests coming from tracking scripts

Categories

(Core :: Networking, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: kershaw)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [necko-active])

Attachments

(3 files, 4 obsolete files)

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.
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: CDP
No longer blocks: QuantumDOM
Priority: -- → P2
Assignee: nobody → kechang
Attached patch Part1: Lower the channel's priority (obsolete) (deleted) — Splinter Review
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)
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-
Attached patch Part1: Lower the channel's priority for XHR, v2 (obsolete) (deleted) — Splinter Review
Summary: - Use Preferences::AddBoolVarCache
Attachment #8864469 - Attachment is obsolete: true
Attachment #8865819 - Flags: review?(amarchesini)
Attachment #8865819 - Flags: review?(amarchesini) → review+
Attached patch Part2: Lower the channel's priority for Fetch (obsolete) (deleted) — Splinter Review
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)
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+
Rebase.
Attachment #8869363 - Flags: review+
Attached patch Part3: Test case (obsolete) (deleted) — Splinter Review
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+
Attached patch Part3: Test case, r=baku (deleted) — Splinter Review
Fix review comments. Thanks for the review.
Attachment #8869939 - Attachment is obsolete: true
Attachment #8869984 - Flags: review+
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: