commit d585959ccce748ba024c63d09b08d737eabfc70a
author Vlad Popa <pvlad@google.com> Wed Aug 02 18:36:04 2023 -0700
committer Vlad Popa <pvlad@google.com> Wed Aug 02 18:36:04 2023 -0700
tree 59b3b97499107bf14e9106e1364e0f9aea31ca47
parent 10e1b68dbd519b451952b98be870fa22688992bb

APC: Add possible deadlock fix when creating track

When creating a new track the thread lock is being held. This can have
the effect of causing a deadlock when the OpPlayAudioMonitor is being
created since the onFirstRef method could also try to access the same
thread lock. Removing the broadcast when calling onFirstRef which is not
needed and would also avoid the deadlock

Test: atest AudioPlaybackConfigurationTest
Bug: 289885996
Change-Id: I16555608176f1b1dfa180e82346319b55c19de01

services/audioflinger/PlaybackTracks.h

diff --git a/services/audioflinger/PlaybackTracks.h b/services/audioflinger/PlaybackTracks.h
index 978c4aa..5f54e11 100644
--- a/services/audioflinger/PlaybackTracks.h
+++ b/services/audioflinger/PlaybackTracks.h
@@ -58,7 +58,7 @@
 
     sp<PlayAudioOpCallback> mOpCallback;
     // called by PlayAudioOpCallback when OP_PLAY_AUDIO is updated in AppOp callback
-    void checkPlayAudioForUsage();
+    void checkPlayAudioForUsage(bool doBroadcast);
 
     wp<IAfThreadBase> mThread;
     std::atomic_bool mHasOpPlayAudio;

services/audioflinger/Tracks.cpp

diff --git a/services/audioflinger/Tracks.cpp b/services/audioflinger/Tracks.cpp
index 1a00681..ecea9eb 100644
--- a/services/audioflinger/Tracks.cpp
+++ b/services/audioflinger/Tracks.cpp
@@ -611,7 +611,9 @@
 
 void OpPlayAudioMonitor::onFirstRef()
 {
-    checkPlayAudioForUsage();
+    // make sure not to broadcast the initial state since it is not needed and could
+    // cause a deadlock since this method can be called with the mThread->mLock held
+    checkPlayAudioForUsage(/*doBroadcast=*/false);
     if (mAttributionSource.packageName.has_value()) {
         mOpCallback = new PlayAudioOpCallback(this);
         mAppOpsManager.startWatchingMode(AppOpsManager::OP_PLAY_AUDIO,
@@ -626,7 +628,7 @@
 // Note this method is never called (and never to be) for audio server / patch record track
 // - not called from constructor due to check on UID,
 // - not called from PlayAudioOpCallback because the callback is not installed in this case
-void OpPlayAudioMonitor::checkPlayAudioForUsage()
+void OpPlayAudioMonitor::checkPlayAudioForUsage(bool doBroadcast)
 {
     const bool hasAppOps = mAttributionSource.packageName.has_value()
         && mAppOpsManager.checkAudioOpNoThrow(
@@ -636,11 +638,13 @@
     bool shouldChange = !hasAppOps;  // check if we need to update.
     if (mHasOpPlayAudio.compare_exchange_strong(shouldChange, hasAppOps)) {
         ALOGD("OpPlayAudio: track:%d usage:%d %smuted", mId, mUsage, hasAppOps ? "not " : "");
-        auto thread = mThread.promote();
-        if (thread != nullptr && thread->type() == IAfThreadBase::OFFLOAD) {
-            // Wake up Thread if offloaded, otherwise it may be several seconds for update.
-            Mutex::Autolock _l(thread->mutex());
-            thread->broadcast_l();
+        if (doBroadcast) {
+            auto thread = mThread.promote();
+            if (thread != nullptr && thread->type() == IAfThreadBase::OFFLOAD) {
+                // Wake up Thread if offloaded, otherwise it may be several seconds for update.
+                Mutex::Autolock _l(thread->mutex());
+                thread->broadcast_l();
+            }
         }
     }
 }
@@ -658,7 +662,7 @@
     }
     sp<OpPlayAudioMonitor> monitor = mMonitor.promote();
     if (monitor != NULL) {
-        monitor->checkPlayAudioForUsage();
+        monitor->checkPlayAudioForUsage(/*doBroadcast=*/true);
     }
 }