Portals: Don't cancel navigations started while orphaned

When we cancel an existing navigation upon portal activation, we've also
been inadvertently canceling navigations initiated immediately following
the predecessor's call to activate.

We now only cancel the existing navigations. This also unearthed some
issues that occur when a navigation which started when orphaned commits.
We also fix those.

Bug: 1058455
Change-Id: I78cdb0ebf548d6b9710ab8315f232ca1179817e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2166386Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#763551}

content/browser/portal/portal.cc

@@ -157,6 +157,18 @@ RenderFrameProxyHost* Portal::CreateProxyAndAttachPortal() {
       portal_contents_.ReleaseOwnership(), outer_node->current_frame_host(),
       false /* is_full_page */);
 
+  // If a cross-process navigation started while the predecessor was orphaned,
+  // we need to create a view for the speculative RFH as well.
+  if (RenderFrameHostImpl* speculative_rfh =
+          portal_contents_->GetPendingMainFrame()) {
+    if (RenderWidgetHostViewBase* view = static_cast<RenderWidgetHostViewBase*>(
+            speculative_rfh->GetView())) {
+      view->Destroy();
+    }
+    portal_contents_->CreateRenderWidgetHostViewForRenderManager(
+        speculative_rfh->render_view_host());
+  }
+
   FrameTreeNode* frame_tree_node =
       portal_contents_->GetMainFrame()->frame_tree_node();
   RenderFrameProxyHost* proxy_host =
@@ -366,9 +378,7 @@ void Portal::Activate(blink::TransferableMessage data,
                                 kRejectedDueToPredecessorNavigation);
     return;
   }
-  // TODO(1058455): This also cancels navigations requested immediately after
-  // the predecessor calls activate. We should only cancel existing navigations.
-  outer_root_node->StopLoading();
+  outer_root_node->navigator()->CancelNavigation(outer_root_node);
 
   DCHECK(!is_closing_) << "Portal should not be shutting down when contents "
                           "ownership is yielded";
@@ -399,6 +409,7 @@ void Portal::Activate(blink::TransferableMessage data,
 
   auto* outer_contents_main_frame_view = static_cast<RenderWidgetHostViewBase*>(
       outer_contents->GetMainFrame()->GetView());
+  DCHECK(!outer_contents->GetPendingMainFrame());
   auto* portal_contents_main_frame_view =
       static_cast<RenderWidgetHostViewBase*>(
           successor_contents_raw->GetMainFrame()->GetView());

content/browser/portal/portal_browsertest.cc

@@ -3,13 +3,16 @@
 // found in the LICENSE file.
 
 #include <memory>
+#include <tuple>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/memory/ptr_util.h"
+#include "base/strings/stringprintf.h"
 #include "base/test/bind_test_util.h"
 #include "base/test/scoped_feature_list.h"
+#include "base/test/test_timeouts.h"
 #include "build/build_config.h"
 #include "components/viz/host/host_frame_sink_manager.h"
 #include "content/browser/compositor/surface_utils.h"
@@ -1158,30 +1161,92 @@ IN_PROC_BROWSER_TEST_F(PortalBrowserTest, RemovePortalWhenUnloading) {
   EXPECT_TRUE(ExecJs(main_frame, "div.remove();"));
 }
 
+class PortalOrphanedNavigationBrowserTest
+    : public PortalBrowserTest,
+      public testing::WithParamInterface<std::tuple<bool, bool>> {
+ public:
+  PortalOrphanedNavigationBrowserTest()
+      : cross_site_(std::get<0>(GetParam())),
+        commit_after_adoption_(std::get<1>(GetParam())) {}
+
+  // Provides meaningful param names instead of /0, /1, ...
+  static std::string DescribeParams(
+      const testing::TestParamInfo<ParamType>& info) {
+    bool cross_site;
+    bool commit_after_adoption;
+    std::tie(cross_site, commit_after_adoption) = info.param;
+    return base::StringPrintf("%sSite_Commit%sAdoption",
+                              cross_site ? "Cross" : "Same",
+                              commit_after_adoption ? "After" : "Before");
+  }
+
+ protected:
+  bool cross_site() const { return cross_site_; }
+  bool commit_after_adoption() const { return commit_after_adoption_; }
+
+ private:
+  // Whether the predecessor navigates cross site while orphaned.
+  const bool cross_site_;
+  // Whether the predecessor's navigation commits before or after adoption.
+  const bool commit_after_adoption_;
+};
+
+INSTANTIATE_TEST_SUITE_P(All,
+                         PortalOrphanedNavigationBrowserTest,
+                         testing::Combine(testing::Bool(), testing::Bool()),
+                         PortalOrphanedNavigationBrowserTest::DescribeParams);
+
 // Tests that a portal can navigate while orphaned.
-IN_PROC_BROWSER_TEST_F(PortalBrowserTest, OrphanedNavigation) {
-  EXPECT_TRUE(NavigateToURL(
-      shell(), embedded_test_server()->GetURL("portal.test", "/title1.html")));
+IN_PROC_BROWSER_TEST_P(PortalOrphanedNavigationBrowserTest,
+                       OrphanedNavigation) {
+  GURL main_url(embedded_test_server()->GetURL("portal.test", "/title1.html"));
+  ASSERT_TRUE(NavigateToURL(shell(), main_url));
   WebContentsImpl* web_contents_impl =
       static_cast<WebContentsImpl*>(shell()->web_contents());
-  RenderFrameHostImpl* main_frame = web_contents_impl->GetMainFrame();
 
-  GURL a_url(embedded_test_server()->GetURL("a.com", "/title1.html"));
-  Portal* portal = CreatePortalToUrl(web_contents_impl, a_url);
-  WebContentsImpl* portal_contents = portal->GetPortalContents();
+  GURL portal_url(embedded_test_server()->GetURL("a.com", "/title1.html"));
+  Portal* portal = CreatePortalToUrl(web_contents_impl, portal_url);
 
-  // Block the activate callback so that the predecessor portal stays orphaned.
-  EXPECT_TRUE(ExecJs(portal_contents->GetMainFrame(),
-                     "window.onportalactivate = e => { while(true) {} };"));
+  GURL predecessor_nav_url(embedded_test_server()->GetURL(
+      cross_site() ? "b.com" : "portal.test", "/title2.html"));
+
+  if (commit_after_adoption()) {
+    // Block the activate callback so that there is ample time to start the
+    // navigation while orphaned.
+    // TODO(mcnee): Ideally, we would have a test interceptor to precisely
+    // control when to proceed with adoption.
+    const int adoption_delay = TestTimeouts::tiny_timeout().InMilliseconds();
+    EXPECT_TRUE(
+        ExecJs(portal->GetPortalContents()->GetMainFrame(),
+               JsReplace("window.onportalactivate = e => {"
+                         "  let end = performance.now() + $1;"
+                         "  while (performance.now() < end);"
+                         "  document.body.appendChild(e.adoptPredecessor());"
+                         "};",
+                         adoption_delay)));
+  } else {
+    // Block the activate callback so that the predecessor portal stays
+    // orphaned.
+    EXPECT_TRUE(ExecJs(portal->GetPortalContents()->GetMainFrame(),
+                       "window.onportalactivate = e => { while(true) {} };"));
+  }
 
   // Activate the portal and navigate the predecessor.
   PortalActivatedObserver activated_observer(portal);
-  TestNavigationObserver main_frame_navigation_observer(web_contents_impl);
-  ExecuteScriptAsync(main_frame,
-                     "document.querySelector('portal').activate();"
-                     "window.location.reload()");
+  TestNavigationManager navigation_manager(web_contents_impl,
+                                           predecessor_nav_url);
+  ExecuteScriptAsync(web_contents_impl->GetMainFrame(),
+                     JsReplace("document.querySelector('portal').activate();"
+                               "window.location.href = $1;",
+                               predecessor_nav_url));
   activated_observer.WaitForActivate();
-  main_frame_navigation_observer.Wait();
+  if (commit_after_adoption()) {
+    ASSERT_TRUE(navigation_manager.WaitForRequestStart());
+    EXPECT_EQ(blink::mojom::PortalActivateResult::kPredecessorWasAdopted,
+              activated_observer.WaitForActivateResult());
+  }
+  navigation_manager.WaitForNavigationFinished();
+  EXPECT_TRUE(navigation_manager.was_successful());
 }
 
 // Tests that the browser doesn't crash if the renderer tries to create the

content/browser/portal/portal_navigation_throttle_browsertest.cc

@@ -332,16 +332,9 @@ IN_PROC_BROWSER_TEST_F(PortalNavigationThrottleBrowserTest,
                      "});"));
 
   TestNavigationObserver navigation_observer(predecessor_contents);
-  // TODO(1058455): Due to a race, navigating immediately after calling
-  // |portal.activate()| always gets canceled. This test would correctly fail
-  // regardless due to an initial request to /notreached, however these next
-  // assertions about whether the navigation was cancelled would trivially pass.
-  // We use a setTimeout for now to make these assertions meaningful.
   EXPECT_TRUE(ExecJs(predecessor_contents,
                      JsReplace("document.querySelector('portal').activate();"
-                               "setTimeout(() => {"
-                               "  location.href = $1;"
-                               "});",
+                               "location.href = $1;",
                                orphan_navigation_url)));
   navigation_observer.Wait();
   EXPECT_FALSE(navigation_observer.last_navigation_succeeded());

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

@@ -91,7 +91,8 @@ void PortalContents::OnActivateResponse(
   bool should_destroy_contents = false;
   switch (result) {
     case mojom::blink::PortalActivateResult::kPredecessorWasAdopted:
-      GetDocument().GetPage()->SetInsidePortal(true);
+      if (!GetDocument().IsContextDestroyed())
+        GetDocument().GetPage()->SetInsidePortal(true);
       FALLTHROUGH;
     case mojom::blink::PortalActivateResult::kPredecessorWillUnload:
       activate_resolver_->Resolve();