Timer: Set data member before scheduling task

Also increase tolerance for unit tests

Bug: 116081383
Test: Run unit test
Change-Id: I5531c2259cbbc58b6ee7527813e8d5c4164e94c5

2 files changed, 32 insertions, 38 deletions

diff --git a/common/timer.cc b/common/timer.cc
index f35b955b9..f02a67dad 100644
--- a/common/timer.cc
+++ b/common/timer.cc
@@ -66,6 +66,9 @@ bool Timer::ScheduleTaskHelper(const base::WeakPtr<MessageLoopThread>& thread,
   expected_time_next_task_us_ = time_next_task_us;
   task_ = std::move(task);
   task_wrapper_.Reset(base::Bind(&Timer::RunTask, base::Unretained(this)));
+  message_loop_thread_ = thread;
+  period_ = delay;
+  is_periodic_ = is_periodic;
   uint64_t time_until_next_us = time_next_task_us - time_get_os_boottime_us();
   if (!thread->DoInThreadDelayed(
           from_here, task_wrapper_.callback(),
@@ -75,11 +78,11 @@ bool Timer::ScheduleTaskHelper(const base::WeakPtr<MessageLoopThread>& thread,
                << ", from " << from_here.ToString();
     expected_time_next_task_us_ = 0;
     task_wrapper_.Cancel();
+    message_loop_thread_ = nullptr;
+    period_ = {};
+    is_periodic_ = false;
     return false;
   }
-  message_loop_thread_ = thread;
-  period_ = delay;
-  is_periodic_ = is_periodic;
   return true;
 }
 
diff --git a/common/timer_unittest.cc b/common/timer_unittest.cc
index 8ae85896d..d855895d5 100644
--- a/common/timer_unittest.cc
+++ b/common/timer_unittest.cc
@@ -134,6 +134,7 @@ TEST_F(TimerTest, schedule_task) {
   EXPECT_TRUE(timer_->IsScheduled());
   future.get();
   ASSERT_EQ(name, my_name);
+  std::this_thread::sleep_for(std::chrono::milliseconds(delay_error_ms));
   EXPECT_FALSE(timer_->IsScheduled());
 }
 
@@ -173,7 +174,7 @@ TEST_F(TimerTest, periodic_run) {
                  num_tasks, promise_),
       base::TimeDelta::FromMilliseconds(delay_ms));
   future.get();
-  ASSERT_EQ(counter_, num_tasks);
+  ASSERT_GE(counter_, num_tasks);
   timer_->CancelAndWait();
 }
 
@@ -288,43 +289,28 @@ TEST_F(TimerTest, schedule_multiple_delayed_slow_tasks_stress) {
   VerifyMultipleDelayedTasks(100, 10, 20);
 }
 
-// Verify that when MessageLoopThread is shutdown, the pending task will be
-// cancelled
-TEST_F(TimerTest, message_loop_thread_down_cancel_pending_task) {
+TEST_F(TimerTest, message_loop_thread_down_cancel_scheduled_periodic_task) {
   std::string name = "test_thread";
   MessageLoopThread message_loop_thread(name);
   message_loop_thread.StartUp();
   std::string my_name;
+  auto future = promise_->get_future();
   uint32_t delay_ms = 5;
+  int num_tasks = 5;
 
-  timer_->Schedule(
-      message_loop_thread.GetWeakPtr(), FROM_HERE,
-      base::Bind(&TimerTest::ShouldNotHappen, base::Unretained(this)),
-      base::TimeDelta::FromMilliseconds(delay_ms));
-  EXPECT_TRUE(timer_->IsScheduled());
-  std::this_thread::sleep_for(std::chrono::milliseconds(delay_ms - 3));
-  message_loop_thread.ShutDown();
-  std::this_thread::sleep_for(std::chrono::milliseconds(delay_ms));
-}
-
-// Verify that when MessageLoopThread is shutdown, the pending periodic task
-// will be cancelled
-TEST_F(TimerTest, message_loop_thread_down_cancel_pending_periodic_task) {
-  std::string name = "test_thread";
-  MessageLoopThread message_loop_thread(name);
-  message_loop_thread.StartUp();
-  uint32_t delay_ms = 5;
-
-  timer_->Schedule(
+  timer_->SchedulePeriodic(
       message_loop_thread.GetWeakPtr(), FROM_HERE,
-      base::Bind(&TimerTest::ShouldNotHappen, base::Unretained(this)),
+      base::Bind(&TimerTest::IncreaseTaskCounter, base::Unretained(this),
+                 num_tasks, promise_),
       base::TimeDelta::FromMilliseconds(delay_ms));
-  EXPECT_TRUE(timer_->IsScheduled());
-  std::this_thread::sleep_for(std::chrono::milliseconds(delay_ms - 2));
+  future.wait();
   message_loop_thread.ShutDown();
-  timer_->CancelAndWait();
-  EXPECT_FALSE(timer_->IsScheduled());
-  std::this_thread::sleep_for(std::chrono::milliseconds(delay_ms));
+  std::this_thread::sleep_for(
+      std::chrono::milliseconds(delay_ms + delay_error_ms));
+  int counter = counter_;
+  std::this_thread::sleep_for(
+      std::chrono::milliseconds(delay_ms + delay_error_ms));
+  ASSERT_EQ(counter, counter_);
 }
 
 TEST_F(TimerTest, schedule_task_cancel_previous_task) {
@@ -334,18 +320,23 @@ TEST_F(TimerTest, schedule_task_cancel_previous_task) {
   std::string my_name;
   auto future = promise_->get_future();
   uint32_t delay_ms = 5;
+  int num_tasks = 5;
 
   timer_->SchedulePeriodic(
       message_loop_thread.GetWeakPtr(), FROM_HERE,
-      base::Bind(&TimerTest::ShouldNotHappen, base::Unretained(this)),
+      base::Bind(&TimerTest::IncreaseTaskCounter, base::Unretained(this),
+                 num_tasks, promise_),
       base::TimeDelta::FromMilliseconds(delay_ms));
-  std::this_thread::sleep_for(std::chrono::milliseconds(delay_ms - 2));
+  future.wait();
   timer_->Schedule(message_loop_thread.GetWeakPtr(), FROM_HERE,
-                   base::Bind(&TimerTest::GetName, base::Unretained(this),
-                              &my_name, promise_),
+                   base::Bind([]() {}),
                    base::TimeDelta::FromMilliseconds(delay_ms));
-  future.wait();
-  ASSERT_EQ(name, my_name);
+  std::this_thread::sleep_for(
+      std::chrono::milliseconds(delay_ms + delay_error_ms));
+  int counter = counter_;
+  std::this_thread::sleep_for(
+      std::chrono::milliseconds(delay_ms + delay_error_ms));
+  ASSERT_EQ(counter, counter_);
 }
 
 TEST_F(TimerTest, reschedule_task_dont_invoke_new_task_early) {