Ben Kelly [:bkelly, not reviewing] Assignee
	Description • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 1 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 2 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 3 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 4 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 5 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 6 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 7 • 7 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 8 • 7 years ago

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.

	Pulsebot
	Comment 9 • 7 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 10 • 7 years ago
bugherder

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 11 • 7 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 12 • 7 years ago

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

See comment 11.

	Julien Cristau [:jcristau]
	Comment 13 • 7 years ago

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

webcompat fix for beta55

	Carsten Book [:Tomcat]
	Comment 14 • 7 years ago

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')

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 15 • 7 years ago

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

	Andrei Vaida [:avaida]
	Comment 16 • 7 years ago

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