Fix deadlock when callback modifies callback list.

Bug: 117555757
Test: SystemSuspendUnitTest
Change-Id: I40e7ffcc1bbc744a5b9e6d739166f1eebb8ed151

diff --git a/suspend/1.0/default/SystemSuspend.cpp b/suspend/1.0/default/SystemSuspend.cpp
index 5f70671..eae240a 100644 (file)
--- 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 (file)
--- 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) {