From 0aa2189d717b991f4bf223659049a83e57985190 Mon Sep 17 00:00:00 2001
From: Phil Yan <[email protected]>
Date: Mon, 15 Jun 2026 13:10:37 -0700
Subject: [PATCH] [Doc-PiP] Fix Android Document PiP async lifecycle race

An asynchronous startup gap on Android allowed the opener tab to
navigate or close without tearing down the Document PiP window. When a
close request occurred during the Intent startup phase (before the
Java WebContentsDelegate attached), WebContentsImpl::ClosePage()
silently no-oped, while the C++ controller detached from the opener.

Repo example: http://crbug.com/521615681#comment3

This CL fixes the race condition by introducing a two-way handshake:
1. Java registers its Activity with PictureInPictureWindowManager JNI,
   passing its WebContents to verify session ownership. If the C++
   session was closed while the intent was in flight, startup aborts.
2. If C++ closes the session before the delegate has attached,
   it explicitly tells the Java Activity to finish() via JNI.

To keep the platform-specific logic clean, the JNI bridge registers a
PictureInPictureWindowManager::Observer to monitor session exits and
holds a JavaObjectWeakGlobalRef to the Activity, avoiding memory leaks
and keeping the C++ manager completely platform-agnostic.

Bug: 521615681
Change-Id: Ie58c6da77e78546dbec17d134d44ede0f8adad20
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7914193
Reviewed-by: Wenyu Fu <[email protected]>
Commit-Queue: Phil Yan <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1647023}
---

diff --git a/chrome/android/java/src/org/chromium/chrome/browser/media/DocumentPictureInPictureActivity.java b/chrome/android/java/src/org/chromium/chrome/browser/media/DocumentPictureInPictureActivity.java
index f52292c..f4f45e5 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/media/DocumentPictureInPictureActivity.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/media/DocumentPictureInPictureActivity.java
@@ -25,6 +25,7 @@
 import androidx.annotation.VisibleForTesting;
 import androidx.appcompat.app.AppCompatDelegate;
 
+import org.jni_zero.CalledByNative;
 import org.jni_zero.NativeMethods;
 
 import org.chromium.base.AconfigFlaggedApiDelegate;
@@ -142,6 +143,18 @@
         }
 
         mWebContents = webContents;
+
+        // Handshake with native WindowManager. Since Activity startup is async, C++ might have
+        // closed the session while this intent was in-flight. If so, abort to prevent an orphaned
+        // window.
+        boolean isSessionValid =
+                DocumentPictureInPictureActivityJni.get().registerJavaActivity(this, mWebContents);
+        if (!isSessionValid) {
+            Log.e(TAG, "Native PiP session already closed. Aborting startup.");
+            finish();
+            return;
+        }
+
         WebContents parentWebContents =
                 sParentWebContentsForTesting != null
                         ? sParentWebContentsForTesting
@@ -909,12 +922,22 @@
 
     @NativeMethods
     public interface Natives {
+        boolean registerJavaActivity(
+                DocumentPictureInPictureActivity activity, WebContents webContents);
+
         void onActivityStartForTesting( // IN-TEST
                 WebContents parentWebContent, WebContents webContents);
 
         void onBackToTab();
     }
 
+    @CalledByNative
+    public void closeActivity() {
+        if (!isFinishing()) {
+            finish();
+        }
+    }
+
     static class DocumentPictureInPictureNightModeStateProvider implements NightModeStateProvider {
         public void initialize(AppCompatDelegate delegate) {
             delegate.setLocalNightMode(AppCompatDelegate.MODE_NIGHT_YES);
diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/media/DocumentPictureInPictureActivityTest.java b/chrome/android/javatests/src/org/chromium/chrome/browser/media/DocumentPictureInPictureActivityTest.java
index e3ea963b..3d4b47c 100644
--- a/chrome/android/javatests/src/org/chromium/chrome/browser/media/DocumentPictureInPictureActivityTest.java
+++ b/chrome/android/javatests/src/org/chromium/chrome/browser/media/DocumentPictureInPictureActivityTest.java
@@ -279,6 +279,13 @@
                     ChromeTabUtils.loadUrlOnUiThread(mTab, dataUrl);
                 });
 
+        // Re-establish the native PiP session with the parent WebContents at its new URL.
+        ThreadUtils.runOnUiThreadBlocking(
+                () -> {
+                    DocumentPictureInPictureActivity.onActivityStartForTesting(
+                            mParentWebContents, mWebContents);
+                });
+
         // Launch the PiP activity. Its verifyOpenerOrigin() will check the origin of
         // mParentWebContents
         // (which is now opaque, serializing to "null") against the intent's initial opener origin
diff --git a/chrome/android/junit/src/org/chromium/chrome/browser/media/DocumentPictureInPictureActivityUnitTest.java b/chrome/android/junit/src/org/chromium/chrome/browser/media/DocumentPictureInPictureActivityUnitTest.java
index 379a2e08..8e071f1 100644
--- a/chrome/android/junit/src/org/chromium/chrome/browser/media/DocumentPictureInPictureActivityUnitTest.java
+++ b/chrome/android/junit/src/org/chromium/chrome/browser/media/DocumentPictureInPictureActivityUnitTest.java
@@ -11,6 +11,7 @@
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoInteractions;
@@ -58,6 +59,7 @@
     @Mock private AconfigFlaggedApiDelegate mAconfigFlaggedApiDelegate;
     @Mock private AppTask mAppTask;
     @Mock private DisplayAndroidManager mDisplayAndroidManager;
+    @Mock private DocumentPictureInPictureActivity.Natives mMockActivityNatives;
 
     private DocumentPictureInPictureActivity mActivity;
 
@@ -68,6 +70,7 @@
         DisplayAndroidManager.setInstanceForTesting(mDisplayAndroidManager);
         DisplayAndroid.setNonMultiDisplayForTesting(mDisplayAndroid);
         PictureInPictureBoundsCacheBridgeJni.setInstanceForTesting(mMockNatives);
+        DocumentPictureInPictureActivityJni.setInstanceForTesting(mMockActivityNatives);
 
         // Common Display setup
         when(mDisplayAndroid.getDipScale()).thenReturn(1.0f);
@@ -98,6 +101,7 @@
         AndroidTaskUtils.setAppTaskForTesting(null);
         DisplayAndroidManager.resetInstanceForTesting();
         DisplayAndroid.setNonMultiDisplayForTesting(null);
+        DocumentPictureInPictureActivityJni.setInstanceForTesting(null);
     }
 
     @Test
@@ -309,4 +313,41 @@
         // It should NOT cache these bounds.
         verifyNoInteractions(mMockNatives);
     }
+
+    @Test
+    @Config(sdk = Build.VERSION_CODES.S)
+    public void testPerformPreInflationStartup_AbortsIfSessionInvalid() {
+        // Mock registerJavaActivity to return false (indicating native session closed).
+        when(mMockActivityNatives.registerJavaActivity(any(), any())).thenReturn(false);
+
+        // Setup mock WebContents to prevent crashes during early setup.
+        WebContents webContents = mock(WebContents.class);
+        when(webContents.isDestroyed()).thenReturn(false);
+        DocumentPictureInPictureActivity.setWebContentsForTesting(webContents);
+
+        mActivity.performPreInflationStartup();
+
+        // Verify that performPreInflationStartup aborts early by calling finish().
+        verify(mActivity).finish();
+    }
+
+    @Test
+    @Config(sdk = Build.VERSION_CODES.S)
+    public void testCloseActivity() {
+        doReturn(false).when(mActivity).isFinishing();
+
+        mActivity.closeActivity();
+
+        verify(mActivity).finish();
+    }
+
+    @Test
+    @Config(sdk = Build.VERSION_CODES.S)
+    public void testCloseActivity_DoesNotFinishIfAlreadyFinishing() {
+        doReturn(true).when(mActivity).isFinishing();
+
+        mActivity.closeActivity();
+
+        verify(mActivity, never()).finish();
+    }
 }
diff --git a/chrome/browser/android/BUILD.gn b/chrome/browser/android/BUILD.gn
index 0bf3459b7..0d14775 100644
--- a/chrome/browser/android/BUILD.gn
+++ b/chrome/browser/android/BUILD.gn
@@ -268,7 +268,6 @@
       "initialize_feature_list_android.cc",
       "initialize_feature_list_android.h",
       "intent_handler.cc",
-      "media/document_picture_in_picture_bridge_android.cc",
       "media/media_capture_devices_dispatcher_android.cc",
       "media_state_observer.cc",
       "media_state_observer.h",
diff --git a/chrome/browser/android/media/document_picture_in_picture_bridge_android.cc b/chrome/browser/android/media/document_picture_in_picture_bridge_android.cc
deleted file mode 100644
index 38be33d..0000000
--- a/chrome/browser/android/media/document_picture_in_picture_bridge_android.cc
+++ /dev/null