Fix 5243349 RemoteControlDisplay incorrectly updated

This fixes a case where the RCD would display transport control
 for a RemoteControlClient that didn't have audio focus.
This was happening because registering an RCD was directly calling
 the updateRemoteControlDisplay method, without first calling
 the checkUpdateRemoteControlDisplay method which verifies the
 conditions before updating the display. One of those conditions
 is that the audio focus stack shouldn't be empty.

To verify this fix, several functions were also rename to clearly
 indicate the lock order and verify we properly synchronize on
 the right objects. In doing so, a missing synchronization on
 audio focus was found.

Change-Id: If1baaac224ea676aeb83ac0aefcc53f87461c32e

1 files changed, 47 insertions, 43 deletions

diff --git a/media/java/android/media/AudioService.java b/media/java/android/media/AudioService.java
index de94ebd..adb6619 100644
--- a/media/java/android/media/AudioService.java
+++ b/media/java/android/media/AudioService.java
@@ -2637,7 +2637,7 @@ public class AudioService extends IAudioService.Stub {
                 notifyTopOfAudioFocusStack();
                 // there's a new top of the stack, let the remote control know
                 synchronized(mRCStack) {
-                    checkUpdateRemoteControlDisplay_syncRcs(RC_INFO_ALL);
+                    checkUpdateRemoteControlDisplay_syncAfRcs(RC_INFO_ALL);
                 }
             }
         } else {
@@ -2680,7 +2680,7 @@ public class AudioService extends IAudioService.Stub {
             notifyTopOfAudioFocusStack();
             // there's a new top of the stack, let the remote control know
             synchronized(mRCStack) {
-                checkUpdateRemoteControlDisplay_syncRcs(RC_INFO_ALL);
+                checkUpdateRemoteControlDisplay_syncAfRcs(RC_INFO_ALL);
             }
         }
     }
@@ -2784,7 +2784,7 @@ public class AudioService extends IAudioService.Stub {
 
             // there's a new top of the stack, let the remote control know
             synchronized(mRCStack) {
-                checkUpdateRemoteControlDisplay_syncRcs(RC_INFO_ALL);
+                checkUpdateRemoteControlDisplay_syncAfRcs(RC_INFO_ALL);
             }
         }//synchronized(mAudioFocusLock)
 
@@ -3182,7 +3182,7 @@ public class AudioService extends IAudioService.Stub {
      * Helper function:
      * Called synchronized on mRCStack
      */
-    private void clearRemoteControlDisplay_syncRcs() {
+    private void clearRemoteControlDisplay_syncAfRcs() {
         synchronized(mCurrentRcLock) {
             mCurrentRcClient = null;
         }
@@ -3191,18 +3191,21 @@ public class AudioService extends IAudioService.Stub {
     }
 
     /**
-     * Helper function:
-     * Called synchronized on mRCStack
-     * mRCStack.isEmpty() is false
+     * Helper function for code readability: only to be called from
+     *    checkUpdateRemoteControlDisplay_syncAfRcs() which checks the preconditions for
+     *    this method.
+     * Preconditions:
+     *    - called synchronized mAudioFocusLock then on mRCStack
+     *    - mRCStack.isEmpty() is false
      */
-    private void updateRemoteControlDisplay_syncRcs(int infoChangedFlags) {
+    private void updateRemoteControlDisplay_syncAfRcs(int infoChangedFlags) {
         RemoteControlStackEntry rcse = mRCStack.peek();
         int infoFlagsAboutToBeUsed = infoChangedFlags;
         // this is where we enforce opt-in for information display on the remote controls
         //   with the new AudioManager.registerRemoteControlClient() API
         if (rcse.mRcClient == null) {
             //Log.w(TAG, "Can't update remote control display with null remote control client");
-            clearRemoteControlDisplay_syncRcs();
+            clearRemoteControlDisplay_syncAfRcs();
             return;
         }
         synchronized(mCurrentRcLock) {
@@ -3219,17 +3222,17 @@ public class AudioService extends IAudioService.Stub {
 
     /**
      * Helper function:
-     * Called synchronized on mFocusLock, then mRCStack
+     * Called synchronized on mAudioFocusLock, then mRCStack
      * Check whether the remote control display should be updated, triggers the update if required
      * @param infoChangedFlags the flags corresponding to the remote control client information
      *     that has changed, if applicable (checking for the update conditions might trigger a
      *     clear, rather than an update event).
      */
-    private void checkUpdateRemoteControlDisplay_syncRcs(int infoChangedFlags) {
+    private void checkUpdateRemoteControlDisplay_syncAfRcs(int infoChangedFlags) {
         // determine whether the remote control display should be refreshed
         // if either stack is empty, there is a mismatch, so clear the RC display
         if (mRCStack.isEmpty() || mFocusStack.isEmpty()) {
-            clearRemoteControlDisplay_syncRcs();
+            clearRemoteControlDisplay_syncAfRcs();
             return;
         }
         // if the top of the two stacks belong to different packages, there is a mismatch, clear
@@ -3237,18 +3240,18 @@ public class AudioService extends IAudioService.Stub {
                 && (mFocusStack.peek().mPackageName != null)
                 && !(mRCStack.peek().mCallingPackageName.compareTo(
                         mFocusStack.peek().mPackageName) == 0)) {
-            clearRemoteControlDisplay_syncRcs();
+            clearRemoteControlDisplay_syncAfRcs();
             return;
         }
         // if the audio focus didn't originate from the same Uid as the one in which the remote
         //   control information will be retrieved, clear
         if (mRCStack.peek().mCallingUid != mFocusStack.peek().mCallingUid) {
-            clearRemoteControlDisplay_syncRcs();
+            clearRemoteControlDisplay_syncAfRcs();
             return;
         }
         // refresh conditions were verified: update the remote controls
-        // ok to call, mRCStack is not empty
-        updateRemoteControlDisplay_syncRcs(infoChangedFlags);
+        // ok to call: synchronized mAudioFocusLock then on mRCStack, mRCStack is not empty
+        updateRemoteControlDisplay_syncAfRcs(infoChangedFlags);
     }
 
     /** see AudioManager.registerMediaButtonEventReceiver(ComponentName eventReceiver) */
@@ -3259,7 +3262,7 @@ public class AudioService extends IAudioService.Stub {
             synchronized(mRCStack) {
                 pushMediaButtonReceiver(eventReceiver);
                 // new RC client, assume every type of information shall be queried
-                checkUpdateRemoteControlDisplay_syncRcs(RC_INFO_ALL);
+                checkUpdateRemoteControlDisplay_syncAfRcs(RC_INFO_ALL);
             }
         }
     }
@@ -3274,7 +3277,7 @@ public class AudioService extends IAudioService.Stub {
                 removeMediaButtonReceiver(eventReceiver);
                 if (topOfStackWillChange) {
                     // current RC client will change, assume every type of info needs to be queried
-                    checkUpdateRemoteControlDisplay_syncRcs(RC_INFO_ALL);
+                    checkUpdateRemoteControlDisplay_syncAfRcs(RC_INFO_ALL);
                 }
             }
         }
@@ -3283,6 +3286,7 @@ public class AudioService extends IAudioService.Stub {
     /** see AudioManager.registerRemoteControlClient(ComponentName eventReceiver, ...) */
     public void registerRemoteControlClient(ComponentName eventReceiver,
             IRemoteControlClient rcClient, String clientName, String callingPackageName) {
+        if (DEBUG_RC) Log.i(TAG, "Register remote control client rcClient="+rcClient);
         synchronized(mAudioFocusLock) {
             synchronized(mRCStack) {
                 // store the new display information
@@ -3330,7 +3334,7 @@ public class AudioService extends IAudioService.Stub {
                 // if the eventReceiver is at the top of the stack
                 // then check for potential refresh of the remote controls
                 if (isCurrentRcController(eventReceiver)) {
-                    checkUpdateRemoteControlDisplay_syncRcs(RC_INFO_ALL);
+                    checkUpdateRemoteControlDisplay_syncAfRcs(RC_INFO_ALL);
                 }
             }
         }
@@ -3435,35 +3439,35 @@ public class AudioService extends IAudioService.Stub {
      */
     public void registerRemoteControlDisplay(IRemoteControlDisplay rcd) {
         if (DEBUG_RC) Log.d(TAG, ">>> registerRemoteControlDisplay("+rcd+")");
-        synchronized(mRCStack) {
-            if ((mRcDisplay == rcd) || (rcd == null)) {
-                return;
-            }
-            // if we had a display before, stop monitoring its death
-            rcDisplay_stopDeathMonitor_syncRcStack();
-            mRcDisplay = rcd;
-            // new display, start monitoring its death
-            rcDisplay_startDeathMonitor_syncRcStack();
+        synchronized(mAudioFocusLock) {
+            synchronized(mRCStack) {
+                if ((mRcDisplay == rcd) || (rcd == null)) {
+                    return;
+                }
+                // if we had a display before, stop monitoring its death
+                rcDisplay_stopDeathMonitor_syncRcStack();
+                mRcDisplay = rcd;
+                // new display, start monitoring its death
+                rcDisplay_startDeathMonitor_syncRcStack();
 
-            // let all the remote control clients there is a new display
-            // no need to unplug the previous because we only support one display
-            // and the clients don't track the death of the display
-            Iterator<RemoteControlStackEntry> stackIterator = mRCStack.iterator();
-            while(stackIterator.hasNext()) {
-                RemoteControlStackEntry rcse = stackIterator.next();
-                if(rcse.mRcClient != null) {
-                    try {
-                        rcse.mRcClient.plugRemoteControlDisplay(mRcDisplay);
-                    } catch (RemoteException e) {
-                        Log.e(TAG, "Error connecting remote control display to client: " + e);
-                        e.printStackTrace();
+                // let all the remote control clients there is a new display
+                // no need to unplug the previous because we only support one display
+                // and the clients don't track the death of the display
+                Iterator<RemoteControlStackEntry> stackIterator = mRCStack.iterator();
+                while(stackIterator.hasNext()) {
+                    RemoteControlStackEntry rcse = stackIterator.next();
+                    if(rcse.mRcClient != null) {
+                        try {
+                            rcse.mRcClient.plugRemoteControlDisplay(mRcDisplay);
+                        } catch (RemoteException e) {
+                            Log.e(TAG, "Error connecting remote control display to client: " + e);
+                            e.printStackTrace();
+                        }
                     }
                 }
-            }
 
-            if (!mRCStack.isEmpty()) {
                 // we have a new display, of which all the clients are now aware: have it be updated
-                updateRemoteControlDisplay_syncRcs(RC_INFO_ALL);
+                checkUpdateRemoteControlDisplay_syncAfRcs(RC_INFO_ALL);
             }
         }
     }