Enable throttling of tracking timeouts by default

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM
RESOLVED FIXED
3 months ago
a month ago

People

(Reporter: Ehsan, Assigned: farre)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla55
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

We'll use this bug to track the work to enable the work done in bug 1325467 by default.
Seems like maybe we should fix bug 1339909 before enabling this?  Or are you not going to use that feature?
Depends on: 1339909
Andreas, can you take a look at ThrottleTrackingTimeoutsCallback bug 1339909? It looks like Ehsan posted a patch last month, but Ben asked for some changes.
Flags: needinfo?(afarre)
(Assignee)

Comment 3

3 months ago
On it.
Flags: needinfo?(afarre)
(Assignee)

Comment 4

2 months ago
Created attachment 8864124 [details] [diff] [review]
0001-Bug-1355311-Set-default-values-for-throttling-backgr.patch
Attachment #8864124 - Flags: review?(ehsan)

Comment 5

2 months ago
I'd wish we had prefs for this and enable this also in foreground tabs in nightly.
That way we'll see hopefully sooner whether this causes regressions.
A pref would be nice.  It makes it a lot easier to handle problems in the wild since we can flip it via a system addon without cutting a new release.
(Assignee)

Comment 7

2 months ago
Created attachment 8864142 [details] [diff] [review]
0001-Bug-1355311-Set-default-values-for-throttling-backgr.patch

Accidentally almost turned on foreground throttling as well. Changed it to be on using an ifdef NIGHTLY_BUILD.

With the current setup this is controlled by the hidden prefs and default values:

* dom.timeout.tracking_throttling_delay: 30,000 (30 seconds)
* dom.min_tracking_timeout_value:
    if nightly then 10,000 (10 seconds) else dom.min_timeout_value
* dom.min_tracking_background_timeout_value: 10,000 (10 seconds)
Attachment #8864124 - Attachment is obsolete: true
Attachment #8864124 - Flags: review?(ehsan)
Attachment #8864142 - Flags: review?(ehsan)
(Assignee)

Comment 8

2 months ago
Created attachment 8864225 [details] [diff] [review]
0001-Bug-1355311-Set-default-values-for-throttling-backgr.patch
Attachment #8864142 - Attachment is obsolete: true
Attachment #8864142 - Flags: review?(ehsan)
Attachment #8864225 - Flags: review?(ehsan)
Comment on attachment 8864225 [details] [diff] [review]
0001-Bug-1355311-Set-default-values-for-throttling-backgr.patch

Review of attachment 8864225 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Please make sure an intent to ship email is sent indicating that this is being turned on for Nightly...

::: modules/libpref/init/all.js
@@ +1210,4 @@
>  pref("dom.min_timeout_value", 4);
>  // And for background windows
>  pref("dom.min_background_timeout_value", 1000);
> +// Timeout clamp in ms for tracking timeouts we clamp

Please add a comment saying that the prefs below will only have an effect if the privacy.trackingprotection.annotate_channels pref is set to true.  You can copy the comment that we have for the privacy.trackingprotection.lower_network_priority already.
Attachment #8864225 - Flags: review?(ehsan) → review+
(Assignee)

Comment 10

2 months ago
Created attachment 8864508 [details] [diff] [review]
0001-Bug-1355311-Set-default-values-for-throttling-backgr.patch

Added comment about privacy.trackingprotection.annotate_channels pref. Carrying over r+.
Attachment #8864225 - Attachment is obsolete: true
Attachment #8864508 - Flags: review+
The throttling will potentially effect web developers, so this change needs to be covered by documentation.

For document team: reference this email thread, which is full of very useful information: http://bit.ly/2qe9vhZ
Keywords: dev-doc-needed
(Assignee)

Updated

a month ago
Keywords: checkin-needed

Comment 12

a month ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0542e8e2295b
Set default values for throttling background timeouts. r=ehsan
Keywords: checkin-needed
sorry had to back this out for assertion failures in TimeoutManager.cpp like https://treeherder.mozilla.org/logviewer.html#?job_id=99078033&repo=mozilla-inbound&lineNumber=8458
Flags: needinfo?(afarre)

Comment 14

a month ago
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ff22abe5736
Backed out changeset 0542e8e2295b for assertion failure in TimeoutManager.cpp
(Assignee)

Updated

a month ago
Depends on: 1364858
(Assignee)

Comment 15

a month ago
Thanks :ihsiao, I'm on it.
Flags: needinfo?(afarre)
(Assignee)

Updated

a month ago
Keywords: checkin-needed

Comment 16

a month ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6411a4abcc1a
Set default values for throttling background timeouts. r=ehsan
Keywords: checkin-needed

Comment 17

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6411a4abcc1a
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
I've added notes (and updated browser compat info) on:

https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/setTimeout#Throttling_of_tracking_timeout_scripts
https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/setInterval#Throttling_of_intervals

I've also added a note to the Fx55 rel notes page:

https://developer.mozilla.org/en-US/Firefox/Releases/55#DOM_HTML_DOM

Let me know if this reads OK, or if you think anything else is required. Thanks!
Keywords: dev-doc-needed → dev-doc-complete
Blocks: 1366812
Backed out for causing crash in mozilla::dom::TimeoutManager::MaybeStartThrottleTrackingTimout (bug 1366812):

https://hg.mozilla.org/mozilla-central/rev/f9ca97a334296facd2e0ea5582e7f12d0fe70fe4
Status: RESOLVED → REOPENED
Flags: needinfo?(afarre)
Resolution: FIXED → ---
(Assignee)

Updated

a month ago
Depends on: 1367025
Flags: needinfo?(afarre)
Relanding because bug 1367025 may have fixed the cause of the crashes.

Comment 21

a month ago
Pushed by wchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecb5a09e6371
Set default values for throttling background timeouts. r=ehsan
https://hg.mozilla.org/mozilla-central/rev/ecb5a09e6371
Status: REOPENED → RESOLVED
Last Resolved: a month agoa month ago
Resolution: --- → FIXED
No longer blocks: 1366812
Depends on: 1366812
You need to log in before you can comment on or make changes to this bug.