Fix the drift in repeating timers (try #2)

third_party/WebKit/Source/platform/TimerTest.cpp

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "config.h"
 #include "platform/Timer.h"
 
 #include "public/platform/Platform.h"
 #include "public/platform/WebScheduler.h"
 #include "public/platform/WebThread.h"
@@ skipping 23 matching lines @@
     {
         m_task->run();
     }
 
 private:
     OwnPtr<WebTaskRunner::Task> m_task;
 };
 
 class DelayedTask {
 public:
-    DelayedTask(WebTaskRunner::Task* task, long long delayMs)
+    DelayedTask(WebTaskRunner::Task* task, double delaySecs)

[esprehn, 2015/09/28 23:34:43]

delaySeconds

[alex clarke (OOO till 29th), 2015/09/29 10:58:36]

Done.

         : m_task(adoptRef(new RefCountedTaskContainer(task)))
-        , m_runTimeSecs(monotonicallyIncreasingTime() + 0.001 * static_cast<double>(delayMs))
-        , m_delayMs(delayMs) { }
+        , m_runTimeSecs(monotonicallyIncreasingTime() + delaySecs)
+        , m_delaySecs(delaySecs) { }

[esprehn, 2015/09/28 23:34:43]

m_delaySeconds

[alex clarke (OOO till 29th), 2015/09/29 10:58:36]

Done.

 
     bool operator<(const DelayedTask& other) const
     {
         return m_runTimeSecs > other.m_runTimeSecs;
     }
 
     void run() const
     {
         m_task->run();
     }
 
     double runTimeSecs() const
     {
         return m_runTimeSecs;
     }
 
-    long long delayMs() const
+    double delaySecs() const

[esprehn, 2015/09/28 23:34:43]

ditto, don't abbreviate

[alex clarke (OOO till 29th), 2015/09/29 10:58:36]

Done.

     {
-        return m_delayMs;
+        return m_delaySecs;
     }
 
 private:
     RefPtr<RefCountedTaskContainer> m_task;
     double m_runTimeSecs;

[esprehn, 2015/09/28 23:34:43]

This one should be fixed too

[alex clarke (OOO till 29th), 2015/09/29 10:58:36]

Done.

-    long long m_delayMs;
+    double m_delaySecs;
 };
 
 class MockWebTaskRunner : public WebTaskRunner {
 public:
     explicit MockWebTaskRunner(std::priority_queue<DelayedTask>* timerTasks) : m_timerTasks(timerTasks) { }
     ~MockWebTaskRunner() override { }
 
     virtual void postTask(const WebTraceLocation&, Task* task)
     {
         m_timerTasks->push(DelayedTask(task, 0));
     }
 
-    void postDelayedTask(const WebTraceLocation&, Task* task, long long delayMs) override
+    void postDelayedTask(const WebTraceLocation&, Task* task, double delayMs) override
     {
-        m_timerTasks->push(DelayedTask(task, delayMs));
+        m_timerTasks->push(DelayedTask(task, delayMs * 0.001));
     }
 
     std::priority_queue<DelayedTask>* m_timerTasks; // NOT OWNED
 };
 
 class MockWebScheduler : public WebScheduler {
 public:
     MockWebScheduler() : m_timerWebTaskRunner(&m_timerTasks) { }
     ~MockWebScheduler() override { }
 
@@ skipping 50 matching lines @@
             if (m_timerTasks.top().runTimeSecs() > deadline) {
                 gCurrentTimeSecs = deadline;
                 break;
             }
             gCurrentTimeSecs = m_timerTasks.top().runTimeSecs();
             m_timerTasks.top().run();
             m_timerTasks.pop();
         }
     }
 
+    void runPendingTasks()
+    {
+        while (!m_timerTasks.empty() && m_timerTasks.top().runTimeSecs() <= gCurrentTimeSecs) {
+            m_timerTasks.top().run();
+            m_timerTasks.pop();
+        }
+    }
+
     bool hasOneTimerTask() const
     {
         return m_timerTasks.size() == 1;
     }
 
-    long nextTimerTaskDelayMillis() const
+    double nextTimerTaskDelaySecs() const
     {
         ASSERT(hasOneTimerTask());
-        return m_timerTasks.top().delayMs();
+        return m_timerTasks.top().delaySecs();
     }
 
 private:
     std::priority_queue<DelayedTask> m_timerTasks;
     MockWebTaskRunner m_timerWebTaskRunner;
 };
 
 class FakeWebThread : public WebThread {
 public:
     FakeWebThread() : m_webScheduler(adoptPtr(new MockWebScheduler())) { }
@@ skipping 56 matching lines @@
     {
         static const unsigned char enabled[] = {0};
         return enabled;
     }
 
     void runUntilIdle()
     {
         mockScheduler()->runUntilIdle();
     }
 
+    void runPendingTasks()
+    {
+        mockScheduler()->runPendingTasks();
+    }
+
     void runUntilIdleOrDeadlinePassed(double deadline)
     {
         mockScheduler()->runUntilIdleOrDeadlinePassed(deadline);
     }
 
     bool hasOneTimerTask() const
     {
         return mockScheduler()->hasOneTimerTask();
     }
 
-    long nextTimerTaskDelayMillis() const
+    double nextTimerTaskDelaySecs() const
     {
-        return mockScheduler()->nextTimerTaskDelayMillis();
+        return mockScheduler()->nextTimerTaskDelaySecs();
     }
 
 private:
     MockWebScheduler* mockScheduler() const
     {
         return static_cast<MockWebScheduler*>(m_webThread->scheduler());
     }
 
     OwnPtr<FakeWebThread> m_webThread;
 };
@@ skipping 15 matching lines @@
     void TearDown() override
     {
         Platform::initialize(m_oldPlatform);
     }
 
     void countingTask(Timer<TimerTest>*)
     {
         m_runTimes.push_back(monotonicallyIncreasingTime());
     }
 
+    void recordNextFireTimeTask(Timer<TimerTest>* timer)
+    {
+        m_nextFireTimes.push_back(monotonicallyIncreasingTime() + timer->nextFireInterval());
+    }
+
     void advanceTimeBy(double timeSecs)
     {
         gCurrentTimeSecs += timeSecs;
     }
 
     void runUntilIdle()
     {
         m_platform->runUntilIdle();
     }
 
+    void runPendingTasks()
+    {
+        m_platform->runPendingTasks();
+    }
+
     void runUntilIdleOrDeadlinePassed(double deadline)
     {
         m_platform->runUntilIdleOrDeadlinePassed(deadline);
     }
 
     bool hasOneTimerTask() const
     {
         return m_platform->hasOneTimerTask();
     }
 
-    long nextTimerTaskDelayMillis() const
+    double nextTimerTaskDelaySecs() const
     {
-        return m_platform->nextTimerTaskDelayMillis();
+        return m_platform->nextTimerTaskDelaySecs();
     }
 
 protected:
     double m_startTime;
     std::vector<double> m_runTimes;
+    std::vector<double> m_nextFireTimes;

[esprehn, 2015/09/28 23:34:43]

These should be WTF::Vector, we don't use std::vector in blink. Both of these
should be fixed.

[alex clarke (OOO till 29th), 2015/09/29 10:58:36]

I've added a todo.

 
 private:
     OwnPtr<TimerTestPlatform> m_platform;
     Platform* m_oldPlatform;
 };
 
 TEST_F(TimerTest, StartOneShot_Zero)
 {
     Timer<TimerTest> timer(this, &TimerTest::countingTask);
     timer.startOneShot(0, FROM_HERE);
 
     ASSERT(hasOneTimerTask());
-    EXPECT_EQ(0ll, nextTimerTaskDelayMillis());
+    EXPECT_FLOAT_EQ(0.0, nextTimerTaskDelaySecs());
 
     runUntilIdle();
     EXPECT_THAT(m_runTimes, ElementsAre(m_startTime));
 }
 
 TEST_F(TimerTest, StartOneShot_ZeroAndCancel)
 {
     Timer<TimerTest> timer(this, &TimerTest::countingTask);
     timer.startOneShot(0, FROM_HERE);
 
     ASSERT(hasOneTimerTask());
-    EXPECT_EQ(0ll, nextTimerTaskDelayMillis());
+    EXPECT_FLOAT_EQ(0.0, nextTimerTaskDelaySecs());
 
     timer.stop();
 
     runUntilIdle();
     EXPECT_TRUE(m_runTimes.empty());
 }
 
 TEST_F(TimerTest, StartOneShot_ZeroAndCancelThenRepost)
 {
     Timer<TimerTest> timer(this, &TimerTest::countingTask);
     timer.startOneShot(0, FROM_HERE);
 
     ASSERT(hasOneTimerTask());
-    EXPECT_EQ(0ll, nextTimerTaskDelayMillis());
+    EXPECT_FLOAT_EQ(0.0, nextTimerTaskDelaySecs());
 
     timer.stop();
 
     runUntilIdle();
     EXPECT_TRUE(m_runTimes.empty());
 
     timer.startOneShot(0, FROM_HERE);
 
     ASSERT(hasOneTimerTask());
-    EXPECT_EQ(0ll, nextTimerTaskDelayMillis());
+    EXPECT_FLOAT_EQ(0.0, nextTimerTaskDelaySecs());
 
     runUntilIdle();
     EXPECT_THAT(m_runTimes, ElementsAre(m_startTime));
 }
 
 TEST_F(TimerTest, StartOneShot_Zero_RepostingAfterRunning)
 {
     Timer<TimerTest> timer(this, &TimerTest::countingTask);
     timer.startOneShot(0, FROM_HERE);
 
     ASSERT(hasOneTimerTask());
-    EXPECT_EQ(0ll, nextTimerTaskDelayMillis());
+    EXPECT_FLOAT_EQ(0.0, nextTimerTaskDelaySecs());
 
     runUntilIdle();
     EXPECT_THAT(m_runTimes, ElementsAre(m_startTime));
 
     timer.startOneShot(0, FROM_HERE);
 
     ASSERT(hasOneTimerTask());
-    EXPECT_EQ(0ll, nextTimerTaskDelayMillis());
+    EXPECT_FLOAT_EQ(0.0, nextTimerTaskDelaySecs());
 
     runUntilIdle();
     EXPECT_THAT(m_runTimes, ElementsAre(m_startTime, m_startTime));
 }
 
 TEST_F(TimerTest, StartOneShot_NonZero)
 {
     Timer<TimerTest> timer(this, &TimerTest::countingTask);
     timer.startOneShot(10.0, FROM_HERE);
 
     ASSERT(hasOneTimerTask());
-    EXPECT_EQ(10000ll, nextTimerTaskDelayMillis());
+    EXPECT_FLOAT_EQ(10.0, nextTimerTaskDelaySecs());
 
     runUntilIdle();
     EXPECT_THAT(m_runTimes, ElementsAre(m_startTime + 10.0));
 }
 
 TEST_F(TimerTest, StartOneShot_NonZeroAndCancel)
 {
     Timer<TimerTest> timer(this, &TimerTest::countingTask);
     timer.startOneShot(10, FROM_HERE);
 
     ASSERT(hasOneTimerTask());
-    EXPECT_EQ(10000ll, nextTimerTaskDelayMillis());
+    EXPECT_FLOAT_EQ(10.0, nextTimerTaskDelaySecs());
 
     timer.stop();
 
     runUntilIdle();
     EXPECT_TRUE(m_runTimes.empty());
 }
 
 TEST_F(TimerTest, StartOneShot_NonZeroAndCancelThenRepost)
 {
     Timer<TimerTest> timer(this, &TimerTest::countingTask);
     timer.startOneShot(10, FROM_HERE);
 
     ASSERT(hasOneTimerTask());
-    EXPECT_EQ(10000ll, nextTimerTaskDelayMillis());
+    EXPECT_FLOAT_EQ(10.0, nextTimerTaskDelaySecs());
 
     timer.stop();
 
     runUntilIdle();
     EXPECT_TRUE(m_runTimes.empty());
 
     double secondPostTime = monotonicallyIncreasingTime();
     timer.startOneShot(10, FROM_HERE);
 
     ASSERT(hasOneTimerTask());
-    EXPECT_EQ(10000ll, nextTimerTaskDelayMillis());
+    EXPECT_FLOAT_EQ(10.0, nextTimerTaskDelaySecs());
 
     runUntilIdle();
     EXPECT_THAT(m_runTimes, ElementsAre(secondPostTime + 10.0));
 }
 
 TEST_F(TimerTest, StartOneShot_NonZero_RepostingAfterRunning)
 {
     Timer<TimerTest> timer(this, &TimerTest::countingTask);
     timer.startOneShot(10, FROM_HERE);
 
     ASSERT(hasOneTimerTask());
-    EXPECT_EQ(10000ll, nextTimerTaskDelayMillis());
+    EXPECT_FLOAT_EQ(10.0, nextTimerTaskDelaySecs());
 
     runUntilIdle();
     EXPECT_THAT(m_runTimes, ElementsAre(m_startTime + 10.0));
 
     timer.startOneShot(20, FROM_HERE);
 
     ASSERT(hasOneTimerTask());
-    EXPECT_EQ(20000ll, nextTimerTaskDelayMillis());
+    EXPECT_FLOAT_EQ(20.0, nextTimerTaskDelaySecs());
 
     runUntilIdle();
     EXPECT_THAT(m_runTimes, ElementsAre(m_startTime + 10.0, m_startTime + 30.0));
 }
 
 TEST_F(TimerTest, PostingTimerTwiceWithSameRunTimeDoesNothing)
 {
     Timer<TimerTest> timer(this, &TimerTest::countingTask);
     timer.startOneShot(10, FROM_HERE);
     timer.startOneShot(10, FROM_HERE);
 
     ASSERT(hasOneTimerTask());
-    EXPECT_EQ(10000ll, nextTimerTaskDelayMillis());
+    EXPECT_FLOAT_EQ(10.0, nextTimerTaskDelaySecs());
 
     runUntilIdle();
     EXPECT_THAT(m_runTimes, ElementsAre(m_startTime + 10.0));
 }
 
 TEST_F(TimerTest, PostingTimerTwiceWithNewerRunTimeCancelsOriginalTask)
 {
     Timer<TimerTest> timer(this, &TimerTest::countingTask);
     timer.startOneShot(10, FROM_HERE);
     timer.startOneShot(0, FROM_HERE);
@@ skipping 11 matching lines @@
     runUntilIdle();
     EXPECT_THAT(m_runTimes, ElementsAre(m_startTime + 10.0));
 }
 
 TEST_F(TimerTest, StartRepeatingTask)
 {
     Timer<TimerTest> timer(this, &TimerTest::countingTask);
     timer.startRepeating(1.0, FROM_HERE);
 
     ASSERT(hasOneTimerTask());
-    EXPECT_EQ(1000ll, nextTimerTaskDelayMillis());
+    EXPECT_FLOAT_EQ(1.0, nextTimerTaskDelaySecs());
 
     runUntilIdleOrDeadlinePassed(m_startTime + 5.5);
     EXPECT_THAT(m_runTimes, ElementsAre(
         m_startTime + 1.0, m_startTime + 2.0, m_startTime + 3.0, m_startTime + 4.0, m_startTime + 5.0));
 }
 
 TEST_F(TimerTest, StartRepeatingTask_ThenCancel)
 {
     Timer<TimerTest> timer(this, &TimerTest::countingTask);
     timer.startRepeating(1.0, FROM_HERE);
 
     ASSERT(hasOneTimerTask());
-    EXPECT_EQ(1000ll, nextTimerTaskDelayMillis());
+    EXPECT_FLOAT_EQ(1.0, nextTimerTaskDelaySecs());
 
     runUntilIdleOrDeadlinePassed(m_startTime + 2.5);
     EXPECT_THAT(m_runTimes, ElementsAre(m_startTime + 1.0, m_startTime + 2.0));
 
     timer.stop();
     runUntilIdle();
 
     EXPECT_THAT(m_runTimes, ElementsAre(m_startTime + 1.0, m_startTime + 2.0));
 }
 
 TEST_F(TimerTest, StartRepeatingTask_ThenPostOneShot)
 {
     Timer<TimerTest> timer(this, &TimerTest::countingTask);
     timer.startRepeating(1.0, FROM_HERE);
 
     ASSERT(hasOneTimerTask());
-    EXPECT_EQ(1000ll, nextTimerTaskDelayMillis());
+    EXPECT_FLOAT_EQ(1.0, nextTimerTaskDelaySecs());
 
     runUntilIdleOrDeadlinePassed(m_startTime + 2.5);
     EXPECT_THAT(m_runTimes, ElementsAre(m_startTime + 1.0, m_startTime + 2.0));
 
     timer.startOneShot(0, FROM_HERE);
     runUntilIdle();
 
     EXPECT_THAT(m_runTimes, ElementsAre(m_startTime + 1.0, m_startTime + 2.0, m_startTime + 2.5));
 }
 
@@ skipping 203 matching lines @@
     EXPECT_FLOAT_EQ(m_startTime, timer.lastFireTime());
 
     timer.setAlignedFireTime(m_startTime);
     timer.didChangeAlignmentInterval(monotonicallyIncreasingTime());
 
     EXPECT_FLOAT_EQ(0.0, timer.nextFireInterval());
     EXPECT_FLOAT_EQ(0.0, timer.nextUnalignedFireInterval());
     EXPECT_FLOAT_EQ(m_startTime, timer.lastFireTime());
 }
 
+TEST_F(TimerTest, RepeatingTimerDoesNotDrift)
+{
+    Timer<TimerTest> timer(this, &TimerTest::recordNextFireTimeTask);
+    timer.startRepeating(2.0, FROM_HERE);
+
+    ASSERT(hasOneTimerTask());
+    recordNextFireTimeTask(&timer); // Next scheduled task to run at m_startTime + 2.0
+
+    // Simulate timer firing early. Next scheduled task to run at m_startTime + 4.0
+    advanceTimeBy(1.9);
+    runUntilIdleOrDeadlinePassed(gCurrentTimeSecs + 0.2);
+
+    advanceTimeBy(2.0);
+    runPendingTasks(); // Next scheduled task to run at m_startTime + 6.0
+
+    advanceTimeBy(2.1);
+    runPendingTasks(); // Next scheduled task to run at m_startTime + 8.0
+
+    advanceTimeBy(2.9);
+    runPendingTasks(); // Next scheduled task to run at m_startTime + 10.0
+
+    advanceTimeBy(3.1);
+    runPendingTasks(); // Next scheduled task to run at m_startTime + 14.0 (skips a beat)
+
+    advanceTimeBy(4.0);
+    runPendingTasks(); // Next scheduled task to run at m_startTime + 18.0 (skips a beat)
+
+    advanceTimeBy(10.0); // Next scheduled task to run at m_startTime + 28.0 (skips 5 beats)
+    runPendingTasks();
+
+    runUntilIdleOrDeadlinePassed(m_startTime + 5.5);
+    EXPECT_THAT(m_nextFireTimes, ElementsAre(
+        m_startTime + 2.0,
+        m_startTime + 4.0,
+        m_startTime + 6.0,
+        m_startTime + 8.0,
+        m_startTime + 10.0,
+        m_startTime + 14.0,
+        m_startTime + 18.0,
+        m_startTime + 28.0));
+}
 
 } // namespace
 } // namespace blink