Fix notification/media session in Cast and MediaFling

There was a bug that BaseSessionController holds weak reference to
callbacks to avoid holding callback references unnecessary. This was to
prevent potential memory leaks, however the BaseNotificationController
instance was somehow dereferenced in public release build (perhaps due
to some optimization).

This causes Cast/MediaFling to not set up media notification and
MediaSession properly.

This CL fixes the issue by making BaseSessionController holding strong
references. Clients are responsible to remove the callbacks when not
needed.

Bug: 978940
Change-Id: If2146cd2a5c3036897c61cf842e7054182b46055
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1843959Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Commit-Queue: Zhiqiang Zhang <zqzhang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#703448}

chrome/android/features/media_router/java/src/org/chromium/chrome/browser/media/router/caf/BaseSessionController.java

@@ -22,9 +22,7 @@ import org.chromium.chrome.browser.media.router.FlingingController;
 import org.chromium.chrome.browser.media.router.MediaSink;
 import org.chromium.chrome.browser.media.router.MediaSource;
 
-import java.lang.ref.WeakReference;
 import java.util.ArrayList;
-import java.util.Iterator;
 import java.util.List;
 import java.util.Random;
 
@@ -57,7 +55,7 @@ public abstract class BaseSessionController {
     private final CafBaseMediaRouteProvider mProvider;
     private CreateRouteRequestInfo mRouteCreationInfo;
     private final RemoteMediaClient.Callback mRemoteMediaClientCallback;
-    private final List<WeakReference<Callback>> mCallbacks = new ArrayList<>();
+    private final List<Callback> mCallbacks = new ArrayList<>();
 
     public BaseSessionController(CafBaseMediaRouteProvider provider) {
         mProvider = provider;
@@ -65,17 +63,11 @@ public abstract class BaseSessionController {
     }
 
     public void addCallback(Callback callback) {
-        mCallbacks.add(new WeakReference<>(callback));
+        mCallbacks.add(callback);
     }
 
     public void removeCallback(Callback callback) {
-        Iterator<WeakReference<Callback>> iterator = mCallbacks.iterator();
-
-        while (iterator.hasNext()) {
-            if (iterator.next().get() == callback) {
-                iterator.remove();
-            }
-        }
+        mCallbacks.remove(callback);
     }
 
     public void requestSessionLaunch() {
@@ -266,15 +258,8 @@ public abstract class BaseSessionController {
     }
 
     private void notifyCallback(NotifyCallbackAction action) {
-        Iterator<WeakReference<Callback>> iterator = mCallbacks.iterator();
-
-        while (iterator.hasNext()) {
-            Callback callback = iterator.next().get();
-            if (callback == null) {
-                iterator.remove();
-            } else {
-                action.notify(callback);
-            }
+        for (Callback callback : mCallbacks) {
+            action.notify(callback);
         }
     }
 

chrome/android/features/media_router/java/src/org/chromium/chrome/browser/media/router/caf/remoting/CafExpandedControllerActivity.java

@@ -152,6 +152,12 @@ public class CafExpandedControllerActivity
         }
     }
 
+    @Override
+    protected void onDestroy() {
+        mSessionController.removeCallback(this);
+        super.onDestroy();
+    }
+
     @Override
     public void onSessionStarted() {}
 

chrome/android/features/media_router/junit/src/org/chromium/chrome/browser/media/router/caf/BaseSessionControllerTest.java

@@ -164,6 +164,18 @@ public class BaseSessionControllerTest {
         verify(mNotificationController).onSessionEnded();
     }
 
+    @Test
+    public void testSessionLifecyleNotNotifiedAfterCallbackRemoved() {
+        mController.removeCallback(mNotificationController);
+
+        mController.attachToCastSession(mCastSession);
+        mController.onSessionStarted();
+        verify(mNotificationController, never()).onSessionStarted();
+
+        mController.onSessionEnded();
+        verify(mNotificationController, never()).onSessionEnded();
+    }
+
     @Test
     public void testGetCapabilities() {
         mController.attachToCastSession(mCastSession);