Closed Bug 1355311 Opened 6 years ago Closed 6 years ago

Enable throttling of tracking timeouts by default

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: farre)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 3 obsolete files)

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)
On it.
Flags: needinfo?(afarre)
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.
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)
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+
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
Keywords: checkin-needed
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)
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ff22abe5736
Backed out changeset 0542e8e2295b for assertion failure in TimeoutManager.cpp
Depends on: 1364858
Thanks :ihsiao, I'm on it.
Flags: needinfo?(afarre)
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/6411a4abcc1a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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 → ---
Depends on: 1367025
Flags: needinfo?(afarre)
Relanding because bug 1367025 may have fixed the cause of the crashes.
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
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.