Enable throttling of tracking timeouts by default

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM
RESOLVED FIXED
4 months ago
3 months 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.

Comment 1

4 months ago
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

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

Comment 4

4 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

4 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.

Comment 6

4 months ago
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

4 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

4 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

4 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

3 months ago
Keywords: checkin-needed

Comment 12

3 months 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

Comment 13

3 months ago
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

3 months 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

3 months ago
Depends on: 1364858
(Assignee)

Comment 15

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

Updated

3 months ago
Keywords: checkin-needed

Comment 16

3 months 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

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6411a4abcc1a
Status: NEW → RESOLVED
Last Resolved: 3 months 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

Updated

3 months ago
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

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

Comment 21

3 months 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: 3 months ago3 months 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.