Refactor AlarmTimer to report error to the caller.

This is to address the comment
https://chromium-review.googlesource.com/c/aosp/platform/external/libchrome/+/1387893/2/components/timers/alarm_timer_chromeos.h#45

Instead of just upstreaming it, which exposes a method unused in Chromium,
this CL introduces Create() factory method and drops the code to
fallback in case of timerfd_create failure.

Now, a caller should check whether Create() returns null or not.
In case of null, to keep the previous behavior, the caller can
create an instance of the parent class.

Along with the change, in order to run unittest for the class
without capability CAP_WAKE_ALARM, this CL also introduces CreateForTesting().
We used to test just fall-back code paths.

In addition, this CL fixes the FD leak on destruction.

BUG=None
TEST=Build and run components_unittests --gtest_filter=AlarmTimerTest*. Run try.

Change-Id: Ieb9660335120565ccff7f192d7a8192ca1e59ebc
Reviewed-on: https://chromium-review.googlesource.com/c/1411356Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Reviewed-by: Dmitry Titov <dimich@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#626151}

components/gcm_driver/gcm_driver_desktop.cc

@@ -473,8 +473,12 @@ void GCMDriverDesktop::IOWorker::WakeFromSuspendForHeartbeat(bool wake) {
 
   std::unique_ptr<base::RetainingOneShotTimer> timer;
   if (wake)
-    timer = std::make_unique<timers::SimpleAlarmTimer>();
-  else
+    timer = timers::SimpleAlarmTimer::Create();
+
+  // If not |wake|, or SimpleAlarmTimer is not supported on the running
+  // platform (please see SimpleAlarmTimer for the details), fall back to
+  // RetainingOneShotTimer.
+  if (!timer)
     timer = std::make_unique<base::RetainingOneShotTimer>();
 
   gcm_client_->UpdateHeartbeatTimer(std::move(timer));

components/timers/alarm_timer_chromeos.cc

@@ -15,14 +15,40 @@
 #include "base/debug/task_annotator.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
+#include "base/memory/ptr_util.h"
 #include "base/pending_task.h"
 #include "base/trace_event/trace_event.h"
 
 namespace timers {
 
-SimpleAlarmTimer::SimpleAlarmTimer()
-    : alarm_fd_(timerfd_create(CLOCK_REALTIME_ALARM, TFD_CLOEXEC)),
-      weak_factory_(this) {}
+// static
+std::unique_ptr<SimpleAlarmTimer> SimpleAlarmTimer::Create() {
+  return CreateInternal(CLOCK_REALTIME_ALARM);
+}
+
+// static
+std::unique_ptr<SimpleAlarmTimer> SimpleAlarmTimer::CreateForTesting() {
+  // For unittest, use CLOCK_REALTIME in order to run the tests without
+  // CAP_WAKE_ALARM.
+  return CreateInternal(CLOCK_REALTIME);
+}
+
+// static
+std::unique_ptr<SimpleAlarmTimer> SimpleAlarmTimer::CreateInternal(
+    int clockid) {
+  base::ScopedFD alarm_fd(timerfd_create(clockid, TFD_CLOEXEC));
+  if (!alarm_fd.is_valid()) {
+    PLOG(ERROR) << "Failed to create timer fd";
+    return nullptr;
+  }
+
+  // Note: std::make_unique<> cannot be used because the constructor is
+  // private.
+  return base::WrapUnique(new SimpleAlarmTimer(std::move(alarm_fd)));
+}
+
+SimpleAlarmTimer::SimpleAlarmTimer(base::ScopedFD alarm_fd)
+    : alarm_fd_(std::move(alarm_fd)), weak_factory_(this) {}
 
 SimpleAlarmTimer::~SimpleAlarmTimer() {
   DCHECK(origin_task_runner_->RunsTasksInCurrentSequence());
@@ -35,11 +61,6 @@ void SimpleAlarmTimer::Stop() {
   if (!IsRunning())
     return;
 
-  if (!CanWakeFromSuspend()) {
-    base::RetainingOneShotTimer::Stop();
-    return;
-  }
-
   // Cancel any previous callbacks.
   weak_factory_.InvalidateWeakPtrs();
 
@@ -52,11 +73,6 @@ void SimpleAlarmTimer::Reset() {
   DCHECK(origin_task_runner_->RunsTasksInCurrentSequence());
   DCHECK(!base::RetainingOneShotTimer::user_task().is_null());
 
-  if (!CanWakeFromSuspend()) {
-    base::RetainingOneShotTimer::Reset();
-    return;
-  }
-
   // Cancel any previous callbacks and stop watching |alarm_fd_|.
   weak_factory_.InvalidateWeakPtrs();
   alarm_fd_watcher_.reset();
@@ -81,7 +97,7 @@ void SimpleAlarmTimer::Reset() {
   alarm_time.it_value.tv_nsec =
       (delay.InMicroseconds() % base::Time::kMicrosecondsPerSecond) *
       base::Time::kNanosecondsPerMicrosecond;
-  if (timerfd_settime(alarm_fd_, 0, &alarm_time, NULL) < 0)
+  if (timerfd_settime(alarm_fd_.get(), 0, &alarm_time, NULL) < 0)
     PLOG(ERROR) << "Error while setting alarm time.  Timer will not fire";
 
   // The timer is running.
@@ -98,7 +114,7 @@ void SimpleAlarmTimer::Reset() {
     base::debug::TaskAnnotator().WillQueueTask("SimpleAlarmTimer::Reset",
                                                pending_task_.get());
     alarm_fd_watcher_ = base::FileDescriptorWatcher::WatchReadable(
-        alarm_fd_,
+        alarm_fd_.get(),
         base::BindRepeating(&SimpleAlarmTimer::OnAlarmFdReadableWithoutBlocking,
                             weak_factory_.GetWeakPtr()));
   }
@@ -110,7 +126,7 @@ void SimpleAlarmTimer::OnAlarmFdReadableWithoutBlocking() {
 
   // Read from |alarm_fd_| to ack the event.
   char val[sizeof(uint64_t)];
-  if (!base::ReadFromFD(alarm_fd_, val, sizeof(uint64_t)))
+  if (!base::ReadFromFD(alarm_fd_.get(), val, sizeof(uint64_t)))
     PLOG(DFATAL) << "Unable to read from timer file descriptor.";
 
   OnTimerFired();
@@ -137,8 +153,4 @@ void SimpleAlarmTimer::OnTimerFired() {
     Stop();
 }
 
-bool SimpleAlarmTimer::CanWakeFromSuspend() const {
-  return alarm_fd_ != -1;
-}
-
 }  // namespace timers

components/timers/alarm_timer_chromeos.h

@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "base/files/file_descriptor_watcher_posix.h"
+#include "base/files/scoped_file.h"
 #include "base/macros.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
@@ -24,8 +25,7 @@ namespace timers {
 // suspended state. For example, this is useful for running tasks that are
 // needed for maintaining network connectivity, like sending heartbeat messages.
 // Currently, this feature is only available on Chrome OS systems running linux
-// version 3.11 or higher. On all other platforms, the SimpleAlarmTimer behaves
-// exactly the same way as a regular Timer.
+// version 3.11 or higher.
 //
 // A SimpleAlarmTimer instance can only be used from the sequence on which it
 // was instantiated. Start() and Stop() must be called from a thread that
@@ -36,7 +36,15 @@ namespace timers {
 // times but not at a regular interval.
 class SimpleAlarmTimer : public base::RetainingOneShotTimer {
  public:
-  SimpleAlarmTimer();
+  // Creates the SimpleAlarmTimer instance, or returns null on failure, e.g.,
+  // on a platform without timerfd_* system calls support, or missing
+  // capability (CAP_WAKE_ALARM).
+  static std::unique_ptr<SimpleAlarmTimer> Create();
+
+  // Similar to Create(), but for unittests without capability.
+  // Specifically, uses CLOCK_REALTIME instead of CLOCK_REALTIME_ALARM.
+  static std::unique_ptr<SimpleAlarmTimer> CreateForTesting();
+
   ~SimpleAlarmTimer() override;
 
   // Timer overrides.
@@ -44,6 +52,11 @@ class SimpleAlarmTimer : public base::RetainingOneShotTimer {
   void Reset() override;
 
  private:
+  // Shared implementation of Create and CreateForTesting.
+  static std::unique_ptr<SimpleAlarmTimer> CreateInternal(int clockid);
+
+  explicit SimpleAlarmTimer(base::ScopedFD alarm_fd);
+
   // Called when |alarm_fd_| is readable without blocking. Reads data from
   // |alarm_fd_| and calls OnTimerFired().
   void OnAlarmFdReadableWithoutBlocking();
@@ -51,14 +64,8 @@ class SimpleAlarmTimer : public base::RetainingOneShotTimer {
   // Called when the timer fires. Runs the callback.
   void OnTimerFired();
 
-  // Tracks whether the timer has the ability to wake the system up from
-  // suspend. This is a runtime check because we won't know if the system
-  // supports being woken up from suspend until the constructor actually tries
-  // to set it up.
-  bool CanWakeFromSuspend() const;
-
   // Timer file descriptor.
-  const int alarm_fd_;
+  const base::ScopedFD alarm_fd_;
 
   // Watches |alarm_fd_|.
   std::unique_ptr<base::FileDescriptorWatcher::Controller> alarm_fd_watcher_;

components/timers/alarm_timer_unittest.cc

@@ -24,7 +24,9 @@
 // regular Timer so it should pass the same tests as the Timer class.
 namespace timers {
 namespace {
-const base::TimeDelta kTenMilliseconds = base::TimeDelta::FromMilliseconds(10);
+
+constexpr base::TimeDelta kTenMilliseconds =
+    base::TimeDelta::FromMilliseconds(10);
 
 class AlarmTimerTester {
  public:
@@ -34,7 +36,7 @@ class AlarmTimerTester {
       : did_run_(did_run),
         quit_closure_(std::move(quit_closure)),
         delay_(delay),
-        timer_(new timers::SimpleAlarmTimer()) {}
+        timer_(SimpleAlarmTimer::CreateForTesting()) {}
   void Start() {
     timer_->Start(
         FROM_HERE, delay_,
@@ -51,7 +53,7 @@ class AlarmTimerTester {
   bool* did_run_;
   base::OnceClosure quit_closure_;
   const base::TimeDelta delay_;
-  std::unique_ptr<timers::SimpleAlarmTimer> timer_;
+  std::unique_ptr<SimpleAlarmTimer> timer_;
 
   DISALLOW_COPY_AND_ASSIGN(AlarmTimerTester);
 };
@@ -64,7 +66,7 @@ class SelfDeletingAlarmTimerTester {
       : did_run_(did_run),
         quit_closure_(std::move(quit_closure)),
         delay_(delay),
-        timer_(new timers::SimpleAlarmTimer()) {}
+        timer_(SimpleAlarmTimer::CreateForTesting()) {}
   void Start() {
     timer_->Start(FROM_HERE, delay_,
                   base::BindRepeating(&SelfDeletingAlarmTimerTester::Run,
@@ -83,7 +85,7 @@ class SelfDeletingAlarmTimerTester {
   bool* did_run_;
   base::OnceClosure quit_closure_;
   const base::TimeDelta delay_;
-  std::unique_ptr<timers::SimpleAlarmTimer> timer_;
+  std::unique_ptr<SimpleAlarmTimer> timer_;
 
   DISALLOW_COPY_AND_ASSIGN(SelfDeletingAlarmTimerTester);
 };
@@ -221,49 +223,47 @@ TEST(AlarmTimerTest, MessageLoopShutdown) {
 }
 
 TEST(AlarmTimerTest, NonRepeatIsRunning) {
-  {
-    base::test::ScopedTaskEnvironment task_environment(
-        base::test::ScopedTaskEnvironment::MainThreadType::IO);
+  base::test::ScopedTaskEnvironment task_environment(
+      base::test::ScopedTaskEnvironment::MainThreadType::IO);
 
-    timers::SimpleAlarmTimer timer;
-    EXPECT_FALSE(timer.IsRunning());
-    timer.Start(FROM_HERE, base::TimeDelta::FromDays(1), base::DoNothing());
+  auto timer = SimpleAlarmTimer::CreateForTesting();
+  EXPECT_FALSE(timer->IsRunning());
+  timer->Start(FROM_HERE, base::TimeDelta::FromDays(1), base::DoNothing());
 
-    // Allow FileDescriptorWatcher to start watching the timer. Without this, a
-    // task posted by FileDescriptorWatcher::WatchReadable() is leaked.
-    base::RunLoop().RunUntilIdle();
+  // Allow FileDescriptorWatcher to start watching the timer. Without this, a
+  // task posted by FileDescriptorWatcher::WatchReadable() is leaked.
+  base::RunLoop().RunUntilIdle();
 
-    EXPECT_TRUE(timer.IsRunning());
-    timer.Stop();
-    EXPECT_FALSE(timer.IsRunning());
-    ASSERT_FALSE(timer.user_task().is_null());
-    timer.Reset();
-    base::RunLoop().RunUntilIdle();
-    EXPECT_TRUE(timer.IsRunning());
-  }
+  EXPECT_TRUE(timer->IsRunning());
+  timer->Stop();
+  EXPECT_FALSE(timer->IsRunning());
+  ASSERT_FALSE(timer->user_task().is_null());
+  timer->Reset();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(timer->IsRunning());
 }
 
 TEST(AlarmTimerTest, RetainNonRepeatIsRunning) {
   base::test::ScopedTaskEnvironment task_environment(
       base::test::ScopedTaskEnvironment::MainThreadType::IO);
 
-  timers::SimpleAlarmTimer timer;
-  EXPECT_FALSE(timer.IsRunning());
-  timer.Start(FROM_HERE, base::TimeDelta::FromDays(1), base::DoNothing());
+  auto timer = SimpleAlarmTimer::CreateForTesting();
+  EXPECT_FALSE(timer->IsRunning());
+  timer->Start(FROM_HERE, base::TimeDelta::FromDays(1), base::DoNothing());
 
   // Allow FileDescriptorWatcher to start watching the timer. Without this, a
   // task posted by FileDescriptorWatcher::WatchReadable() is leaked.
   base::RunLoop().RunUntilIdle();
 
-  EXPECT_TRUE(timer.IsRunning());
-  timer.Reset();
+  EXPECT_TRUE(timer->IsRunning());
+  timer->Reset();
   base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(timer.IsRunning());
-  timer.Stop();
-  EXPECT_FALSE(timer.IsRunning());
-  timer.Reset();
+  EXPECT_TRUE(timer->IsRunning());
+  timer->Stop();
+  EXPECT_FALSE(timer->IsRunning());
+  timer->Reset();
   base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(timer.IsRunning());
+  EXPECT_TRUE(timer->IsRunning());
 }
 
 namespace {
@@ -293,16 +293,16 @@ TEST(AlarmTimerTest, ContinuationStopStart) {
   base::test::ScopedTaskEnvironment task_environment(
       base::test::ScopedTaskEnvironment::MainThreadType::IO);
 
-  timers::SimpleAlarmTimer timer;
-  timer.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(10),
-              base::BindRepeating(&SetCallbackHappened1,
-                                  base::DoNothing().Repeatedly()));
-  timer.Stop();
+  auto timer = SimpleAlarmTimer::CreateForTesting();
+  timer->Start(FROM_HERE, base::TimeDelta::FromMilliseconds(10),
+               base::BindRepeating(&SetCallbackHappened1,
+                                   base::DoNothing().Repeatedly()));
+  timer->Stop();
 
   base::RunLoop run_loop;
-  timer.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(40),
-              base::BindRepeating(&SetCallbackHappened2,
-                                  run_loop.QuitWhenIdleClosure()));
+  timer->Start(FROM_HERE, base::TimeDelta::FromMilliseconds(40),
+               base::BindRepeating(&SetCallbackHappened2,
+                                   run_loop.QuitWhenIdleClosure()));
   run_loop.Run();
 
   EXPECT_FALSE(g_callback_happened1);
@@ -315,12 +315,12 @@ TEST(AlarmTimerTest, ContinuationReset) {
       base::test::ScopedTaskEnvironment::MainThreadType::IO);
 
   base::RunLoop run_loop;
-  timers::SimpleAlarmTimer timer;
-  timer.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(10),
-              base::BindRepeating(&SetCallbackHappened1,
-                                  run_loop.QuitWhenIdleClosure()));
-  timer.Reset();
-  ASSERT_FALSE(timer.user_task().is_null());
+  auto timer = SimpleAlarmTimer::CreateForTesting();
+  timer->Start(FROM_HERE, base::TimeDelta::FromMilliseconds(10),
+               base::BindRepeating(&SetCallbackHappened1,
+                                   run_loop.QuitWhenIdleClosure()));
+  timer->Reset();
+  ASSERT_FALSE(timer->user_task().is_null());
   run_loop.Run();
   EXPECT_TRUE(g_callback_happened1);
 }
@@ -334,16 +334,13 @@ TEST(AlarmTimerTest, DeleteTimerWhileCallbackIsRunning) {
   base::RunLoop run_loop;
 
   // Will be deleted by the callback.
-  timers::SimpleAlarmTimer* timer = new timers::SimpleAlarmTimer;
-
-  timer->Start(
+  auto timer = SimpleAlarmTimer::CreateForTesting();
+  auto* timer_ptr = timer.get();
+  timer_ptr->Start(
       FROM_HERE, base::TimeDelta::FromMilliseconds(10),
-      base::BindRepeating(
-          [](timers::SimpleAlarmTimer* timer, base::RunLoop* run_loop) {
-            delete timer;
-            run_loop->Quit();
-          },
-          timer, &run_loop));
+      base::BindRepeating([](std::unique_ptr<SimpleAlarmTimer> timer,
+                             base::RunLoop* run_loop) { run_loop->Quit(); },
+                          base::Passed(std::move(timer)), &run_loop));
   run_loop.Run();
 }
 
@@ -355,16 +352,13 @@ TEST(AlarmTimerTest, DeleteTimerWhileCallbackIsRunningZeroDelay) {
   base::RunLoop run_loop;
 
   // Will be deleted by the callback.
-  timers::SimpleAlarmTimer* timer = new timers::SimpleAlarmTimer;
-
-  timer->Start(
+  auto timer = SimpleAlarmTimer::CreateForTesting();
+  auto* timer_ptr = timer.get();
+  timer_ptr->Start(
       FROM_HERE, base::TimeDelta(),
-      base::BindRepeating(
-          [](timers::SimpleAlarmTimer* timer, base::RunLoop* run_loop) {
-            delete timer;
-            run_loop->Quit();
-          },
-          timer, &run_loop));
+      base::BindRepeating([](std::unique_ptr<SimpleAlarmTimer> timer,
+                             base::RunLoop* run_loop) { run_loop->Quit(); },
+                          base::Passed(std::move(timer)), &run_loop));
   run_loop.Run();
 }