Add renderer-side CHECK()s that we don't perform invalid same-doc navs

Step 6 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 should only be performed when the commit will
happen in the currently loaded document. The renderer handled the
browser doing the wrong thing in all kinds of ways, due to the browser
making incorrect decisions when:
- On an empty error page
- Having an ongoing cross-document navigation which hasn't committed yet

Now the browser does the right thing, so the renderer does not need to
handle the fallback - except in one case which is a frameset. In that
case the browser doesn't know the document is frameset, and the
renderer currently enforces a policy that same-document navigations
in a frameset document cause it to be reloaded.

This is done for both renderer-initiated navigations (by sending
the navigation to the browser process instead of performing the
navigation immediately) and then by bouncing any same-document
requests from the browser back as RestartCrossDocument.

R=nasko@chromium.org

Bug: 1125106
Change-Id: I74ca38a2213215a107c955127f1f664b96b58aaa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2558926
Auto-Submit: danakj <danakj@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843994}

content/renderer/render_frame_impl.cc

@@ -5224,19 +5224,15 @@ blink::mojom::CommitResult RenderFrameImpl::PrepareForHistoryNavigationCommit(
     // If this is marked as a same document load but we haven't committed
     // anything, we can't proceed with the load. The browser shouldn't let this
     // happen.
-    if (GetWebFrame()->GetCurrentHistoryItem().IsNull()) {
-      NOTREACHED();
-      return blink::mojom::CommitResult::RestartCrossDocument;
-    }
+    CHECK(!GetWebFrame()->GetCurrentHistoryItem().IsNull());
 
     // Additionally, if the current history item's document sequence number
     // doesn't match the one sent from the browser, it is possible that this
     // renderer has committed a different document. In such case, the navigation
-    // cannot be loaded as a same-document navigation.
-    if (GetWebFrame()->GetCurrentHistoryItem().DocumentSequenceNumber() !=
-        item_for_history_navigation->DocumentSequenceNumber()) {
-      return blink::mojom::CommitResult::RestartCrossDocument;
-    }
+    // cannot be loaded as a same-document navigation. The browser shouldn't let
+    // this happen.
+    CHECK_EQ(GetWebFrame()->GetCurrentHistoryItem().DocumentSequenceNumber(),
+             item_for_history_navigation->DocumentSequenceNumber());
   }
 
   // Note: we used to check that initial history navigation in the child frame

third_party/blink/renderer/core/loader/document_loader.cc

@@ -1115,19 +1115,25 @@ mojom::CommitResult DocumentLoader::CommitSameDocumentNavigation(
     page->HistoryNavigationVirtualTimePauser().UnpauseVirtualTime();
 
   if (!frame_->IsNavigationAllowed())
-    return mojom::CommitResult::Aborted;
+    return mojom::blink::CommitResult::Aborted;
+
+  if (frame_->GetDocument()->IsFrameSet()) {
+    // Navigations in a frameset are always cross-document. Renderer-initiated
+    // navigations in a frameset will be deferred to the browser, and all
+    // renderer-initiated navigations are treated as cross-document. So this one
+    // must have been browser-initiated, where it was not aware that the
+    // document is a frameset. In that case we just restart the navigation,
+    // making it cross-document. This gives a consistent outcome for all
+    // navigations in a frameset.
+    return mojom::blink::CommitResult::RestartCrossDocument;
+  }
 
   if (!IsBackForwardLoadType(frame_load_type)) {
     // In the case of non-history navigations, check that this is a
-    // same-document navigation. If not, the navigation should restart as a
-    // cross-document navigation.
-    if (!url.HasFragmentIdentifier() ||
-        !EqualIgnoringFragmentIdentifier(frame_->GetDocument()->Url(), url) ||
-        frame_->GetDocument()->IsFrameSet()) {
-      // TODO(danakj): Convert to a CHECK() and stop doing RestartCrossDocument.
-      NOTREACHED();
-      return mojom::CommitResult::RestartCrossDocument;
-    }
+    // same-document navigation. The browser should not send invalid
+    // same-document navigations.
+    CHECK(url.HasFragmentIdentifier());
+    CHECK(EqualIgnoringFragmentIdentifier(frame_->GetDocument()->Url(), url));
   }
 
   // If the requesting document is cross-origin, perform the navigation