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 2 open bugs)
Details
(Whiteboard: [necko-active])
Attachments
(3 files, 4 obsolete files)
(deleted),
patch
|
kershaw
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
kershaw
:
review+
|
Details | Diff | Splinter Review |
(deleted),
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.
Reporter | ||
Updated•8 years ago
|
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•8 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•8 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [necko-next] → [necko-active]
Comment 8•8 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•8 years ago
|
||
Summary:
- Use Preferences::AddBoolVarCache
Attachment #8864469 -
Attachment is obsolete: true
Attachment #8865819 -
Flags: review?(amarchesini)
Updated•8 years ago
|
Attachment #8865819 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 10•8 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•8 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•8 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
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•