Closed
Bug 1371787
Opened 7 years ago
Closed 7 years ago
dom.min_timeout_value is poorly named and over applied in TimeoutManager.cpp
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: bkelly, Assigned: bkelly)
References
(Blocks 1 open bug)
Details
Attachments
(8 files, 9 obsolete files)
(deleted),
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
The dom.min_timeout_value pref controls our clamping value for deeply nested setTimeout() calls and setInterval(). The name, however, suggests that it applies to all setTimeout() calls. We should really rename it. Also, we are incorrectly applying it to all timeouts being restored from bfcache in TimeoutManager::Thaw(). I noticed this while looking at a test failure in bug 1371664.
Assignee | ||
Comment 1•7 years ago
|
||
Assignee | ||
Comment 2•7 years ago
|
||
Assignee | ||
Comment 3•7 years ago
|
||
Assignee | ||
Comment 4•7 years ago
|
||
Assignee | ||
Comment 5•7 years ago
|
||
Assignee | ||
Comment 6•7 years ago
|
||
Assignee | ||
Comment 7•7 years ago
|
||
Assignee | ||
Comment 8•7 years ago
|
||
Assignee | ||
Comment 9•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5aa16fc3f2fdd6571e9ab50f32f5d816e028422
Assignee | ||
Comment 10•7 years ago
|
||
Comment on attachment 8876295 [details] [diff] [review] P1 Rename dom.min_timeout_value to dom.min_clamp_timeout_value. r=ehsan This patch renames the confusing dom.min_timeout_value to clarify that it only applies to clamped timeouts. Clearly this confused a number of people reading the code in the past.
Attachment #8876295 -
Flags: review?(ehsan)
Assignee | ||
Comment 11•7 years ago
|
||
Comment on attachment 8876296 [details] [diff] [review] P2 Don't adjust Timeout::When() values in TimeoutManager::Resume(). r=ehsan Remove the code in ::Resume() that clamps the minimum delay of all timers. This code has been around for a long time, but doesn't make a lot of sense. Any clamping or throttling is already accounted for within the Timeout::When() value. This code is applying clamping/throttling a second time when the window resumes from being suspended. So this happens after something like a sync XHR or when a window comes out bfcache. I don't think this is necessary since we've already clamped/throttled the delay when the timeout was first scheduled. Removing this code will reduce the number of places we change Timeout::When() which will in turn make it easier to change our timeout list structure. Finally, the code being removed completely ignores that it might change the ordering of the timeouts. That's probably a bug.
Attachment #8876296 -
Flags: review?(ehsan)
Assignee | ||
Comment 12•7 years ago
|
||
Note, the pref is mentioned in this MDN article, so we should update that (assuming the patches are r+'d): https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/setTimeout#Minimum_delay_and_timeout_nesting
Keywords: dev-doc-needed
Assignee | ||
Comment 13•7 years ago
|
||
Comment on attachment 8876297 [details] [diff] [review] P3 Move some Timeout initialization earlier. r=ehsan This is a small refactoring patch that consolidates some initialization earlier in the method. This is in preparation for a later patch where we will pass that Timeout to DOMMinTimeoutValue().
Attachment #8876297 -
Flags: review?(ehsan)
Assignee | ||
Comment 14•7 years ago
|
||
Comment on attachment 8876298 [details] [diff] [review] P4 Pass Timeout to DOMMinTimeoutValue(). r=ehsan Pass the entire Timeout to DOMMinTimeoutValue() instead of just the tracking flag. This is in preparation for doing more of the conditional logic within DOMMinTimeoutValue.
Attachment #8876298 -
Flags: review?(ehsan)
Assignee | ||
Comment 15•7 years ago
|
||
Comment on attachment 8876299 [details] [diff] [review] P5 Move the Timeout conditional checking into DOMMinTimeoutValue(). r=ehsan This patch moves the conditional logic into DOMMinTimeoutValue() instead of requiring the caller to do it. This is very desirable since we've had a number of bugs in the past where the conditionals were not all updated correctly. Note, this also lets us apply the various minimum values at a more granular level. We can apply tracking throttling but not the clamping minimum, etc. In the past we had to apply all the minimums regardless of the reason we entered this method.
Attachment #8876299 -
Flags: review?(ehsan)
Assignee | ||
Comment 16•7 years ago
|
||
Comment on attachment 8876300 [details] [diff] [review] P6 Move the std::max() calculation into DOMMinTimeoutValue() and rename the method to CalculateDelay(). r=ehsan This patch is a refactoring to reduce code duplication. It moves the various std::max() calculations after DOMMinTimeoutValue() into the method. It also renames the method to CalculateDelay() to reflect its new behavior.
Attachment #8876300 -
Flags: review?(ehsan)
Assignee | ||
Comment 17•7 years ago
|
||
Comment on attachment 8876301 [details] [diff] [review] P7 Make CalculateDelay() return a TimeDuration. r=ehsan This is another small refactor to make CalculateDelay() return a TimeDuration. This just de-duplicates some of the conversion code sprinkled around TimeoutManager.
Attachment #8876301 -
Flags: review?(ehsan)
Assignee | ||
Comment 18•7 years ago
|
||
Comment on attachment 8876302 [details] [diff] [review] P8 Allow TimeDuration to be passed to CalculateDelay. r=ehsan This is another refactoring patch to reduce duplicate code. It allows TimeDuration to be passed as the default delay to CalculateDelay(). Right now we also still accept the integer milliseconds value, but that will go away in the next patch.
Attachment #8876302 -
Flags: review?(ehsan)
Assignee | ||
Comment 19•7 years ago
|
||
Comment on attachment 8876303 [details] [diff] [review] P9 Change Timeout::mInterval member to a TimeDuration. r=ehsan This patch makes us store the Timeout::mInterval value as a TimeDuration. We can then pass this directly to CalculateDelay() and remove the integer milliseconds support.
Attachment #8876303 -
Flags: review?(ehsan)
Comment 20•7 years ago
|
||
Comment on attachment 8876295 [details] [diff] [review] P1 Rename dom.min_timeout_value to dom.min_clamp_timeout_value. r=ehsan Review of attachment 8876295 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, but I don't think this is worth the headache. We've had this pref since bug 625256 (landed in 2011) and as you have noted it has been heavily documented, so one side effect of this patch is undoing any customization that users may have in their profiles around this. The other side effect is add-on compat. Looking at the add-ons dxr there aren't tons of add-ons that use this but there were a few shield study add-ons such as https://addons.mozilla.org/en-us/firefox/addon/page-reload-study-1/ which had this pref mentioned in their source code (see <https://dxr.mozilla.org/addons/source/addons/803638/node_modules/shield-studies-addon-utils/jetpack/timers.js#24>). This patch will silently break those due to the try/catch in that code (that is a copy of the timers.js you're modifying here -- it could be that by fixing this code in a better way we can fix those add-ons by getting them repackaged, but I'm not 100% sure how that would work). If you want to do this you should probably reach out to the shield team first and ask them to fix their code. This isn't a super firm r-... I feel kind of bad that we can't fix our code because of reasons like the above, so if you'd like to try to convince me, I'd be open to that! :-) I'm mostly minusing because I think the amount of work involved to do this "properly" exceeds the gains we'd get out of doing it at all, and also I think we could alternatively add some documentation around the appearance of this in all.js and TimeoutManager.cpp to make it clearer what the pref does without changing its name. Let me know what you think.
Attachment #8876295 -
Flags: review?(ehsan) → review-
Comment 21•7 years ago
|
||
(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #11) > Remove the code in ::Resume() that clamps the minimum delay of all timers. > This code has been around for a long time, but doesn't make a lot of sense. > Any clamping or throttling is already accounted for within the > Timeout::When() value. This code is applying clamping/throttling a second > time when the window resumes from being suspended. So this happens after > something like a sync XHR or when a window comes out bfcache. Hmm, I was trying to figure out where this code came from originally, since it's hard to evaluate whether it's OK to take it out now without knowing that. Going back in history I got to bug 1303167 and it's really hard to figure out what happened in that bug by reading the code changes. Do you mind explaining what happened to this code path in that bug, and whether this is new as of that bug or whether it existed before it too? (I mostly don't remember when we used to call ResumeTimeouts() in the old setup...)
Flags: needinfo?(bkelly)
Assignee | ||
Comment 22•7 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #21) > Hmm, I was trying to figure out where this code came from originally, since > it's hard to evaluate whether it's OK to take it out now without knowing > that. Going back in history I got to bug 1303167 and it's really hard to > figure out what happened in that bug by reading the code changes. Do you > mind explaining what happened to this code path in that bug, and whether > this is new as of that bug or whether it existed before it too? (I mostly > don't remember when we used to call ResumeTimeouts() in the old setup...) It does appear I introduced the change to mWhen in bug 1303167. Before that we were just applying the minimum unconditionally to the nsITimer delay. So there are two issues here: 1. Should we be clamping the next firing time of timers at all after we Resume() from sync xhr, bfcache, etc? 2. And if we are clamping the next firing time, should that time be reflected in the Timeout::When() value? AFAICT we have been doing (1) forever: http://searchfox.org/mozilla-central/rev/afdb1c662224eebfe6529f9438759f19d8832857/dom/src/base/nsGlobalWindow.cpp#6225 Here the old code is storing the "time remaining" in mWhen. This code, however, unconditionally limited all timers to this minimum regardless of nesting, etc: http://searchfox.org/mozilla-central/rev/afdb1c662224eebfe6529f9438759f19d8832857/dom/src/base/nsGlobalWindow.cpp#5027 I think the application of the minimum in Resume is a holdover from when we really did have an actual minimum value. It probably should have been at least modified to looking nesting level, background state, etc before applying it when we introduced the clamping depth stuff. Even then, however, it seems like it was overkill. The original mWhen still had the throttling already in place. Applying it again on Resume seems unnecessary. I think (2) is a bug I introduced in bug 1303167. I was confused before we baked the throttled minimum time into mWhen in SetTimeoutOrInterval(), so I thought it was a mistake not to do it in Resume(). How do you want to proceed? I think modifying Timeout::When() here is probably not what we want to do. I also think doing any application of the minimum here is unnecessary and a holdover from ancient code.
Flags: needinfo?(bkelly) → needinfo?(ehsan)
Comment 23•7 years ago
|
||
(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #22) > So there are two issues here: > > 1. Should we be clamping the next firing time of timers at all after we > Resume() from sync xhr, bfcache, etc? I think this was done in order to readjust things in case the background-ness of the window has changed since the last time it went into the bf-cache. But as you mentioned on IRC, with MinSchedulingDelay() we don't need to worry about that any more. > 2. And if we are clamping the next firing time, should that time be > reflected in the Timeout::When() value? > > I think (2) is a bug I introduced in bug 1303167. I was confused before we > baked the throttled minimum time into mWhen in SetTimeoutOrInterval(), so I > thought it was a mistake not to do it in Resume(). Yeah this one indeed looks like a regression from that bug. > How do you want to proceed? I think modifying Timeout::When() here is > probably not what we want to do. I also think doing any application of the > minimum here is unnecessary and a holdover from ancient code. I think the part 2 patch is fine as is, based on your explanation. Thanks!
Flags: needinfo?(ehsan)
Updated•7 years ago
|
Attachment #8876296 -
Flags: review?(ehsan) → review+
Updated•7 years ago
|
Attachment #8876297 -
Flags: review?(ehsan) → review+
Updated•7 years ago
|
Attachment #8876298 -
Flags: review?(ehsan) → review+
Updated•7 years ago
|
Attachment #8876299 -
Flags: review?(ehsan) → review+
Comment 24•7 years ago
|
||
Comment on attachment 8876300 [details] [diff] [review] P6 Move the std::max() calculation into DOMMinTimeoutValue() and rename the method to CalculateDelay(). r=ehsan Review of attachment 8876300 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the below addressed. ::: dom/base/TimeoutManager.cpp @@ +232,5 @@ > // uses 5. > #define DOM_CLAMP_TIMEOUT_NESTING_LEVEL 5 > > int32_t > +TimeoutManager::CalculateDelay(Timeout* aTimeout, int32_t aDefaultDelay) const { I don't think you should pass a "default delay" here. This really is the timeout interval, and since you are passing the timeout object itself it seems unnecessary to pass the interval separately. I think you should change the signature back to: int32_t CalculateDelay(Timeout* aTimeout) const; and make the function take the timeout interval *always* into account. Then, at the MOZ_LOG caller below that is really doing something special, change the call site to: CalculateDelay(timeout) - timeout->mInterval
Attachment #8876300 -
Flags: review?(ehsan) → review+
Updated•7 years ago
|
Attachment #8876301 -
Flags: review?(ehsan) → review+
Comment 25•7 years ago
|
||
Comment on attachment 8876302 [details] [diff] [review] P8 Allow TimeDuration to be passed to CalculateDelay. r=ehsan Review of attachment 8876302 [details] [diff] [review]: ----------------------------------------------------------------- This patch isn't needed per my previous comment.
Attachment #8876302 -
Flags: review?(ehsan) → review-
Comment 26•7 years ago
|
||
Comment on attachment 8876303 [details] [diff] [review] P9 Change Timeout::mInterval member to a TimeDuration. r=ehsan Review of attachment 8876303 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/TimeoutManager.cpp @@ -390,5 @@ > > RefPtr<Timeout> timeout = new Timeout(); > timeout->mWindow = &mWindow; > timeout->mIsInterval = aIsInterval; > - timeout->mInterval = interval; I think based on my previous comment, this patch should turn into only two hunks, this one, and the declaration change! :-)
Attachment #8876303 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 27•7 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #24) > I don't think you should pass a "default delay" here. This really is the > timeout interval, and since you are passing the timeout object itself it > seems unnecessary to pass the interval separately. > > I think you should change the signature back to: > > int32_t CalculateDelay(Timeout* aTimeout) const; > > and make the function take the timeout interval *always* into account. > Then, at the MOZ_LOG caller below that is really doing something special, > change the call site to: > > CalculateDelay(timeout) - timeout->mInterval Ok. I wanted to do that, but the log was the issue. I think when I originally wrote it I still had the Resume() code which uses a different value as well.
Assignee | ||
Comment 28•7 years ago
|
||
We're not changing the pref, so no dev-doc-needed.
Keywords: dev-doc-needed
Assignee | ||
Comment 29•7 years ago
|
||
Ok, this leaves the pref name alone, but changes the names within TimeoutManager.cpp to better reflect what its supposed to do now.
Attachment #8876295 -
Attachment is obsolete: true
Attachment #8878170 -
Flags: review?(ehsan)
Assignee | ||
Comment 30•7 years ago
|
||
Attachment #8876296 -
Attachment is obsolete: true
Attachment #8878171 -
Flags: review+
Assignee | ||
Comment 31•7 years ago
|
||
Attachment #8876297 -
Attachment is obsolete: true
Attachment #8878173 -
Flags: review+
Assignee | ||
Comment 32•7 years ago
|
||
Attachment #8876298 -
Attachment is obsolete: true
Attachment #8878175 -
Flags: review+
Assignee | ||
Comment 34•7 years ago
|
||
Attachment #8876300 -
Attachment is obsolete: true
Attachment #8878178 -
Flags: review+
Assignee | ||
Comment 35•7 years ago
|
||
Attachment #8876301 -
Attachment is obsolete: true
Attachment #8878179 -
Flags: review+
Assignee | ||
Comment 36•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=79130959cf2c065ba469de83cbb0a76fba98f0d3
Attachment #8876302 -
Attachment is obsolete: true
Attachment #8876303 -
Attachment is obsolete: true
Attachment #8878180 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Attachment #8878177 -
Flags: review+
Assignee | ||
Comment 37•7 years ago
|
||
Comment on attachment 8878170 [details] [diff] [review] P1 Rename TimeoutManager "min timeout" values to "min clamp timeout" for clarity. r=ehsan Ehsan gave me r+ on irc for this.
Attachment #8878170 -
Flags: review?(ehsan) → review+
Comment 38•7 years ago
|
||
Pushed by bkelly@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d4caf4187300 P1 Rename TimeoutManager "min timeout" values to "min clamp timeout" for clarity. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/9e6ae8bd143a P2 Don't adjust Timeout::When() values in TimeoutManager::Resume(). r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/a367269bcc9e P3 Move some Timeout initialization earlier. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/66c516526c98 P4 Pass Timeout to DOMMinTimeoutValue(). r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/0775422cb097 P5 Move the Timeout conditional checking into DOMMinTimeoutValue(). r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/a727ed846650 P6 Move the std::max() calculation into DOMMinTimeoutValue() and rename the method to CalculateDelay(). r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/91baba2cd85f P7 Make CalculateDelay() return a TimeDuration. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/4113e070fcbf P8 Change Timeout::mInterval member to a TimeDuration. r=ehsan
![]() |
||
Comment 39•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d4caf4187300 https://hg.mozilla.org/mozilla-central/rev/9e6ae8bd143a https://hg.mozilla.org/mozilla-central/rev/a367269bcc9e https://hg.mozilla.org/mozilla-central/rev/66c516526c98 https://hg.mozilla.org/mozilla-central/rev/0775422cb097 https://hg.mozilla.org/mozilla-central/rev/a727ed846650 https://hg.mozilla.org/mozilla-central/rev/91baba2cd85f https://hg.mozilla.org/mozilla-central/rev/4113e070fcbf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•