1378394 - setInterval() should be calculated from start of the callback executuion

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

Prior to bug 1363829 we always calculated the next setInterval() execution time from the beginning of the current callback function.  After bug 1363829 we started calculating the next interval based on the end of the callback.

So if you have:

  function burnTime() {
    let start = performance.now();
    while((performance.now() - start) < 1000);
  }

  function interval() {
    console.log('### ### Start: ' + performance.now());
    burnTime();
    console.log('### ### End: ' + performance.now());
  }

  setInterval(interval, 5000)

We should get values like this:

  ### ### Start: 29131.395 
  ### ### End: 30134.16 
  ### ### Start: 34133.020000000004 
  ### ### End: 35134.125 
  ### ### Start: 39129.375 
  ### ### End: 40130.41

There are only 4 seconds from "end" to "start", because the 1 second execution time is included in the interval calculation.

Currently we are getting 5 seconds from "end" to "start" instead:

  ### ### Start: 135146.58000000002
  ### ### End: 136147.77
  ### ### Start: 141148.935
  ### ### End: 142149.935
  ### ### Start: 147156.255
  ### ### End: 148157.05000000002

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Calculate next setInterval() time from start of previous callback. r=farre

This should fix the problem, but I need to write a test to prevent future regressions.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Calculate next setInterval() time from start of previous callback. r=farre

Slightly better version of the patch which should make it more explicit what is going on and also avoids an extra TimeStamp::Now() call.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

This is a web compat regression in FF55.  We should uplift the fix for this before it hits the release channel.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Test that setInterval() is calculated based on start of last callback. r=farre

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

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Test that setInterval() is calculated based on start of last callback. r=farre

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

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Test that setInterval() is calculated based on start of last callback. r=farre

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

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8883592 [details] [diff] [review]
P1 Calculate next setInterval() time from start of previous callback. r=farre

Andreas,

In my previous bug I was a bit over-zealous in updating the "now" timestamp.  In particular, RescheduleTimeout() depends on it not being updated yet in order to properly calculate the delay from the start of the last callback.

This patch refactors RescheduleTimeout() to be more explicit about the timestamps it needs.  I also added some comments to try to explain the situation.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8883618 [details] [diff] [review]
P2 Test that setInterval() is calculated based on start of last callback. r=farre

This test verifies that we calculate the interval delay based on the start of the callback and not the end.  It does this by burning up a lot of time in the callback and then checking when it fires.  I tried to use relatively large delays to avoid intermittent test failures on slow platforms.

If necessary we can disable this on debug/android if it becomes a problem.  I think we should have some kind of test in place, though.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0cdf2fd1fdf
P1 Calculate next setInterval() time from start of previous callback. r=farre
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9f6572b6288
P2 Test that setInterval() is calculated based on start of last callback. r=farre

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c0cdf2fd1fdf
https://hg.mozilla.org/mozilla-central/rev/d9f6572b6288

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8883592 [details] [diff] [review]
P1 Calculate next setInterval() time from start of previous callback. r=farre

Approval Request Comment
[Feature/Bug causing the regression]: Regression caused by bug 1363829
[User impact if declined]: Sites using setInterval() may experience slightly different behavior which could be observable to users.
[Is this code covered by automated tests?]: This bug includes a new test verifying the correct behavior.
[Has the fix been verified in Nightly?]: I have verified its fixed in the latest nightly update.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: Minimal risk
[Why is the change risky/not risky?]: Its a very small change.  We just need to calculate the next sleep delay based on a different TimeStamp.  The new test provides coverage to ensure its working properly.
[String changes made/needed]: None

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8883618 [details] [diff] [review]
P2 Test that setInterval() is calculated based on start of last callback. r=farre

See comment 11.

Comment 13

[Julien Cristau [:jcristau]]

Comment on attachment 8883592 [details] [diff] [review]
P1 Calculate next setInterval() time from start of previous callback. r=farre

webcompat fix for beta55

Comment 14

[Carsten Book [:Tomcat]]

needs a rebased patch for conflicts like

grafting 428175:c0cdf2fd1fdf "Bug 1378394 P1 Calculate next setInterval() time from start of previous callback. r=farre"
merging dom/base/TimeoutManager.cpp
merging dom/base/TimeoutManager.h
warning: conflicts while merging dom/base/TimeoutManager.cpp! (edit, then use 'hg resolve --mark')

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/7c337b70c63049c0bd492b45c2b0e09686d19ebb
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/3d1277c0c560998260a60777541ad4e396800d58

Comment 16

[Andrei Vaida [:avaida]]

(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #11)
> [Is this code covered by automated tests?]: This bug includes a new test
> verifying the correct behavior.
> [Has the fix been verified in Nightly?]: I have verified its fixed in the
> latest nightly update.
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Ben's assessment on manual testing needs and the fact that this fix has automated coverage.