1From 07763c630b9e6ee4538a86d291bfce8357dec934 Mon Sep 17 00:00:00 2001
2From: Hidehiko Abe <[email protected]>
3Date: Thu, 13 Jun 2019 22:12:42 +0900
4Subject: [PATCH] Refactor AlarmTimer to report error to the caller.
5
6This is to address the comment
7https://chromium-review.googlesource.com/c/aosp/platform/external/libchrome/+/1387893/2/components/timers/alarm_timer_chromeos.h#45
8
9Instead of just upstreaming it, which exposes a method unused in Chromium,
10this CL introduces Create() factory method and drops the code to
11fallback in case of timerfd_create failure.
12
13Now, a caller should check whether Create() returns null or not.
14In case of null, to keep the previous behavior, the caller can
15create an instance of the parent class.
16
17Along with the change, in order to run unittest for the class
18without capability CAP_WAKE_ALARM, this CL also introduces CreateForTesting().
19We used to test just fall-back code paths.
20
21In addition, this CL fixes the FD leak on destruction.
22
23This patch modified the original upstream patch, by
24 - keeping the old constructor for backward-compatibility.
25 - keeping CanWakeFromSuspend, and calls of CanWakeFromSuspend from Stop
26   and Reset, for backward-compatibility, so that unittest won't fail
27   due to -1 alarm_fd_ from default constructor when there's no
28   capability to alarm.
29
30BUG=None
31TEST=Build and run components_unittests --gtest_filter=AlarmTimerTest*. Run try.
32
33Change-Id: Ieb9660335120565ccff7f192d7a8192ca1e59ebc
34Reviewed-on: https://chromium-review.googlesource.com/c/1411356
35Reviewed-by: Ryo Hashimoto <[email protected]>
36Reviewed-by: Dmitry Titov <[email protected]>
37Reviewed-by: Dan Erat <[email protected]>
38Commit-Queue: Hidehiko Abe <[email protected]>
39Cr-Commit-Position: refs/heads/master@{#626151}
40---
41 components/timers/alarm_timer_chromeos.cc | 51 ++++++++++++++---------
42 components/timers/alarm_timer_chromeos.h  | 27 +++++++-----
43 2 files changed, 49 insertions(+), 29 deletions(-)
44
45diff --git a/components/timers/alarm_timer_chromeos.cc b/components/timers/alarm_timer_chromeos.cc
46index 0b43134..371eb67 100644
47--- a/components/timers/alarm_timer_chromeos.cc
48+++ b/components/timers/alarm_timer_chromeos.cc
49@@ -15,13 +15,43 @@
50 #include "base/debug/task_annotator.h"
51 #include "base/files/file_util.h"
52 #include "base/logging.h"
53+#include "base/memory/ptr_util.h"
54 #include "base/pending_task.h"
55 #include "base/trace_event/trace_event.h"
56
57 namespace timers {
58
59+// Keep for backward compatibility to uprev r576279.
60 SimpleAlarmTimer::SimpleAlarmTimer()
61     : alarm_fd_(timerfd_create(CLOCK_REALTIME_ALARM, 0)), weak_factory_(this) {}
62+// static
63+std::unique_ptr<SimpleAlarmTimer> SimpleAlarmTimer::Create() {
64+  return CreateInternal(CLOCK_REALTIME_ALARM);
65+}
66+
67+// static
68+std::unique_ptr<SimpleAlarmTimer> SimpleAlarmTimer::CreateForTesting() {
69+  // For unittest, use CLOCK_REALTIME in order to run the tests without
70+  // CAP_WAKE_ALARM.
71+  return CreateInternal(CLOCK_REALTIME);
72+}
73+
74+// static
75+std::unique_ptr<SimpleAlarmTimer> SimpleAlarmTimer::CreateInternal(
76+    int clockid) {
77+  base::ScopedFD alarm_fd(timerfd_create(clockid, TFD_CLOEXEC));
78+  if (!alarm_fd.is_valid()) {
79+    PLOG(ERROR) << "Failed to create timer fd";
80+    return nullptr;
81+  }
82+
83+  // Note: std::make_unique<> cannot be used because the constructor is
84+  // private.
85+  return base::WrapUnique(new SimpleAlarmTimer(std::move(alarm_fd)));
86+}
87+
88+SimpleAlarmTimer::SimpleAlarmTimer(base::ScopedFD alarm_fd)
89+    : alarm_fd_(std::move(alarm_fd)), weak_factory_(this) {}
90
91 SimpleAlarmTimer::~SimpleAlarmTimer() {
92   DCHECK(origin_task_runner_->RunsTasksInCurrentSequence());
93@@ -80,7 +97,7 @@ void SimpleAlarmTimer::Reset() {
94   alarm_time.it_value.tv_nsec =
95       (delay.InMicroseconds() % base::Time::kMicrosecondsPerSecond) *
96       base::Time::kNanosecondsPerMicrosecond;
97-  if (timerfd_settime(alarm_fd_, 0, &alarm_time, NULL) < 0)
98+  if (timerfd_settime(alarm_fd_.get(), 0, &alarm_time, NULL) < 0)
99     PLOG(ERROR) << "Error while setting alarm time.  Timer will not fire";
100
101   // The timer is running.
102@@ -97,7 +114,7 @@ void SimpleAlarmTimer::Reset() {
103     base::debug::TaskAnnotator().WillQueueTask("SimpleAlarmTimer::Reset",
104                                                pending_task_.get());
105     alarm_fd_watcher_ = base::FileDescriptorWatcher::WatchReadable(
106-        alarm_fd_,
107+        alarm_fd_.get(),
108         base::BindRepeating(&SimpleAlarmTimer::OnAlarmFdReadableWithoutBlocking,
109                             weak_factory_.GetWeakPtr()));
110   }
111@@ -109,7 +126,7 @@ void SimpleAlarmTimer::OnAlarmFdReadableWithoutBlocking() {
112
113   // Read from |alarm_fd_| to ack the event.
114   char val[sizeof(uint64_t)];
115-  if (!base::ReadFromFD(alarm_fd_, val, sizeof(uint64_t)))
116+  if (!base::ReadFromFD(alarm_fd_.get(), val, sizeof(uint64_t)))
117     PLOG(DFATAL) << "Unable to read from timer file descriptor.";
118
119   OnTimerFired();
120diff --git a/components/timers/alarm_timer_chromeos.h b/components/timers/alarm_timer_chromeos.h
121index 1ff689e..e1301e9 100644
122--- a/components/timers/alarm_timer_chromeos.h
123+++ b/components/timers/alarm_timer_chromeos.h
124@@ -8,6 +8,7 @@
125 #include <memory>
126
127 #include "base/files/file_descriptor_watcher_posix.h"
128+#include "base/files/scoped_file.h"
129 #include "base/macros.h"
130 #include "base/memory/scoped_refptr.h"
131 #include "base/memory/weak_ptr.h"
132@@ -24,8 +25,7 @@ namespace timers {
133 // suspended state. For example, this is useful for running tasks that are
134 // needed for maintaining network connectivity, like sending heartbeat messages.
135 // Currently, this feature is only available on Chrome OS systems running linux
136-// version 3.11 or higher. On all other platforms, the SimpleAlarmTimer behaves
137-// exactly the same way as a regular Timer.
138+// version 3.11 or higher.
139 //
140 // A SimpleAlarmTimer instance can only be used from the sequence on which it
141 // was instantiated. Start() and Stop() must be called from a thread that
142@@ -36,7 +36,16 @@ namespace timers {
143 // times but not at a regular interval.
144 class SimpleAlarmTimer : public base::RetainingOneShotTimer {
145  public:
146   SimpleAlarmTimer();
147+  // Creates the SimpleAlarmTimer instance, or returns null on failure, e.g.,
148+  // on a platform without timerfd_* system calls support, or missing
149+  // capability (CAP_WAKE_ALARM).
150+  static std::unique_ptr<SimpleAlarmTimer> Create();
151+
152+  // Similar to Create(), but for unittests without capability.
153+  // Specifically, uses CLOCK_REALTIME instead of CLOCK_REALTIME_ALARM.
154+  static std::unique_ptr<SimpleAlarmTimer> CreateForTesting();
155+
156   ~SimpleAlarmTimer() override;
157
158   // Timer overrides.
159@@ -44,6 +52,11 @@ class SimpleAlarmTimer : public base::RetainingOneShotTimer {
160   void Reset() override;
161
162  private:
163+  // Shared implementation of Create and CreateForTesting.
164+  static std::unique_ptr<SimpleAlarmTimer> CreateInternal(int clockid);
165+
166+  explicit SimpleAlarmTimer(base::ScopedFD alarm_fd);
167+
168   // Called when |alarm_fd_| is readable without blocking. Reads data from
169   // |alarm_fd_| and calls OnTimerFired().
170   void OnAlarmFdReadableWithoutBlocking();
171@@ -61,5 +74,5 @@ class SimpleAlarmTimer : public base::RetainingOneShotTimer {
172   // Timer file descriptor.
173-  const int alarm_fd_;
174+  const base::ScopedFD alarm_fd_;
175
176   // Watches |alarm_fd_|.
177   std::unique_ptr<base::FileDescriptorWatcher::Controller> alarm_fd_watcher_;
178--
1792.22.0.rc2.383.gf4fbbf30c2-goog
180
181