From fe9db40e76673d7a9fa3a7f66fcbb9b34844edec Mon Sep 17 00:00:00 2001
From: Mohamed Adel <[email protected]>
Date: Fri, 11 Sep 2026 12:35:09 -0700
Subject: [PATCH] Improve lifecycle and navigation handling in PostMessageHandler

Update PostMessageHandler to properly track and clean up its
WebContentsObserver during resets and WebContents destruction. Ensure
that closeChannelAndForgetWebContents() cleans up cached state
regardless of whether a message channel was previously active.

Additionally, ensure navigation state advancement only occurs for
committed navigations, reset target URIs alongside source URIs on
cross-document navigations, and validate committed frame URLs before
extracting message origin metadata.

1. Defaulting the target origin to the verified origin: in
   onOriginVerified(), if mPostMessageTargetUri is not set it now
   defaults to origin.uri() rather than remaining null/wildcard.

2. Target origin matching on initialization: initializeWithWebContents()
   now validates that the active main frame's committed origin matches
   the target origin, via isTargetOriginMatchingWebContents(), before
   establishing the message channel.

TAG=agy

Fixed: 517150523
Change-Id: I7d1fc59133f85367c7f6b80ffe5786a5d952ef65
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8391560
Commit-Queue: Daniel Murphy <[email protected]>
Reviewed-by: Daniel Murphy <[email protected]>
Auto-Submit: Moe Adel <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1696513}
---
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/browserservices/PostMessageHandler.java b/chrome/android/java/src/org/chromium/chrome/browser/browserservices/PostMessageHandler.java
index 4d27daace..e8e36511 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/browserservices/PostMessageHandler.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/browserservices/PostMessageHandler.java
@@ -32,6 +32,7 @@
 import org.chromium.content_public.browser.MessagePort.MessageCallback;
 import org.chromium.content_public.browser.NavigationHandle;
 import org.chromium.content_public.browser.Page;
+import org.chromium.content_public.browser.RenderFrameHost;
 import org.chromium.content_public.browser.WebContents;
 import org.chromium.content_public.browser.WebContentsObserver;
 import org.chromium.net.GURLUtils;
@@ -51,7 +52,16 @@
     private final MessageCallback mMessageCallback;
     private final PostMessageBackend mPostMessageBackend;
     private @Nullable WebContents mWebContents;
+    private @Nullable WebContentsObserver mWebContentsObserver;
     private MessagePort @Nullable [] mChannel;
+
+    /**
+     * The origin of the document {@link #mChannel} was created for, captured at channel creation
+     * time. Resolving the origin lazily per message would be racy: a message can sit in the UI task
+     * queue while a navigation commits, which would attribute it to the wrong document.
+     */
+    private @Nullable String mChannelOrigin;
+
     private @Nullable Uri mPostMessageSourceUri;
     private @Nullable Uri mPostMessageTargetUri;
 
@@ -84,11 +94,9 @@
                     }
 
                     Bundle bundle = null;
-                    GURL url = mWebContents.getMainFrame().getLastCommittedURL();
-                    if (url != null) {
-                        String origin = GURLUtils.getOrigin(url.getSpec());
+                    if (mChannelOrigin != null && !mChannelOrigin.isEmpty()) {
                         bundle = new Bundle();
-                        bundle.putString(POST_MESSAGE_ORIGIN, origin);
+                        bundle.putString(POST_MESSAGE_ORIGIN, mChannelOrigin);
                     }
                     assumeNonNull(messagePayload.getAsString());
                     mPostMessageBackend.onPostMessage(messagePayload.getAsString(), bundle);
@@ -109,42 +117,107 @@
         }
         // Can't reset with the same web contents twice.
         if (webContents.equals(mWebContents)) return;
+        if (mWebContents != null) {
+            closeChannel();
+            if (mWebContentsObserver != null) {
+                mWebContentsObserver.observe(null);
+                mWebContentsObserver = null;
+            }
+        }
         mWebContents = webContents;
-        new WebContentsObserver(webContents) {
-            private boolean mNavigatedOnce;
+        mWebContentsObserver =
+                new WebContentsObserver(webContents) {
+                    // A reset can attach to a WebContents that has already committed a document,
+                    // for example when a hidden tab is promoted or a tab is swapped in. Starting
+                    // from false there would misread the next genuine cross-document navigation as
+                    // the initial one and leave the channel alive across the document change.
+                    private boolean mNavigatedOnce =
+                            !GURL.isEmptyOrInvalid(webContents.getLastCommittedUrl());
 
-            @Override
-            public void didFinishNavigationInPrimaryMainFrame(NavigationHandle navigation) {
-                if (mNavigatedOnce && navigation.hasCommitted() && !navigation.isSameDocument()) {
-                    mPostMessageSourceUri = null;
-                    closeChannel();
-                    return;
-                }
-                mNavigatedOnce = true;
-            }
+                    @Override
+                    public void didFinishNavigationInPrimaryMainFrame(NavigationHandle navigation) {
+                        if (mNavigatedOnce
+                                && navigation.hasCommitted()
+                                && !navigation.isSameDocument()) {
+                            mPostMessageSourceUri = null;
+                            mPostMessageTargetUri = null;
+                            closeChannel();
+                            return;
+                        }
+                        if (navigation.hasCommitted()) {
+                            mNavigatedOnce = true;
+                        }
+                    }
 
-            @Override
-            public void primaryMainFrameRenderProcessGone(
-                    @TerminationStatus int terminationStatus) {
-                closeChannelAndForgetWebContents();
-            }
+                    @Override
+                    public void primaryMainFrameRenderProcessGone(
+                            @TerminationStatus int terminationStatus) {
+                        closeChannelAndForgetWebContents();
+                    }
 
-            @Override
-            public void documentLoadedInPrimaryMainFrame(
-                    Page page,
-                    GlobalRenderFrameHostId rfhId,
-                    @LifecycleState int rfhLifecycleState) {
-                if (mChannel != null || mPostMessageSourceUri == null) {
-                    return;
-                }
-                initializeWithWebContents(webContents);
-            }
-        };
+                    @Override
+                    public void webContentsDestroyed() {
+                        closeChannelAndForgetWebContents();
+                    }
+
+                    @Override
+                    public void documentLoadedInPrimaryMainFrame(
+                            Page page,
+                            GlobalRenderFrameHostId rfhId,
+                            @LifecycleState int rfhLifecycleState) {
+                        if (mChannel != null || mPostMessageSourceUri == null) {
+                            return;
+                        }
+                        initializeWithWebContents(webContents);
+                    }
+                };
+    }
+
+    private boolean isTargetOriginMatchingWebContents(WebContents webContents) {
+        String target = mPostMessageTargetUri == null ? "" : mPostMessageTargetUri.toString();
+        // An empty or "*" target origin is a wildcard, as understood by
+        // WebContents#postMessageToMainFrame and the postMessage specification, so there is
+        // nothing to match against.
+        if (target.isEmpty() || "*".equals(target)) {
+            return true;
+        }
+        RenderFrameHost mainFrame = webContents.getMainFrame();
+        if (mainFrame == null) {
+            return false;
+        }
+        // A sandboxed frame has an opaque security context even though its URL still looks like a
+        // regular https:// URL, so the URL comparison below would wrongly match. Reject those
+        // outright rather than handing a port to a document that cannot be attributed to an
+        // origin.
+        org.chromium.url.Origin committedOrigin = mainFrame.getLastCommittedOrigin();
+        if (committedOrigin != null && committedOrigin.isOpaque()) {
+            return false;
+        }
+        GURL url = mainFrame.getLastCommittedURL();
+        if (url == null || url.isEmpty() || !url.isValid()) {
+            return false;
+        }
+        Origin currentOrigin = Origin.create(url.getSpec());
+        Origin targetOrigin = Origin.create(mPostMessageTargetUri);
+        if (currentOrigin == null || targetOrigin == null) {
+            return false;
+        }
+        return currentOrigin.equals(targetOrigin);
     }
 
     private void initializeWithWebContents(final WebContents webContents) {
+        if (mPostMessageSourceUri == null || webContents.isDestroyed()) return;
+        if (!isTargetOriginMatchingWebContents(webContents)) {
+            closeChannel();
+            return;
+        }
+        // Replace any existing channel without tearing down the backend: onDisconnectChannel()
+        // unbinds the PostMessageService, which would make the onNotifyMessageChannelReady() call
+        // below a no-op and leave the client waiting forever.
+        closeChannelPort();
         mChannel = webContents.createMessageChannel();
         mChannel[0].setMessageCallback(mMessageCallback, null);
+        mChannelOrigin = getCommittedOrigin(webContents);
 
         assumeNonNull(mPostMessageSourceUri);
         webContents.postMessageToMainFrame(
@@ -157,6 +230,32 @@
     }
 
     /**
+     * @return The origin of the primary main frame's committed document, in the same format
+     *     previously reported to clients via {@link #POST_MESSAGE_ORIGIN}, or null if it cannot be
+     *     determined.
+     */
+    private static @Nullable String getCommittedOrigin(WebContents webContents) {
+        RenderFrameHost mainFrame = webContents.getMainFrame();
+        if (mainFrame == null) return null;
+        GURL url = mainFrame.getLastCommittedURL();
+        if (url == null || url.isEmpty() || !url.isValid()) return null;
+        return GURLUtils.getOrigin(url.getSpec());
+    }
+
+    /**
+     * Drops the message channel without notifying the client. {@link MessagePort#close()} throws if
+     * the port has already been transferred, so guard against that.
+     */
+    private void closeChannelPort() {
+        if (mChannel == null) return;
+        if (!mChannel[0].isClosed() && !mChannel[0].isTransferred()) {
+            mChannel[0].close();
+        }
+        mChannel = null;
+        mChannelOrigin = null;
+    }
+
+    /**
      * Closes the message channel and notifies the client that it is gone, keeping the {@link
      * WebContents}. The client can re-establish messaging for the new document by calling
      * requestPostMessageChannel() again, which re-verifies the origin and re-enters {@link
@@ -164,8 +263,7 @@
      */
     private void closeChannel() {
         if (mChannel == null) return;
-        mChannel[0].close();
-        mChannel = null;
+        closeChannelPort();
         mPostMessageBackend.onDisconnectChannel(ContextUtils.getApplicationContext());
     }
 
@@ -175,12 +273,14 @@
      * channel can be re-established until {@link #reset} supplies a new {@link WebContents}.
      */
     private void closeChannelAndForgetWebContents() {
-        // The reference is only dropped when there was a live channel: keeping a stale
-        // WebContents otherwise is what makes reset() bail out early instead of attaching a
-        // second observer to the same WebContents.
-        if (mChannel == null) return;
         closeChannel();
+        if (mWebContentsObserver != null) {
+            mWebContentsObserver.observe(null);
+            mWebContentsObserver = null;
+        }
         mWebContents = null;
+        mPostMessageSourceUri = null;
+        mPostMessageTargetUri = null;
     }
 
     /**
@@ -188,10 +288,11 @@
      *
      * @param postMessageUri The postMessageUri value to be set.
      */
-    public void initializeWithPostMessageUri(Uri postMessageUri, @Nullable Uri targetOrigin) {
+    public void initializeWithPostMessageUri(
+            @Nullable Uri postMessageUri, @Nullable Uri targetOrigin) {
         mPostMessageSourceUri = postMessageUri;
         mPostMessageTargetUri = targetOrigin;
-        if (mWebContents != null && !mWebContents.isDestroyed()) {
+        if (mPostMessageSourceUri != null && mWebContents != null && !mWebContents.isDestroyed()) {
             initializeWithWebContents(mWebContents);
         }
     }
@@ -216,9 +317,12 @@
         PostTask.postTask(
                 TaskTraits.UI_DEFAULT,
                 () -> {
-                    // It is still possible that the page has navigated while this task is in
-                    // the queue. If that happens fail gracefully.
-                    if (mChannel == null || mChannel[0].isClosed()) return;
+                    // It is still possible that the page has navigated, or that the port has been
+                    // transferred, while this task is in the queue. Both make postMessage() throw,
+                    // so fail gracefully.
+                    if (mChannel == null || mChannel[0].isClosed() || mChannel[0].isTransferred()) {
+                        return;
+                    }
                     mChannel[0].postMessage(new MessagePayload(message), null);
                 });
         return CustomTabsService.RESULT_SUCCESS;
@@ -228,9 +332,10 @@
     public void onOriginVerified(
             String packageName, Origin origin, boolean result, @TriState int online) {
         if (!result) return;
+        Uri targetOrigin = mPostMessageTargetUri != null ? mPostMessageTargetUri : origin.uri();
         initializeWithPostMessageUri(
                 OriginVerifier.getPostMessageUriFromVerifiedOrigin(packageName, origin),
-                mPostMessageTargetUri);
+                targetOrigin);
     }
 
     /**
@@ -239,7 +344,7 @@
      *
      * @param postMessageTargetUri Uri to post the first message to.
      */
-    public void setPostMessageTargetUri(Uri postMessageTargetUri) {
+    public void setPostMessageTargetUri(@Nullable Uri postMessageTargetUri) {
         mPostMessageTargetUri = postMessageTargetUri;
     }
 
@@ -253,4 +358,12 @@
     public @Nullable Uri getPostMessageUriForTesting() {
         return mPostMessageSourceUri;
     }
+
+    public @Nullable WebContents getWebContentsForTesting() {
+        return mWebContents;
+    }
+
+    public @Nullable WebContentsObserver getWebContentsObserverForTesting() {
+        return mWebContentsObserver;
+    }
 }
diff --git a/chrome/android/junit/src/org/chromium/chrome/browser/browserservices/PostMessageHandlerTest.java b/chrome/android/junit/src/org/chromium/chrome/browser/browserservices/PostMessageHandlerTest.java
index b223173..79763f8 100644
--- a/chrome/android/junit/src/org/chromium/chrome/browser/browserservices/PostMessageHandlerTest.java
+++ b/chrome/android/junit/src/org/chromium/chrome/browser/browserservices/PostMessageHandlerTest.java
@@ -5,18 +5,24 @@
 package org.chromium.chrome.browser.browserservices;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.lenient;
+import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 import android.net.Uri;
+import android.os.Bundle;
 
 import androidx.browser.customtabs.CustomTabsService;
 import androidx.browser.customtabs.PostMessageBackend;
 
+import org.junit.After;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -26,18 +32,34 @@
 import org.mockito.Mock;
 import org.mockito.junit.MockitoJUnit;
 import org.mockito.junit.MockitoRule;
+import org.robolectric.shadows.ShadowLooper;
 
+import org.chromium.base.TerminationStatus;
+import org.chromium.base.TriState;
 import org.chromium.base.test.BaseRobolectricTestRunner;
+import org.chromium.components.embedder_support.util.Origin;
+import org.chromium.content_public.browser.GlobalRenderFrameHostId;
+import org.chromium.content_public.browser.LifecycleState;
+import org.chromium.content_public.browser.MessagePayload;
 import org.chromium.content_public.browser.MessagePort;
+import org.chromium.content_public.browser.MessagePort.MessageCallback;
 import org.chromium.content_public.browser.NavigationHandle;
+import org.chromium.content_public.browser.Page;
+import org.chromium.content_public.browser.RenderFrameHost;
 import org.chromium.content_public.browser.WebContents;
 import org.chromium.content_public.browser.WebContentsObserver;
+import org.chromium.mojo_base.mojom.UnguessableToken;
+import org.chromium.net.GURLUtils;
+import org.chromium.net.GURLUtilsJni;
+import org.chromium.url.GURL;
 
 /** Unit tests for {@link PostMessageHandler}. */
 @RunWith(BaseRobolectricTestRunner.class)
 public class PostMessageHandlerTest {
     private static final Uri SOURCE_URI = Uri.parse("android-app://org.chromium.test");
     private static final Uri TARGET_URI = Uri.parse("https://www.example.com");
+    private static final String POST_MESSAGE_ORIGIN =
+            "androidx.browser.customtabs.POST_MESSAGE_ORIGIN";
 
... (file diff truncated)