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)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: kershaw)
References
(Blocks 1 open bug)
Details
(Whiteboard: [necko-active])
Attachments
(3 files, 4 obsolete files)
7.37 KB,
patch
|
kershaw
:
review+
|
Details | Diff | Splinter Review |
6.99 KB,
patch
|
kershaw
:
review+
|
Details | Diff | Splinter Review |
8.83 KB,
patch
|
kershaw
:
review+
|
Details | Diff | Splinter Review |
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•8 years 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]
Reporter | ||
Comment 4•8 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•8 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?
![]() |
||
Comment 6•8 years ago
|
||
(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.
Updated•8 years ago
|
![]() |
||
Updated•8 years ago
|
Priority: -- → P2
![]() |
||
Updated•8 years ago
|
Assignee: nobody → kechang
Assignee | ||
Comment 7•7 years ago
|
||
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•7 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [necko-next] → [necko-active]
Comment 8•7 years ago
|
||
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•7 years ago
|
||
Summary: - Use Preferences::AddBoolVarCache
Attachment #8864469 -
Attachment is obsolete: true
Attachment #8865819 -
Flags: review?(amarchesini)
Updated•7 years ago
|
Attachment #8865819 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 10•7 years ago
|
||
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 11•7 years ago
|
||
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•7 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•7 years ago
|
||
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 15•7 years ago
|
||
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 16•7 years ago
|
||
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•7 years ago
|
||
Fix review comments. Thanks for the review.
Attachment #8869939 -
Attachment is obsolete: true
Attachment #8869984 -
Flags: review+
Assignee | ||
Comment 18•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=86fa2e4820e0306891842b97d7204378787dcf8d
Keywords: checkin-needed
Comment 19•7 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•7 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
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 21•9 days ago
|
||
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.
See Also: → 1915186
You need to log in
before you can comment on or make changes to this bug.
Description
•