commit aa20c5818e3d5c609b49c7b346214262f069e2bf
author Alec Mouri <alecmouri@google.com> Wed Oct 11 19:44:54 2023 +0000
committer Alec Mouri <alecmouri@google.com> Thu Oct 12 22:03:33 2023 +0000
tree 09d8fdade92145ec8218f518de03db6091a7eddd
parent a3ba81193d5a61aa7db8a5846392b85c7d382648

Clear out callback when canceling an alarm

VSyncDispatchTimerQueue tries to cancel its timer callback when
destroying itself, but Timekeeper might still run the callback if
disarming its timer happened immediately after polling but before
invoking the callback. To avoid this race condition, delete the callback
while holding Timekeeper's mutex.

Bug: 304675978
Test: builds
Change-Id: Ifa7a4fcb65481b6a5ddfcfeb6bc7a53cb37ec168

services/surfaceflinger/Scheduler/include/scheduler/Timer.h

diff --git a/services/surfaceflinger/Scheduler/include/scheduler/Timer.h b/services/surfaceflinger/Scheduler/include/scheduler/Timer.h
index 58ad6cb..67f7abe 100644
--- a/services/surfaceflinger/Scheduler/include/scheduler/Timer.h
+++ b/services/surfaceflinger/Scheduler/include/scheduler/Timer.h
@@ -60,6 +60,7 @@
     void reset() EXCLUDES(mMutex);
     void cleanup() REQUIRES(mMutex);
     void setDebugState(DebugState) EXCLUDES(mMutex);
+    void setCallback(std::function<void()>&&) REQUIRES(mMutex);
 
     int mTimerFd = -1;
 
@@ -71,6 +72,7 @@
     void endDispatch();
 
     mutable std::mutex mMutex;
+
     std::function<void()> mCallback GUARDED_BY(mMutex);
     bool mExpectingCallback GUARDED_BY(mMutex) = false;
     DebugState mDebugState GUARDED_BY(mMutex);

services/surfaceflinger/Scheduler/src/Timer.cpp

diff --git a/services/surfaceflinger/Scheduler/src/Timer.cpp b/services/surfaceflinger/Scheduler/src/Timer.cpp
index a4cf57f..09e8a1e 100644
--- a/services/surfaceflinger/Scheduler/src/Timer.cpp
+++ b/services/surfaceflinger/Scheduler/src/Timer.cpp
@@ -93,8 +93,8 @@
         close(mPipes[kWritePipe]);
         mPipes[kWritePipe] = -1;
     }
-    mExpectingCallback = false;
-    mCallback = {};
+
+    setCallback({});
 }
 
 void Timer::endDispatch() {
@@ -112,8 +112,7 @@
     static constexpr int ns_per_s =
             std::chrono::duration_cast<std::chrono::nanoseconds>(1s).count();
 
-    mCallback = std::move(callback);
-    mExpectingCallback = true;
+    setCallback(std::move(callback));
 
     struct itimerspec old_timer;
     struct itimerspec new_timer {
@@ -142,6 +141,8 @@
     if (timerfd_settime(mTimerFd, 0, &new_timer, &old_timer)) {
         ALOGW("Failed to disarm timerfd");
     }
+
+    setCallback({});
 }
 
 void Timer::threadMain() {
@@ -231,6 +232,11 @@
     mDebugState = state;
 }
 
+void Timer::setCallback(std::function<void()>&& callback) {
+    mExpectingCallback = bool(callback);
+    mCallback = std::move(callback);
+}
+
 void Timer::dump(std::string& result) const {
     std::lock_guard lock(mMutex);
     result.append("\t\tDebugState: ");