1371787 - dom.min_timeout_value is poorly named and over applied in TimeoutManager.cpp

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Rename dom.min_timeout_value to dom.min_clamp_timeout_value. r=ehsan

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Don't adjust Timeout::When() values in TimeoutManager::Resume(). r=ehsan

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Move some Timeout initialization earlier. r=ehsan

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Pass Timeout to DOMMinTimeoutValue(). r=ehsan

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Move the Timeout conditional checking into DOMMinTimeoutValue(). r=ehsan

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Move the std::max() calculation into DOMMinTimeoutValue() and rename the method to CalculateDelay(). r=ehsan

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Make CalculateDelay() return a TimeDuration. r=ehsan

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Allow TimeDuration to be passed to CalculateDelay. r=ehsan

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P9 Change Timeout::mInterval member to a TimeDuration. r=ehsan

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5aa16fc3f2fdd6571e9ab50f32f5d816e028422

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

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().

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 20

[(no longer active)]

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.

Comment 21

[(no longer active)]

(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...)

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 23

[(no longer active)]

(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!

Comment 24

[(no longer active)]

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

Comment 25

[(no longer active)]

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.

Comment 26

[(no longer active)]

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!  :-)

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

We're not changing the pref, so no dev-doc-needed.

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Rename TimeoutManager "min timeout" values to "min clamp timeout" for clarity. r=ehsan

Ok, this leaves the pref name alone, but changes the names within TimeoutManager.cpp to better reflect what its supposed to do now.

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Don't adjust Timeout::When() values in TimeoutManager::Resume(). r=ehsan

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Move some Timeout initialization earlier. r=ehsan

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Pass Timeout to DOMMinTimeoutValue(). r=ehsan

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Move the Timeout conditional checking into DOMMinTimeoutValue(). r=ehsan

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Move the std::max() calculation into DOMMinTimeoutValue() and rename the method to CalculateDelay(). r=ehsan

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Make CalculateDelay() return a TimeDuration. r=ehsan

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Change Timeout::mInterval member to a TimeDuration. r=ehsan

https://treeherder.mozilla.org/#/jobs?repo=try&revision=79130959cf2c065ba469de83cbb0a76fba98f0d3

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 38

[Pulsebot]

Pushed by [e-mail]:
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

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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