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

Author: jmtrivi

media/java/android/media/AudioService.java

@@ -2637,7 +2637,7 @@ private void removeFocusStackEntry(String clientToRemove, boolean signal) {
                 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 @@ private void removeFocusStackEntryForClient(IBinder cb) {
             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 int requestAudioFocus(int mainStreamType, int focusChangeHint, IBinder cb
 
             // 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 @@ private void onRcDisplayUpdate(RemoteControlStackEntry rcse, int flags /* USED ?
      * Helper function:
      * Called synchronized on mRCStack
      */
-    private void clearRemoteControlDisplay_syncRcs() {
+    private void clearRemoteControlDisplay_syncAfRcs() {
         synchronized(mCurrentRcLock) {
             mCurrentRcClient = null;
         }
@@ -3191,18 +3191,21 @@ private void clearRemoteControlDisplay_syncRcs() {
     }
 
     /**
-     * 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,36 +3222,36 @@ private void updateRemoteControlDisplay_syncRcs(int infoChangedFlags) {
 
     /**
      * 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
         if ((mRCStack.peek().mCallingPackageName != null)
                 && (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 void registerMediaButtonEventReceiver(ComponentName eventReceiver) {
             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 void unregisterMediaButtonEventReceiver(ComponentName eventReceiver) {
                 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 void unregisterMediaButtonEventReceiver(ComponentName eventReceiver) {
     /** 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 void registerRemoteControlClient(ComponentName eventReceiver,
                 // 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 @@ private void rcDisplay_startDeathMonitor_syncRcStack() {
      */
     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);
             }
         }
     }