Cancel rather than block navigations which violate portals 1P rules.

Since this can lead to a WebContents with no committed entries, deal
with that case in the browser-side Activate logic, by rejecting
activations which occur before we can say with certainty that we
will load a page (i.e. navigation commit).

Bug: 1046121,1034740,942198
Change-Id: Icd219dc0bb1e4a9ab6fdc433f8644c14e69964e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2015768Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Lucas Gadani <lfg@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#736369}

content/browser/frame_host/render_frame_host_impl.cc

@@ -1280,6 +1280,8 @@ void RenderFrameHostImpl::OnPortalActivated(
                 break;
               case blink::mojom::PortalActivateResult::
                   kRejectedDueToPredecessorNavigation:
+              case blink::mojom::PortalActivateResult::
+                  kRejectedDueToPortalNotReady:
               case blink::mojom::PortalActivateResult::kDisconnected:
               case blink::mojom::PortalActivateResult::kAbortedDueToBug:
                 // The renderer is misbehaving.

content/browser/portal/portal.cc

@@ -320,6 +320,15 @@ void Portal::Activate(blink::TransferableMessage data,
       << "The binding should have been closed when the portal's outer "
          "FrameTreeNode was deleted due to swap out.";
 
+  // If no navigation has yet committed in the portal, it cannot be activated as
+  // this would lead to an empty tab contents (without even an about:blank).
+  DCHECK(portal_contents_);
+  if (portal_contents_->GetController().GetLastCommittedEntryIndex() < 0) {
+    std::move(callback).Run(
+        blink::mojom::PortalActivateResult::kRejectedDueToPortalNotReady);
+    return;
+  }
+
   // If a navigation in the main frame is occurring, stop it if possible and
   // reject the activation if it's too late. There are a few cases here:
   // - a different RenderFrameHost has been assigned to the FrameTreeNode

content/browser/portal/portal_navigation_throttle.cc

@@ -75,7 +75,7 @@ PortalNavigationThrottle::WillStartOrRedirectRequest() {
       base::StringPrintf("Navigating a portal to cross-origin content (from "
                          "%s) is not currently permitted and was blocked.",
                          origin.Serialize().c_str()));
-  return BLOCK_REQUEST;
+  return CANCEL;
 }
 
 }  // namespace content

content/browser/portal/portal_navigation_throttle_browsertest.cc

@@ -14,6 +14,7 @@
 #include "content/public/test/content_browser_test.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "content/shell/browser/shell.h"
+#include "content/test/portal/portal_activated_observer.h"
 #include "content/test/portal/portal_created_observer.h"
 #include "net/base/escape.h"
 #include "net/dns/mock_host_resolver.h"
@@ -70,7 +71,7 @@ class PortalNavigationThrottleBrowserTest : public ContentBrowserTest {
 
   Portal* InsertAndWaitForPortal(const GURL& url,
                                  bool expected_to_succeed = true) {
-    TestNavigationObserver navigation_observer(url);
+    TestNavigationObserver navigation_observer(/*web_contents=*/nullptr, 1);
     navigation_observer.StartWatchingNewWebContents();
     PortalCreatedObserver portal_created_observer(GetMainFrame());
     EXPECT_TRUE(ExecJs(
@@ -84,6 +85,8 @@ class PortalNavigationThrottleBrowserTest : public ContentBrowserTest {
     navigation_observer.Wait();
     EXPECT_EQ(navigation_observer.last_navigation_succeeded(),
               expected_to_succeed);
+    if (expected_to_succeed)
+      EXPECT_EQ(navigation_observer.last_navigation_url(), url);
     return portal;
   }
 
@@ -97,7 +100,7 @@ class PortalNavigationThrottleBrowserTest : public ContentBrowserTest {
                base::StringPrintf(
                    "document.querySelector('body > portal').src = '%s';",
                    url.spec().c_str())));
-    navigation_observer.WaitForNavigationFinished();
+    navigation_observer.Wait();
     return navigation_observer.last_navigation_succeeded();
   }
 
@@ -109,7 +112,7 @@ class PortalNavigationThrottleBrowserTest : public ContentBrowserTest {
     EXPECT_TRUE(ExecJs(
         portal->GetPortalContents(),
         base::StringPrintf("location.href = '%s';", url.spec().c_str())));
-    navigation_observer.WaitForNavigationFinished();
+    navigation_observer.Wait();
     return navigation_observer.last_navigation_succeeded();
   }
 
@@ -190,14 +193,14 @@ IN_PROC_BROWSER_TEST_F(PortalNavigationThrottleBrowserTest,
   ASSERT_TRUE(NavigateToURL(
       GetWebContents(),
       embedded_test_server()->GetURL("portal.test", "/title1.html")));
-  Portal* portal = InsertAndWaitForPortal(
-      embedded_test_server()->GetURL("portal.test", "/title2.html"));
-
+  GURL referrer_url =
+      embedded_test_server()->GetURL("portal.test", "/title2.html");
   GURL destination_url =
       embedded_test_server()->GetURL("not.portal.test", "/notreached");
+
+  Portal* portal = InsertAndWaitForPortal(referrer_url);
   EXPECT_FALSE(NavigatePortalViaSrcAttribute(portal, destination_url, 1));
-  EXPECT_EQ(portal->GetPortalContents()->GetLastCommittedURL(),
-            destination_url);
+  EXPECT_EQ(portal->GetPortalContents()->GetLastCommittedURL(), referrer_url);
 }
 
 IN_PROC_BROWSER_TEST_F(PortalNavigationThrottleBrowserTest,
@@ -205,14 +208,14 @@ IN_PROC_BROWSER_TEST_F(PortalNavigationThrottleBrowserTest,
   ASSERT_TRUE(NavigateToURL(
       GetWebContents(),
       embedded_test_server()->GetURL("portal.test", "/title1.html")));
-  Portal* portal = InsertAndWaitForPortal(
-      embedded_test_server()->GetURL("portal.test", "/title2.html"));
-
+  GURL referrer_url =
+      embedded_test_server()->GetURL("portal.test", "/title2.html");
   GURL destination_url =
       embedded_test_server()->GetURL("not.portal.test", "/notreached");
+
+  Portal* portal = InsertAndWaitForPortal(referrer_url);
   EXPECT_FALSE(NavigatePortalViaLocationHref(portal, destination_url, 1));
-  EXPECT_EQ(portal->GetPortalContents()->GetLastCommittedURL(),
-            destination_url);
+  EXPECT_EQ(portal->GetPortalContents()->GetLastCommittedURL(), referrer_url);
 }
 
 IN_PROC_BROWSER_TEST_F(PortalNavigationThrottleBrowserTest,
@@ -220,16 +223,16 @@ IN_PROC_BROWSER_TEST_F(PortalNavigationThrottleBrowserTest,
   ASSERT_TRUE(NavigateToURL(
       GetWebContents(),
       embedded_test_server()->GetURL("portal.test", "/title1.html")));
-  Portal* portal = InsertAndWaitForPortal(
-      embedded_test_server()->GetURL("portal.test", "/title2.html"));
-
+  GURL referrer_url =
+      embedded_test_server()->GetURL("portal.test", "/title2.html");
   GURL destination_url =
       embedded_test_server()->GetURL("not.portal.test", "/notreached");
   GURL redirect_url = GetServerRedirectURL(embedded_test_server(),
                                            "portal.test", destination_url);
+
+  Portal* portal = InsertAndWaitForPortal(referrer_url);
   EXPECT_FALSE(NavigatePortalViaSrcAttribute(portal, redirect_url, 1));
-  EXPECT_EQ(portal->GetPortalContents()->GetLastCommittedURL(),
-            destination_url);
+  EXPECT_EQ(portal->GetPortalContents()->GetLastCommittedURL(), referrer_url);
 }
 
 IN_PROC_BROWSER_TEST_F(PortalNavigationThrottleBrowserTest,
@@ -237,15 +240,41 @@ IN_PROC_BROWSER_TEST_F(PortalNavigationThrottleBrowserTest,
   ASSERT_TRUE(NavigateToURL(
       GetWebContents(),
       embedded_test_server()->GetURL("portal.test", "/title1.html")));
-  Portal* portal = InsertAndWaitForPortal(
-      embedded_test_server()->GetURL("portal.test", "/title2.html"));
 
+  GURL referrer_url =
+      embedded_test_server()->GetURL("portal.test", "/title2.html");
   GURL destination_url =
       embedded_test_server()->GetURL("portal.test", "/notreached");
   GURL redirect_url = GetServerRedirectURL(embedded_test_server(),
                                            "not.portal.test", destination_url);
+
+  Portal* portal = InsertAndWaitForPortal(referrer_url);
   EXPECT_FALSE(NavigatePortalViaSrcAttribute(portal, redirect_url, 1));
-  EXPECT_EQ(portal->GetPortalContents()->GetLastCommittedURL(), redirect_url);
+  EXPECT_EQ(portal->GetPortalContents()->GetLastCommittedURL(), referrer_url);
+}
+
+IN_PROC_BROWSER_TEST_F(PortalNavigationThrottleBrowserTest,
+                       ActivateAfterCanceledInitialNavigation) {
+  ASSERT_TRUE(NavigateToURL(
+      GetWebContents(),
+      embedded_test_server()->GetURL("portal.test", "/title1.html")));
+  GURL referrer_url =
+      embedded_test_server()->GetURL("portal.test", "/title2.html");
+  GURL destination_url =
+      embedded_test_server()->GetURL("not.portal.test", "/notreached");
+
+  Portal* portal =
+      InsertAndWaitForPortal(destination_url, /*expected_to_succeed=*/false);
+  EXPECT_NE(portal, nullptr);
+
+  std::string result =
+      EvalJs(GetMainFrame(),
+             "document.querySelector('body > portal').activate()"
+             ".then(() => 'activated', e => e.message)")
+          .ExtractString();
+  EXPECT_THAT(result, ::testing::HasSubstr("not yet ready or was blocked"));
+  EXPECT_EQ(GetWebContents()->GetLastCommittedURL(),
+            embedded_test_server()->GetURL("portal.test", "/title1.html"));
 }
 
 IN_PROC_BROWSER_TEST_F(PortalNavigationThrottleBrowserTest,

third_party/blink/public/mojom/portal/portal.mojom

@@ -19,6 +19,10 @@ enum PortalActivateResult {
   // The predecessor was adopted by its successor.
   kPredecessorWasAdopted,
 
+  // The activation could not proceed because the portal had not yet begun
+  // to load a document, or its load was canceled.
+  kRejectedDueToPortalNotReady,
+
   // The activation could not proceed since the predecessor main frame was
   // navigating and the navigation could not be cancelled.
   kRejectedDueToPredecessorNavigation,

third_party/blink/renderer/core/html/portal/portal_contents.cc

@@ -71,6 +71,23 @@ ScriptPromise PortalContents::Activate(ScriptState* script_state,
 
 void PortalContents::OnActivateResponse(
     mojom::blink::PortalActivateResult result) {
+  auto reject = [&](DOMExceptionCode code, const char* message) {
+    if (GetDocument().IsContextDestroyed())
+      return;
+
+    ScriptState* script_state = activate_resolver_->GetScriptState();
+    ScriptState::Scope scope(script_state);
+    // TODO(jbroman): It's slightly unfortunate to hard-code the string
+    // HTMLPortalElement here. Ideally this would be threaded through from
+    // there and carried with the ScriptPromiseResolver. See
+    // https://crbug.com/991544.
+    ExceptionState exception_state(script_state->GetIsolate(),
+                                   ExceptionState::kExecutionContext,
+                                   "HTMLPortalElement", "activate");
+    exception_state.ThrowDOMException(code, message);
+    activate_resolver_->Reject(exception_state);
+  };
+
   bool should_destroy_contents = false;
   switch (result) {
     case mojom::blink::PortalActivateResult::kPredecessorWasAdopted:
@@ -82,24 +99,14 @@ void PortalContents::OnActivateResponse(
       break;
 
     case mojom::blink::PortalActivateResult::
-        kRejectedDueToPredecessorNavigation: {
-      if (!GetDocument().IsContextDestroyed()) {
-        ScriptState* script_state = activate_resolver_->GetScriptState();
-        ScriptState::Scope scope(script_state);
-        // TODO(jbroman): It's slightly unfortunate to hard-code the string
-        // HTMLPortalElement here. Ideally this would be threaded through from
-        // there and carried with the ScriptPromiseResolver. See
-        // https://crbug.com/991544.
-        ExceptionState exception_state(script_state->GetIsolate(),
-                                       ExceptionState::kExecutionContext,
-                                       "HTMLPortalElement", "activate");
-        exception_state.ThrowDOMException(
-            DOMExceptionCode::kInvalidStateError,
-            "A top-level navigation is in progress.");
-        activate_resolver_->Reject(exception_state);
-      }
+        kRejectedDueToPredecessorNavigation:
+      reject(DOMExceptionCode::kInvalidStateError,
+             "A top-level navigation is in progress.");
+      break;
+    case mojom::blink::PortalActivateResult::kRejectedDueToPortalNotReady:
+      reject(DOMExceptionCode::kInvalidStateError,
+             "The portal was not yet ready or was blocked.");
       break;
-    }
     case mojom::blink::PortalActivateResult::kDisconnected:
       // Only called when |remote_portal_| is disconnected. This usually happens
       // when the browser/test runner is being shut down.

third_party/blink/web_tests/external/wpt/portals/predecessor-fires-unload.html

@@ -17,6 +17,7 @@ promise_test(async () => {
     const portal = w.document.createElement('portal');
     portal.src = new URL('resources/simple-portal.html', location.href);
     w.document.body.appendChild(portal);
+    await nextEvent(portal, 'load');
     const pagehideFired = nextEvent(w, 'pagehide');
     const unloadFired = nextEvent(w, 'unload');
     await portal.activate();

third_party/blink/web_tests/external/wpt/portals/resources/predecessor-fires-unload-watch-unload.html

@@ -5,10 +5,11 @@ function nextEvent(target, type) {
   return new Promise((resolve, reject) => target.addEventListener(type, e => resolve(e), {once: true}));
 }
 
-onload = function() {
+onload = async function() {
   const portal = document.createElement('portal');
   portal.src = new URL('simple-portal.html', location.href);
   document.body.appendChild(portal);
+  await nextEvent(portal, 'load');
 
   let firedEvents = [];
   for (let type of ['pagehide', 'unload']) {
@@ -19,6 +20,6 @@ onload = function() {
   }
 
   portal.activate();
-}
+};
 </script>
 </body>

third_party/blink/web_tests/http/tests/portals/crbug-1031677.html

@@ -4,7 +4,8 @@
 // This test passes if it does not crash.
 // This does surprise the test runner by closing the main web contents.
 var portal = document.createElement("portal");
+portal.src = '/external/wpt/portals/resources/simple-portal.html';
 document.body.appendChild(portal);
-portal.activate();
+portal.onload = () => portal.activate();
 </script>
 </body>