Android/frameworks_base
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix potential NPE when connecting

Browse Source 
In one case NPE could be caused by a callback in MediaResumeListener
setting the browser to null. However the code for this can be simplified

- Move disconnect() calls to ResumeMediaBrowser where possible
- Remove callback that is now unnecessary
- Also, the controller commands in MediaResumeListener were redundant since
ResumeMediaBrowser already handles restarting playback

Fixes: 183122550
Test: atest MediaResumeListenerTest ResumeMediaBrowserTest
Change-Id: I7cbde108b4bedcd6e22e08c59af83ef42b586dd4
This commit is contained in:
Beth Thibodeau
2021-05-24 16:04:23 -04:00
parent 0859a5f4a3
commit 9c0a439ea1
4 changed files with 111 additions and 64 deletions
Download Patch File Download Diff File Expand all files Collapse all files

30
packages/SystemUI/src/com/android/systemui/media/MediaResumeListener.kt
View File

@@ -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(
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 = mediaBrowserFactory.create(null, componentName)
mediaBrowser?.restart()
}
}

81
packages/SystemUI/src/com/android/systemui/media/ResumeMediaBrowser.java
View File

@@ -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,
MediaBrowserFactory browserFactory) {
public ResumeMediaBrowser(Context context, @Nullable Callback callback,
ComponentName componentName, MediaBrowserFactory browserFactory) {
mContext = context;
mCallback = callback;
mComponentName = componentName;
@@ -93,18 +94,24 @@ public class ResumeMediaBrowser {
List<MediaBrowser.MediaItem> children) {
if (children.size() == 0) {
Log.d(TAG, "No children found for " + mComponentName);
mCallback.onError();
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) {
mCallback.addTrack(desc, mMediaBrowser.getServiceComponent(),
ResumeMediaBrowser.this);
if (mCallback != null) {
mCallback.addTrack(desc, mMediaBrowser.getServiceComponent(),
ResumeMediaBrowser.this);
}
} else {
Log.d(TAG, "Child found but not playable for " + mComponentName);
mCallback.onError();
if (mCallback != null) {
mCallback.onError();
}
}
}
disconnect();
@@ -113,7 +120,9 @@ public class ResumeMediaBrowser {
@Override
public void onError(String parentId) {
Log.d(TAG, "Subscribe error for " + mComponentName + ": " + parentId);
mCallback.onError();
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);
mCallback.onError();
if (mCallback != null) {
mCallback.onError();
}
disconnect();
}
};
@@ -138,13 +149,20 @@ 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) {
mCallback.onConnected();
mMediaBrowser.subscribe(root, mSubscriptionCallback);
if (!TextUtils.isEmpty(root)) {
if (mCallback != null) {
mCallback.onConnected();
}
if (mMediaBrowser != null) {
mMediaBrowser.subscribe(root, mSubscriptionCallback);
}
return;
}
}
mCallback.onError();
if (mCallback != null) {
mCallback.onError();
}
disconnect();
}
/**
@@ -153,7 +171,9 @@ public class ResumeMediaBrowser {
@Override
public void onConnectionSuspended() {
Log.d(TAG, "Connection suspended for " + mComponentName);
mCallback.onError();
if (mCallback != null) {
mCallback.onError();
}
disconnect();
}
@@ -163,16 +183,18 @@ public class ResumeMediaBrowser {
@Override
public void onConnectionFailed() {
Log.d(TAG, "Connection failed for " + mComponentName);
mCallback.onError();
if (mCallback != null) {
mCallback.onError();
}
disconnect();
}
};
/**
* Disconnect the media browser. This should be called after restart or testConnection have
* completed to close the connection.
* Disconnect the media browser. This should be done after callbacks have completed to
* 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()) {
mCallback.onError();
if (mCallback != null) {
mCallback.onError();
}
disconnect();
return;
}
MediaSession.Token token = mMediaBrowser.getSessionToken();
@@ -203,17 +229,26 @@ public class ResumeMediaBrowser {
controller.getTransportControls();
controller.getTransportControls().prepare();
controller.getTransportControls().play();
mCallback.onConnected();
if (mCallback != null) {
mCallback.onConnected();
}
// listener should disconnect after media player update
}
@Override
public void onConnectionFailed() {
mCallback.onError();
if (mCallback != null) {
mCallback.onError();
}
disconnect();
}
@Override
public void onConnectionSuspended() {
mCallback.onError();
if (mCallback != null) {
mCallback.onError();
}
disconnect();
}
}, rootHints);
mMediaBrowser.connect();

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

@@ -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()
}
}

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

@@ -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()
}
/**