Merge "Fix potential NPE when connecting" into sc-dev

packages/SystemUI/src/com/android/systemui/media/MediaResumeListener.kt

@@ -23,7 +23,6 @@ import android.content.Intent
 import android.content.IntentFilter
 import android.content.pm.PackageManager
 import android.media.MediaDescription
-import android.media.session.MediaController
 import android.os.UserHandle
 import android.provider.Settings
 import android.service.media.MediaBrowserService
@@ -171,6 +170,7 @@ class MediaResumeListener @Inject constructor(
         if (useMediaResumption) {
             // If this had been started from a resume state, disconnect now that it's live
             mediaBrowser?.disconnect()
+            mediaBrowser = null
             // If we don't have a resume action, check if we haven't already
             if (data.resumeAction == null && !data.hasCheckedForResume &&
                     !blockedApps.contains(data.packageName) && data.isLocalSession) {
@@ -201,7 +201,6 @@ class MediaResumeListener @Inject constructor(
      */
     private fun tryUpdateResumptionList(key: String, componentName: ComponentName) {
         Log.d(TAG, "Testing if we can connect to $componentName")
-        mediaBrowser?.disconnect()
         mediaBrowser = mediaBrowserFactory.create(
                 object : ResumeMediaBrowser.Callback() {
                     override fun onConnected() {
@@ -211,7 +210,6 @@ class MediaResumeListener @Inject constructor(
                     override fun onError() {
                         Log.e(TAG, "Cannot resume with $componentName")
                         mediaDataManager.setResumeAction(key, null)
-                        mediaBrowser?.disconnect()
                         mediaBrowser = null
                     }
 
@@ -224,7 +222,6 @@ class MediaResumeListener @Inject constructor(
                         Log.d(TAG, "Can get resumable media from $componentName")
                         mediaDataManager.setResumeAction(key, getResumeAction(componentName))
                         updateResumptionList(componentName)
-                        mediaBrowser?.disconnect()
                         mediaBrowser = null
                     }
                 },
@@ -262,30 +259,7 @@ class MediaResumeListener @Inject constructor(
      */
     private fun getResumeAction(componentName: ComponentName): Runnable {
         return Runnable {
-            mediaBrowser?.disconnect()
+            mediaBrowser = mediaBrowserFactory.create(null, componentName)
-            mediaBrowser = mediaBrowserFactory.create(
-                object : ResumeMediaBrowser.Callback() {
-                    override fun onConnected() {
-                        if (mediaBrowser?.token == null) {
-                            Log.e(TAG, "Error after connect")
-                            mediaBrowser?.disconnect()
-                            mediaBrowser = null
-                            return
-                        }
-                        Log.d(TAG, "Connected for restart $componentName")
-                        val controller = MediaController(context, mediaBrowser!!.token)
-                        val controls = controller.transportControls
-                        controls.prepare()
-                        controls.play()
-                    }
-
-                    override fun onError() {
-                        Log.e(TAG, "Resume failed for $componentName")
-                        mediaBrowser?.disconnect()
-                        mediaBrowser = null
-                    }
-                },
-                componentName)
             mediaBrowser?.restart()
         }
     }

packages/SystemUI/src/com/android/systemui/media/ResumeMediaBrowser.java

@@ -16,6 +16,7 @@
 
 package com.android.systemui.media;
 
+import android.annotation.Nullable;
 import android.app.PendingIntent;
 import android.content.ComponentName;
 import android.content.Context;
@@ -47,7 +48,7 @@ public class ResumeMediaBrowser {
 
     private static final String TAG = "ResumeMediaBrowser";
     private final Context mContext;
-    private final Callback mCallback;
+    @Nullable private final Callback mCallback;
     private MediaBrowserFactory mBrowserFactory;
     private MediaBrowser mMediaBrowser;
     private ComponentName mComponentName;
@@ -58,8 +59,8 @@ public class ResumeMediaBrowser {
      * @param callback used to report media items found
      * @param componentName Component name of the MediaBrowserService this browser will connect to
      */
-    public ResumeMediaBrowser(Context context, Callback callback, ComponentName componentName,
+    public ResumeMediaBrowser(Context context, @Nullable Callback callback,
-            MediaBrowserFactory browserFactory) {
+            ComponentName componentName, MediaBrowserFactory browserFactory) {
         mContext = context;
         mCallback = callback;
         mComponentName = componentName;
@@ -93,27 +94,35 @@ public class ResumeMediaBrowser {
                 List<MediaBrowser.MediaItem> children) {
             if (children.size() == 0) {
                 Log.d(TAG, "No children found for " + mComponentName);
+                if (mCallback != null) {
                     mCallback.onError();
+                }
             } else {
                 // We ask apps to return a playable item as the first child when sending
                 // a request with EXTRA_RECENT; if they don't, no resume controls
                 MediaBrowser.MediaItem child = children.get(0);
                 MediaDescription desc = child.getDescription();
                 if (child.isPlayable() && mMediaBrowser != null) {
+                    if (mCallback != null) {
                         mCallback.addTrack(desc, mMediaBrowser.getServiceComponent(),
                                 ResumeMediaBrowser.this);
+                    }
                 } else {
                     Log.d(TAG, "Child found but not playable for " + mComponentName);
+                    if (mCallback != null) {
                         mCallback.onError();
                     }
                 }
+            }
             disconnect();
         }
 
         @Override
         public void onError(String parentId) {
             Log.d(TAG, "Subscribe error for " + mComponentName + ": " + parentId);
+            if (mCallback != null) {
                 mCallback.onError();
+            }
             disconnect();
         }
 
@@ -121,7 +130,9 @@ public class ResumeMediaBrowser {
         public void onError(String parentId, Bundle options) {
             Log.d(TAG, "Subscribe error for " + mComponentName + ": " + parentId
                     + ", options: " + options);
+            if (mCallback != null) {
                 mCallback.onError();
+            }
             disconnect();
         }
     };
@@ -138,14 +149,21 @@ public class ResumeMediaBrowser {
             Log.d(TAG, "Service connected for " + mComponentName);
             if (mMediaBrowser != null && mMediaBrowser.isConnected()) {
                 String root = mMediaBrowser.getRoot();
-                if (!TextUtils.isEmpty(root) && mMediaBrowser != null) {
+                if (!TextUtils.isEmpty(root)) {
+                    if (mCallback != null) {
                         mCallback.onConnected();
+                    }
+                    if (mMediaBrowser != null) {
                         mMediaBrowser.subscribe(root, mSubscriptionCallback);
+                    }
                     return;
                 }
             }
+            if (mCallback != null) {
                 mCallback.onError();
             }
+            disconnect();
+        }
 
         /**
          * Invoked when the client is disconnected from the media browser.
@@ -153,7 +171,9 @@ public class ResumeMediaBrowser {
         @Override
         public void onConnectionSuspended() {
             Log.d(TAG, "Connection suspended for " + mComponentName);
+            if (mCallback != null) {
                 mCallback.onError();
+            }
             disconnect();
         }
 
@@ -163,16 +183,18 @@ public class ResumeMediaBrowser {
         @Override
         public void onConnectionFailed() {
             Log.d(TAG, "Connection failed for " + mComponentName);
+            if (mCallback != null) {
                 mCallback.onError();
+            }
             disconnect();
         }
     };
 
     /**
-     * Disconnect the media browser. This should be called after restart or testConnection have
+     * Disconnect the media browser. This should be done after callbacks have completed to
-     * completed to close the connection.
+     * disconnect from the media browser service.
      */
-    public void disconnect() {
+    protected void disconnect() {
         if (mMediaBrowser != null) {
             mMediaBrowser.disconnect();
         }
@@ -183,7 +205,8 @@ public class ResumeMediaBrowser {
      * Connects to the MediaBrowserService and starts playback.
      * ResumeMediaBrowser.Callback#onError or ResumeMediaBrowser.Callback#onConnected will be called
      * depending on whether it was successful.
-     * ResumeMediaBrowser#disconnect should be called after this to ensure the connection is closed.
+     * If the connection is successful, the listener should call ResumeMediaBrowser#disconnect after
+     * getting a media update from the app
      */
     public void restart() {
         disconnect();
@@ -195,7 +218,10 @@ public class ResumeMediaBrowser {
                     public void onConnected() {
                         Log.d(TAG, "Connected for restart " + mMediaBrowser.isConnected());
                         if (mMediaBrowser == null || !mMediaBrowser.isConnected()) {
+                            if (mCallback != null) {
                                 mCallback.onError();
+                            }
+                            disconnect();
                             return;
                         }
                         MediaSession.Token token = mMediaBrowser.getSessionToken();
@@ -203,18 +229,27 @@ public class ResumeMediaBrowser {
                         controller.getTransportControls();
                         controller.getTransportControls().prepare();
                         controller.getTransportControls().play();
+                        if (mCallback != null) {
                             mCallback.onConnected();
                         }
+                        // listener should disconnect after media player update
+                    }
 
                     @Override
                     public void onConnectionFailed() {
+                        if (mCallback != null) {
                             mCallback.onError();
                         }
+                        disconnect();
+                    }
 
                     @Override
                     public void onConnectionSuspended() {
+                        if (mCallback != null) {
                             mCallback.onError();
                         }
+                        disconnect();
+                    }
                 }, rootHints);
         mMediaBrowser.connect();
     }

packages/SystemUI/tests/src/com/android/systemui/media/MediaResumeListenerTest.kt

@@ -89,6 +89,7 @@ class MediaResumeListenerTest : SysuiTestCase() {
     @Mock private lateinit var dumpManager: DumpManager
 
     @Captor lateinit var callbackCaptor: ArgumentCaptor<ResumeMediaBrowser.Callback>
+    @Captor lateinit var actionCaptor: ArgumentCaptor<Runnable>
 
     private lateinit var executor: FakeExecutor
     private lateinit var data: MediaData
@@ -224,9 +225,6 @@ class MediaResumeListenerTest : SysuiTestCase() {
         // But we do not tell it to add new controls
         verify(mediaDataManager, never())
                 .addResumptionControls(anyInt(), any(), any(), any(), any(), any(), any())
-
-        // Finally, make sure the resume browser disconnected
-        verify(resumeBrowser).disconnect()
     }
 
     @Test
@@ -267,4 +265,39 @@ class MediaResumeListenerTest : SysuiTestCase() {
         verify(mediaDataManager, times(3)).addResumptionControls(anyInt(),
                 any(), any(), any(), any(), any(), eq(PACKAGE_NAME))
     }
+
+    @Test
+    fun testGetResumeAction_restarts() {
+        // Set up mocks to successfully find a MBS that returns valid media
+        val pm = mock(PackageManager::class.java)
+        whenever(mockContext.packageManager).thenReturn(pm)
+        val resolveInfo = ResolveInfo()
+        val serviceInfo = ServiceInfo()
+        serviceInfo.packageName = PACKAGE_NAME
+        resolveInfo.serviceInfo = serviceInfo
+        resolveInfo.serviceInfo.name = CLASS_NAME
+        val resumeInfo = listOf(resolveInfo)
+        whenever(pm.queryIntentServices(any(), anyInt())).thenReturn(resumeInfo)
+
+        val description = MediaDescription.Builder().setTitle(TITLE).build()
+        val component = ComponentName(PACKAGE_NAME, CLASS_NAME)
+        whenever(resumeBrowser.testConnection()).thenAnswer {
+            callbackCaptor.value.addTrack(description, component, resumeBrowser)
+        }
+
+        // When media data is loaded that has not been checked yet, and does have a MBS
+        val dataCopy = data.copy(resumeAction = null, hasCheckedForResume = false)
+        resumeListener.onMediaDataLoaded(KEY, null, dataCopy)
+
+        // Then we test whether the service is valid and set the resume action
+        executor.runAllReady()
+        verify(resumeBrowser).testConnection()
+        verify(mediaDataManager).setResumeAction(eq(KEY), capture(actionCaptor))
+
+        // When the resume action is run
+        actionCaptor.value.run()
+
+        // Then we call restart
+        verify(resumeBrowser).restart()
+    }
 }

packages/SystemUI/tests/src/com/android/systemui/media/ResumeMediaBrowserTest.kt

@@ -91,8 +91,9 @@ public class ResumeMediaBrowserTest : SysuiTestCase() {
         setupBrowserFailed()
         resumeBrowser.testConnection()
 
-        // Then it calls onError
+        // Then it calls onError and disconnects
         verify(callback).onError()
+        verify(browser).disconnect()
     }
 
     @Test
@@ -111,8 +112,9 @@ public class ResumeMediaBrowserTest : SysuiTestCase() {
         setupBrowserConnectionNoResults()
         resumeBrowser.testConnection()
 
-        // Then it calls onError
+        // Then it calls onError and disconnects
         verify(callback).onError()
+        verify(browser).disconnect()
     }
 
     @Test
@@ -132,8 +134,9 @@ public class ResumeMediaBrowserTest : SysuiTestCase() {
         setupBrowserFailed()
         resumeBrowser.findRecentMedia()
 
-        // Then it calls onError
+        // Then it calls onError and disconnects
         verify(callback).onError()
+        verify(browser).disconnect()
     }
 
     @Test
@@ -143,8 +146,9 @@ public class ResumeMediaBrowserTest : SysuiTestCase() {
         whenever(browser.getRoot()).thenReturn(null)
         resumeBrowser.findRecentMedia()
 
-        // Then it calls onError
+        // Then it calls onError and disconnects
         verify(callback).onError()
+        verify(browser).disconnect()
     }
 
     @Test
@@ -163,8 +167,9 @@ public class ResumeMediaBrowserTest : SysuiTestCase() {
         setupBrowserConnectionNoResults()
         resumeBrowser.findRecentMedia()
 
-        // Then it calls onError
+        // Then it calls onError and disconnects
         verify(callback).onError()
+        verify(browser).disconnect()
     }
 
     @Test
@@ -173,8 +178,9 @@ public class ResumeMediaBrowserTest : SysuiTestCase() {
         setupBrowserConnectionNotPlayable()
         resumeBrowser.findRecentMedia()
 
-        // Then it calls onError
+        // Then it calls onError and disconnects
         verify(callback).onError()
+        verify(browser).disconnect()
     }
 
     @Test
@@ -193,8 +199,9 @@ public class ResumeMediaBrowserTest : SysuiTestCase() {
         setupBrowserFailed()
         resumeBrowser.restart()
 
-        // Then it calls onError
+        // Then it calls onError and disconnects
         verify(callback).onError()
+        verify(browser).disconnect()
     }
 
     @Test
@@ -202,13 +209,11 @@ public class ResumeMediaBrowserTest : SysuiTestCase() {
         // When restart is called and we connect successfully
         setupBrowserConnection()
         resumeBrowser.restart()
+        verify(callback).onConnected()
 
         // Then it creates a new controller and sends play command
         verify(transportControls).prepare()
         verify(transportControls).play()
-
-        // Then it calls onConnected
-        verify(callback).onConnected()
     }
 
     /**