From c0578b912871d97250fb8f6cbf6a68cc046d7014 Mon Sep 17 00:00:00 2001
From: Tri Vo <trong@google.com>
Date: Wed, 10 Oct 2018 15:29:48 -0700
Subject: [PATCH] Fix deadlock when callback modifies callback list.

Bug: 117555757
Test: SystemSuspendUnitTest
Change-Id: I40e7ffcc1bbc744a5b9e6d739166f1eebb8ed151
---
 suspend/1.0/default/SystemSuspend.cpp         | 11 +++++++++--
 suspend/1.0/default/SystemSuspendUnitTest.cpp | 23 +++++++++++++++++++++++
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/suspend/1.0/default/SystemSuspend.cpp b/suspend/1.0/default/SystemSuspend.cpp
index 5f70671..eae240a 100644
--- a/suspend/1.0/default/SystemSuspend.cpp
+++ b/suspend/1.0/default/SystemSuspend.cpp
@@ -194,8 +194,15 @@ void SystemSuspend::initAutosuspend() {
             if (!success) {
                 PLOG(VERBOSE) << "error writing to /sys/power/state";
             }
-            auto callbackLock = std::lock_guard(mCallbackLock);
-            for (const auto& callback : mCallbacks) {
+
+            // A callback could potentially modify mCallbacks (e.g. via registerCallback). That must
+            // not result in a deadlock. To that end, we make a copy of mCallbacks and release
+            // mCallbackLock before calling the copied callbacks.
+            auto callbackLock = std::unique_lock(mCallbackLock);
+            auto callbacksCopy = mCallbacks;
+            callbackLock.unlock();
+
+            for (const auto& callback : callbacksCopy) {
                 callback->notifyWakeup(success).isOk();  // ignore errors
             }
             updateSleepTime(success);
diff --git a/suspend/1.0/default/SystemSuspendUnitTest.cpp b/suspend/1.0/default/SystemSuspendUnitTest.cpp
index 968e52f..6d08ec4 100644
--- a/suspend/1.0/default/SystemSuspendUnitTest.cpp
+++ b/suspend/1.0/default/SystemSuspendUnitTest.cpp
@@ -335,6 +335,29 @@ TEST_F(SystemSuspendTest, DeadCallback) {
     // or checking isOk() on every call.
     checkLoop(3);
 }
+
+// Callback that registers another callback.
+class CbRegisteringCb : public ISystemSuspendCallback {
+   public:
+    CbRegisteringCb(sp<ISystemSuspend> suspendService) : mSuspendService(suspendService) {}
+    Return<void> notifyWakeup(bool x) {
+        sp<MockCallback> cb = new MockCallback(nullptr);
+        cb->disable();
+        mSuspendService->registerCallback(cb);
+        return Void();
+    }
+
+   private:
+    sp<ISystemSuspend> mSuspendService;
+};
+
+// Tests that callback registering another callback doesn't result in a deadlock.
+TEST_F(SystemSuspendTest, CallbackRegisterCallbackNoDeadlock) {
+    sp<CbRegisteringCb> cb = new CbRegisteringCb(suspendService);
+    ASSERT_TRUE(suspendService->registerCallback(cb));
+    checkLoop(3);
+}
+
 }  // namespace android
 
 int main(int argc, char** argv) {
-- 
2.11.0