Always commit same-document navigations to the current RenderFrameHost

Step 4 for bug 1125106. This is a subset of the mega-patch in
https://chromium-review.googlesource.com/c/chromium/src/+/2462248.

A same-document navigation is a request to navigate the currently loaded
document to the specified anchor tag from the URL. Renderer-initiated
navigations simply perform the scroll operation and update history.
Browser-initiated ones are treated as a navigation. However there are
many things that could cause the given URL to want to load in a
different SiteInstance than what was used for the previous load. But
using a different SiteInstance would also mean reloading the document -
which means it is no longer a same-document navigation.

Previous patches in this series have prevented us from trying to load a
same-document navigation when there would be valid reasons to want to do
a cross-document navigation.

Follow-up patches will introduce CHECK()s in the renderer to show that
we are in fact never trying to perform a same-document navigation
anymore when we would have required a cross-document navigation (with
the exception of navigating a frameset which must fallback to cross-
document).

R=nasko@chromium.org

Bug: 1125106
Change-Id: I3603cb0a3698d9e6241aca4eb8895db4fcdfd139
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2558760
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#833067}

content/browser/renderer_host/navigation_request.cc

@@ -1423,43 +1423,51 @@ void NavigationRequest::BeginNavigation() {
     is_loaded_from_mhtml_archive_ = IsForMhtmlSubframe();
     ComputeSandboxFlagsToCommit();
 
-    // Select an appropriate RenderFrameHost.
-    std::string frame_host_choice_reason;
-    render_frame_host_ =
-        frame_tree_node_->render_manager()->GetFrameHostForNavigation(
-            this, &frame_host_choice_reason);
-
-    // TODO(crbug.com/1116320): Remove the ad-hoc |frame_host_choice_reason| and
-    // other crash keys once the bug investigation completes.  Note that the
-    // crash related to crbug/1116320 is expected to happen inside the call to
-    // CommitNavigation below, a few statements down.
-    SCOPED_CRASH_KEY_STRING256("nav_request", host_choice_reason,
-                               frame_host_choice_reason);
-    SCOPED_CRASH_KEY_BOOL("nav_request", has_source_instance,
-                          !!GetSourceSiteInstance());
-    // Crash keys capturing values affecting |was_opener_suppressed| in
-    // RequiresInitiatorBasedSourceSiteInstance:
-    SCOPED_CRASH_KEY_BOOL("nav_request", is_main_frame, IsInMainFrame());
-    SCOPED_CRASH_KEY_BOOL(
-        "nav_request", got_initiator_routing_id,
-        GetInitiatorRoutingId().frame_routing_id != MSG_ROUTING_NONE);
-    SCOPED_CRASH_KEY_BOOL("nav_request", is_renderer_initiated,
-                          IsRendererInitiated());
-    // Crash keys capturing values affecting whether
-    // SetSourceSiteInstanceToInitiatorIfNeeded is called:
-    SCOPED_CRASH_KEY_BOOL("nav_request", from_begin_navigation,
-                          from_begin_navigation_);
-    SCOPED_CRASH_KEY_NUMBER("nav_request", navigation_type,
-                            static_cast<int>(common_params().navigation_type));
-    SCOPED_CRASH_KEY_BOOL(
-        "nav_request", is_hist_nav_in_new_child,
-        common_params().is_history_navigation_in_new_child_frame);
-    SCOPED_CRASH_KEY_BOOL("nav_request", has_nav_entry, !!GetNavigationEntry());
-
-    if (!Navigator::CheckWebUIRendererDoesNotDisplayNormalURL(
-            render_frame_host_, GetUrlInfo(),
-            /* is_renderer_initiated_check */ false)) {
-      CHECK(false);
+    // Same-document navigations occur in the currently loaded document. See
+    // also RenderFrameHostManager::DidCreateNavigationRequest() which will
+    // expect us to use the current RenderFrameHost for this NavigationRequest,
+    // and https://crbug.com/1125106.
+    if (IsSameDocument()) {
+      render_frame_host_ = frame_tree_node_->current_frame_host();
+    } else {
+      // Select an appropriate RenderFrameHost.
+      std::string frame_host_choice_reason;
+      render_frame_host_ =
+          frame_tree_node_->render_manager()->GetFrameHostForNavigation(
+              this, &frame_host_choice_reason);
+
+      // TODO(crbug.com/1116320): Remove the ad-hoc |frame_host_choice_reason|
+      // and other crash keys once the bug investigation completes.  Note that
+      // the crash related to crbug/1116320 is expected to happen inside the
+      // call to CommitNavigation below, a few statements down.
+      SCOPED_CRASH_KEY_STRING256("nav_request", host_choice_reason,
+                                 frame_host_choice_reason);
+      SCOPED_CRASH_KEY_BOOL("nav_request", has_source_instance,
+                            !!GetSourceSiteInstance());
+      // Crash keys capturing values affecting |was_opener_suppressed| in
+      // RequiresInitiatorBasedSourceSiteInstance:
+      SCOPED_CRASH_KEY_BOOL("nav_request", is_main_frame, IsInMainFrame());
+      SCOPED_CRASH_KEY_BOOL(
+          "nav_request", got_initiator_routing_id,
+          GetInitiatorRoutingId().frame_routing_id != MSG_ROUTING_NONE);
+      SCOPED_CRASH_KEY_BOOL("nav_request", is_renderer_initiated,
+                            IsRendererInitiated());
+      // Crash keys capturing values affecting whether
+      // SetSourceSiteInstanceToInitiatorIfNeeded is called:
+      SCOPED_CRASH_KEY_BOOL("nav_request", from_begin_navigation,
+                            from_begin_navigation_);
+      SCOPED_CRASH_KEY_NUMBER(
+          "nav_request", navigation_type,
+          static_cast<int>(common_params().navigation_type));
+      SCOPED_CRASH_KEY_BOOL(
+          "nav_request", is_hist_nav_in_new_child,
+          common_params().is_history_navigation_in_new_child_frame);
+      SCOPED_CRASH_KEY_BOOL("nav_request", has_nav_entry,
+                            !!GetNavigationEntry());
+
+      CHECK(Navigator::CheckWebUIRendererDoesNotDisplayNormalURL(
+          render_frame_host_, GetUrlInfo(),
+          /*is_renderer_initiated_check=*/false));
     }
 
     ReadyToCommitNavigation(false /* is_error */);
@@ -2411,7 +2419,8 @@ void NavigationRequest::OnResponseStarted(
   } else {
     render_frame_host_ = nullptr;
   }
-  DCHECK(render_frame_host_ || !response_should_be_rendered_);
+  if (!render_frame_host_)
+    DCHECK(!response_should_be_rendered_);
 
   if (render_frame_host_) {
     DetermineOriginIsolationEndResult(IsOptInIsolationRequested(GetURL()));

content/browser/renderer_host/render_frame_host_manager.cc

@@ -698,7 +698,14 @@ void RenderFrameHostManager::DidCreateNavigationRequest(
   const bool force_use_current_render_frame_host =
       // Since the frame from the back-forward cache is being committed to the
       // SiteInstance we already have, it is treated as current.
-      request->IsServedFromBackForwardCache();
+      request->IsServedFromBackForwardCache() ||
+      // Avoid calling GetFrameHostForNavigation() for same-document navigations
+      // since they should always occur in the current document, which means
+      // also in the current SiteInstance.
+      // State may have changed in the browser that would cause us to put the
+      // document in a different SiteInstance if it was loaded again now, but we
+      // do not want to load the document again, see https://crbug.com/1125106.
+      request->IsSameDocument();
 
   if (force_use_current_render_frame_host) {
     // This method should generally be calling GetFrameHostForNavigation() in
@@ -768,6 +775,12 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
       << "Don't call this method for JavaScript URLs as those create a "
          "temporary  NavigationRequest and we don't want to reset an ongoing "
          "navigation's speculative RFH.";
+  // Same-document navigations should be committed in the current document
+  // (and current RenderFrameHost), so we should not come here and ask where
+  // we would load that document. The resulting SiteInstance may have changed
+  // since we did load the current document, but we don't want to reload it if
+  // that is the case. See crbug.com/1125106.
+  DCHECK(!request->IsSameDocument());
   // Inactive frames should never be navigated. If this happens, log a
   // DumpWithoutCrashing to understand the root cause. See
   // https://crbug.com/926820 and https://crbug.com/927705.

content/test/data/empty404.html.mock-http-headers

+HTTP/1.0 404 Not Found
+Content-type: text/html
+Content-Length: 0