Closed Bug 1378394 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox54 --- unaffected
firefox55 + fixed
firefox56 + fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

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
This should fix the problem, but I need to write a test to prevent future regressions.
Slightly better version of the patch which should make it more explicit what is going on and also avoids an extra TimeStamp::Now() call.
Attachment #8883587 - Attachment is obsolete: true
This is a web compat regression in FF55.  We should uplift the fix for this before it hits the release channel.
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.
Attachment #8883592 - Flags: review?(afarre)
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.
Attachment #8883618 - Flags: review?(afarre)
Attachment #8883592 - Flags: review?(afarre) → review+
Attachment #8883618 - Flags: review?(afarre) → review+
Pushed by bkelly@mozilla.com:
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 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
Attachment #8883592 - Flags: approval-mozilla-beta?
Comment on attachment 8883618 [details] [diff] [review]
P2 Test that setInterval() is calculated based on start of last callback. r=farre

See comment 11.
Attachment #8883618 - Flags: approval-mozilla-beta?
Comment on attachment 8883592 [details] [diff] [review]
P1 Calculate next setInterval() time from start of previous callback. r=farre

webcompat fix for beta55
Attachment #8883592 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8883618 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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')
Flags: needinfo?(bkelly)
(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.
Flags: qe-verify-
Depends on: 1398934
Depends on: 1487778
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.