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