Portals: Remove media session notification on activation

Currently, after portal activation, the notification persists, but
clicking any buttons in the notification causes a crash. The crash
happens because the media session observer was cleaned up after portal
activation (the webcontents are swapped during portal activation and
the previous java webcontents gets destroyed, but the native counterpart
is kept alive).

This CL is a band-aid fix that causes the notification to be removed
after portal activation (to match the media session observer being
cleaned up), and avoids the crash. It doesn't fix the underlying
issue of the media playback notification not correctly handling
inner web contents and so portals playing media will not display a
notification.

Bug: 1078400
Change-Id: I533f7ab079c0e8e21927279cf85321b7f1f400a6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2199730Reviewed-by: David Trainor <dtrainor@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Commit-Queue: Adithya Srinivasan <adithyas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#775768}

chrome/android/javatests/src/org/chromium/chrome/browser/portals/PortalsTest.java

@@ -33,6 +33,7 @@ import org.chromium.base.test.util.CommandLineFlags;
 import org.chromium.base.test.util.DisabledTest;
 import org.chromium.base.test.util.Feature;
 import org.chromium.base.test.util.MinAndroidSdkLevel;
+import org.chromium.chrome.R;
 import org.chromium.chrome.browser.ChromeActivity;
 import org.chromium.chrome.browser.banners.AppBannerManager;
 import org.chromium.chrome.browser.engagement.SiteEngagementService;
@@ -52,6 +53,7 @@ import org.chromium.content_public.browser.WebContents;
 import org.chromium.content_public.browser.test.util.Coordinates;
 import org.chromium.content_public.browser.test.util.Criteria;
 import org.chromium.content_public.browser.test.util.CriteriaHelper;
+import org.chromium.content_public.browser.test.util.DOMUtils;
 import org.chromium.content_public.browser.test.util.JavaScriptUtils;
 import org.chromium.content_public.browser.test.util.TestThreadUtils;
 import org.chromium.content_public.browser.test.util.TouchCommon;
@@ -487,16 +489,24 @@ public class PortalsTest {
         Assert.assertEquals(mainTitle, history.get(1).getTitle());
     }
 
+    interface NotificationPredicate {
+        boolean test(StatusBarNotification notification);
+    }
+
+    private NotificationPredicate mMediaCaptureNotificationPred = notification
+            -> TextUtils.equals(notification.getTag(), "MediaCaptureNotificationService");
+    private NotificationPredicate mMediaPlaybackNotificationPred =
+            notification -> notification.getId() == R.id.media_playback_notification;
+
     @TargetApi(Build.VERSION_CODES.M)
-    private void waitForMediaCaptureNotification() {
+    private void waitForNotification(NotificationPredicate pred) {
         CriteriaHelper.pollInstrumentationThread(() -> {
             StatusBarNotification notifications[] =
                     ((NotificationManager) ContextUtils.getApplicationContext().getSystemService(
                              Context.NOTIFICATION_SERVICE))
                             .getActiveNotifications();
             for (StatusBarNotification statusBarNotification : notifications) {
-                if (TextUtils.equals(
-                            statusBarNotification.getTag(), "MediaCaptureNotificationService")) {
+                if (pred.test(statusBarNotification)) {
                     return true;
                 }
             }
@@ -504,6 +514,22 @@ public class PortalsTest {
         });
     }
 
+    @TargetApi(Build.VERSION_CODES.M)
+    private void waitForNoNotifications(NotificationPredicate pred) {
+        CriteriaHelper.pollInstrumentationThread(() -> {
+            StatusBarNotification notifications[] =
+                    ((NotificationManager) ContextUtils.getApplicationContext().getSystemService(
+                             Context.NOTIFICATION_SERVICE))
+                            .getActiveNotifications();
+            for (StatusBarNotification statusBarNotification : notifications) {
+                if (pred.test(statusBarNotification)) {
+                    return false;
+                }
+            }
+            return true;
+        });
+    }
+
     @Test
     @LargeTest
     @Feature({"Portals"})
@@ -529,13 +555,39 @@ public class PortalsTest {
                     ModalDialogProperties.ButtonType.POSITIVE);
         });
         // Wait for video capture notification.
-        waitForMediaCaptureNotification();
+        waitForNotification(mMediaCaptureNotificationPred);
         // Activate portal.
         executeScriptAndAwaitSwap(tab, "activate()");
         // Wait for adoption to complete.
         JavaScriptUtils.runJavascriptWithAsyncResult(tab.getWebContents(), "waitForAdoption()");
         // Check if notification is still shown.
-        waitForMediaCaptureNotification();
+        waitForNotification(mMediaCaptureNotificationPred);
+    }
+
+    @Test
+    @LargeTest
+    @Feature({"Portals"})
+    @MinAndroidSdkLevel(Build.VERSION_CODES.M)
+    public void testMediaNotificationDisappearsAfterActivation() throws Exception {
+        String mainUrl =
+                mTestServer.getURL("/chrome/test/data/android/portals/media-notification.html");
+        mActivityTestRule.startMainActivityWithURL(mainUrl);
+        Tab tab = mActivityTestRule.getActivity().getActivityTab();
+
+        // Start video playback.
+        DOMUtils.clickNode(tab.getWebContents(), "video");
+        // Wait for media notification.
+        waitForNotification(mMediaPlaybackNotificationPred);
+        // Activate portal.
+        executeScriptAndAwaitSwap(tab, "activate()");
+        // Wait for adoption to complete.
+        JavaScriptUtils.runJavascriptWithAsyncResult(tab.getWebContents(), "waitForAdoption()");
+        // Notification should disappear.
+        waitForNoNotifications(mMediaPlaybackNotificationPred);
+        // Activate predecessor.
+        executeScriptAndAwaitSwap(tab, "activate()");
+        // Media notification should show again.
+        waitForNotification(mMediaPlaybackNotificationPred);
     }
 
     @Test

chrome/test/data/android/portals/media-capture-portal.html

@@ -13,5 +13,12 @@
       await adoptionCompletePromise;
       domAutomationController.send(true);
     }
+
+    function activate() {
+      let portal = document.querySelector('portal');
+      portal.activate().then(() => {
+        document.body.removeChild(portal);
+      });
+    }
   </script>
 </body>

chrome/test/data/android/portals/media-notification.html

+<!DOCTYPE html>
+<body>
+  <video id="video" src="../media/test.webm" loop></video>
+  <portal src="media-capture-portal.html"></portal>
+  <script>
+    let video = document.querySelector('video');
+    video.onclick = () => {
+      video.play();
+    }
+
+    function activate() {
+      let portal = document.querySelector('portal');
+      portal.activate();
+    }
+  </script>
+</body>

content/browser/media/session/media_session_android.cc

@@ -38,9 +38,14 @@ MediaSessionAndroid::MediaSessionAndroid(MediaSessionImpl* session)
       Java_MediaSessionImpl_create(env, reinterpret_cast<intptr_t>(this));
   j_media_session_ = JavaObjectWeakGlobalRef(env, j_media_session);
 
-  WebContentsAndroid* contents_android = GetWebContentsAndroid();
-  if (contents_android)
-    contents_android->SetMediaSession(j_media_session);
+  WebContentsImpl* contents =
+      static_cast<WebContentsImpl*>(media_session_->web_contents());
+  if (contents) {
+    web_contents_android_ = contents->GetWebContentsAndroid();
+    DCHECK(web_contents_android_);
+    web_contents_android_->SetMediaSession(j_media_session);
+    web_contents_android_->AddDestructionObserver(this);
+  }
 
   session->AddObserver(observer_receiver_.BindNewPipeAndPassRemote());
 }
@@ -55,9 +60,10 @@ MediaSessionAndroid::~MediaSessionAndroid() {
 
   j_media_session_.reset();
 
-  WebContentsAndroid* contents_android = GetWebContentsAndroid();
-  if (contents_android)
-    contents_android->SetMediaSession(nullptr);
+  if (web_contents_android_) {
+    web_contents_android_->SetMediaSession(nullptr);
+    web_contents_android_->RemoveDestructionObserver(this);
+  }
 }
 
 // static
@@ -71,7 +77,7 @@ ScopedJavaLocalRef<jobject> JNI_MediaSessionImpl_GetMediaSessionFromWebContents(
   MediaSessionImpl* session = MediaSessionImpl::Get(contents);
   DCHECK(session);
   return MediaSessionAndroid::JavaObjectGetter::GetJavaObject(
-      session->session_android());
+      session->GetMediaSessionAndroid());
 }
 
 void MediaSessionAndroid::MediaSessionInfoChanged(
@@ -173,6 +179,14 @@ void MediaSessionAndroid::MediaSessionPositionChanged(
   }
 }
 
+// The Java MediaSession is kept alive by the Java WebContents and will be
+// cleared when the WebContents is destroyed, so we destroy the corresponding
+// MediaSessionAndroid to ensure mediaSessionDestroyed is called.
+void MediaSessionAndroid::WebContentsAndroidDestroyed(
+    WebContentsAndroid* web_contents_android) {
+  media_session_->ClearMediaSessionAndroid();  // Deletes |this|.
+}
+
 void MediaSessionAndroid::Resume(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& j_obj) {
@@ -228,14 +242,6 @@ void MediaSessionAndroid::RequestSystemAudioFocus(
       media_session::mojom::AudioFocusType::kGain);
 }
 
-WebContentsAndroid* MediaSessionAndroid::GetWebContentsAndroid() {
-  WebContentsImpl* contents =
-      static_cast<WebContentsImpl*>(media_session_->web_contents());
-  if (!contents)
-    return nullptr;
-  return contents->GetWebContentsAndroid();
-}
-
 ScopedJavaLocalRef<jobject> MediaSessionAndroid::GetJavaObject() {
   JNIEnv* env = base::android::AttachCurrentThread();
   return j_media_session_.get(env);

content/browser/media/session/media_session_android.h

@@ -11,20 +11,21 @@
 
 #include "base/android/jni_weak_ref.h"
 #include "base/android/scoped_java_ref.h"
+#include "content/browser/web_contents/web_contents_android.h"
 #include "mojo/public/cpp/bindings/receiver.h"
 #include "services/media_session/public/mojom/media_session.mojom.h"
 
 namespace content {
 
 class MediaSessionImpl;
-class WebContentsAndroid;
 
 // This class is interlayer between native MediaSession and Java
 // MediaSession. This class is owned by the native MediaSession and will
 // teardown Java MediaSession when the native MediaSession is destroyed.
 // Java MediaSessionObservers are also proxied via this class.
 class MediaSessionAndroid final
-    : public media_session::mojom::MediaSessionObserver {
+    : public media_session::mojom::MediaSessionObserver,
+      public WebContentsAndroid::DestructionObserver {
  public:
   // Helper class for calling GetJavaObject() in a static method, in order to
   // avoid leaking the Java object outside.
@@ -48,6 +49,12 @@ class MediaSessionAndroid final
   void MediaSessionPositionChanged(
       const base::Optional<media_session::MediaPosition>& position) override;
 
+  // WebContentsAndroid::DestructionObserver overrides:
+  // TODO(crbug.com/1091229): Remove this when we correctly support media
+  // sessions in portals.
+  void WebContentsAndroidDestroyed(
+      WebContentsAndroid* web_contents_android) override;
+
   // MediaSession method wrappers.
   void Resume(JNIEnv* env, const base::android::JavaParamRef<jobject>& j_obj);
   void Suspend(JNIEnv* env, const base::android::JavaParamRef<jobject>& j_obj);
@@ -66,13 +73,14 @@ class MediaSessionAndroid final
       const base::android::JavaParamRef<jobject>& j_obj);
 
  private:
-  WebContentsAndroid* GetWebContentsAndroid();
-
   base::android::ScopedJavaLocalRef<jobject> GetJavaObject();
 
   // The linked Java object. The strong reference is hold by Java WebContensImpl
   // to avoid introducing a new GC root.
   JavaObjectWeakGlobalRef j_media_session_;
+  // WebContentsAndroid corresponding to the Java WebContentsImpl that holds a
+  // strong reference to |j_media_session_|.
+  WebContentsAndroid* web_contents_android_;
 
   MediaSessionImpl* const media_session_;
 

content/browser/media/session/media_session_impl.cc

@@ -225,6 +225,22 @@ MediaSessionImpl::~MediaSessionImpl() {
   DCHECK(audio_focus_state_ == State::INACTIVE);
 }
 
+#if defined(OS_ANDROID)
+void MediaSessionImpl::ClearMediaSessionAndroid() {
+  session_android_.reset();
+}
+
+MediaSessionAndroid* MediaSessionImpl::GetMediaSessionAndroid() {
+  // |session_android_| can be null if a portal is activated, the java
+  // WebContents is destroyed and ClearMediaSessionAndroid is called.
+  // TODO(crbug.com/1091229): Remove this when we correctly support media
+  // sessions in portals.
+  if (!session_android_)
+    session_android_ = std::make_unique<MediaSessionAndroid>(this);
+  return session_android_.get();
+}
+#endif
+
 void MediaSessionImpl::WebContentsDestroyed() {
   // This should only work for tests. In production, all the players should have
   // already been removed before WebContents is destroyed.
@@ -831,7 +847,6 @@ MediaSessionImpl::MediaSessionImpl(WebContents* web_contents)
 #if defined(OS_ANDROID)
   session_android_.reset(new MediaSessionAndroid(this));
 #endif  // defined(OS_ANDROID)
-
   if (web_contents && web_contents->GetMainFrame() &&
       web_contents->GetMainFrame()->GetView()) {
     focused_ = web_contents->GetMainFrame()->GetView()->HasFocus();

content/browser/media/session/media_session_impl.h

@@ -85,11 +85,8 @@ class MediaSessionImpl : public MediaSession,
   ~MediaSessionImpl() override;
 
 #if defined(OS_ANDROID)
-  static MediaSession* FromJavaMediaSession(
-      const base::android::JavaRef<jobject>& j_media_session);
-  MediaSessionAndroid* session_android() const {
-    return session_android_.get();
-  }
+  void ClearMediaSessionAndroid();
+  MediaSessionAndroid* GetMediaSessionAndroid();
 #endif  // defined(OS_ANDROID)
 
   void NotifyMediaSessionMetadataChange();