commit c8dd5e885a9859c8fa8d01cbb99a4bdc1b005f8d
author Wei Jia <wjia@google.com> Thu Jul 13 17:47:56 2017 -0700
committer JP Sugarbroad <jpsugar@google.com> Thu Jul 20 15:26:49 2017 -0700
tree 2e13c43fc7251b547dbcd999289d701d258fc1fa
parent ab9cf54fd79c950045781fd4106503a3c91d9215

MediaPlayerService: fix access of mPlayer in client

Test: poc doesn't crash
Bug: 38234812
Change-Id: I6f9be046ff66d2d5bed27bd712287e4ead550830
(cherry picked from commit 502c2f405355c3253990ac4edae345ac1907f916)

media/libmediaplayerservice/MediaPlayerService.cpp

diff --git a/media/libmediaplayerservice/MediaPlayerService.cpp b/media/libmediaplayerservice/MediaPlayerService.cpp
index bd16e91..6e689e6 100644
--- a/media/libmediaplayerservice/MediaPlayerService.cpp
+++ b/media/libmediaplayerservice/MediaPlayerService.cpp
@@ -82,6 +82,9 @@
 #include "HTTPBase.h"
 #include "RemoteDisplay.h"
 
+static const int kDumpLockRetries = 50;
+static const int kDumpLockSleepUs = 20000;
+
 namespace {
 using android::media::Metadata;
 using android::status_t;
@@ -405,12 +408,32 @@
     snprintf(buffer, 255, "  pid(%d), connId(%d), status(%d), looping(%s)\n",
             mPid, mConnId, mStatus, mLoop?"true": "false");
     result.append(buffer);
-    write(fd, result.string(), result.size());
-    if (mPlayer != NULL) {
-        mPlayer->dump(fd, args);
+
+    sp<MediaPlayerBase> p;
+    sp<AudioOutput> audioOutput;
+    bool locked = false;
+    for (int i = 0; i < kDumpLockRetries; ++i) {
+        if (mLock.tryLock() == NO_ERROR) {
+            locked = true;
+            break;
+        }
+        usleep(kDumpLockSleepUs);
     }
-    if (mAudioOutput != 0) {
-        mAudioOutput->dump(fd, args);
+
+    if (locked) {
+        p = mPlayer;
+        audioOutput = mAudioOutput;
+        mLock.unlock();
+    } else {
+        result.append("  lock is taken, no dump from player and audio output\n");
+    }
+    write(fd, result.string(), result.size());
+
+    if (p != NULL) {
+        p->dump(fd, args);
+    }
+    if (audioOutput != 0) {
+        audioOutput->dump(fd, args);
     }
     write(fd, "\n", 1);
     return NO_ERROR;
@@ -590,7 +613,10 @@
 MediaPlayerService::Client::~Client()
 {
     ALOGV("Client(%d) destructor pid = %d", mConnId, mPid);
-    mAudioOutput.clear();
+    {
+        Mutex::Autolock l(mLock);
+        mAudioOutput.clear();
+    }
     wp<Client> client(this);
     disconnect();
     mService->removeClient(client);
@@ -609,10 +635,9 @@
         Mutex::Autolock l(mLock);
         p = mPlayer;
         mClient.clear();
+        mPlayer.clear();
     }
 
-    mPlayer.clear();
-
     // clear the notification to prevent callbacks to dead client
     // and reset the player. We assume the player will serialize
     // access to itself if necessary.
@@ -633,7 +658,7 @@
 sp<MediaPlayerBase> MediaPlayerService::Client::createPlayer(player_type playerType)
 {
     // determine if we have the right player type
-    sp<MediaPlayerBase> p = mPlayer;
+    sp<MediaPlayerBase> p = getPlayer();
     if ((p != NULL) && (p->playerType() != playerType)) {
         ALOGV("delete player");
         p.clear();
@@ -721,6 +746,7 @@
     }
 
     if (mStatus == OK) {
+        Mutex::Autolock l(mLock);
         mPlayer = p;
     }
 }