CHECK that CommitNavigation/CommitFailedNavigation IPCs always commit.

Currently, state synchronization between the browser and the renderer
process is complicated and buggy for cross-process navigations. The
renderer process is responsible for processing the commit IPC and then
reporting success back up to the browser process, which then updates its
state. However, it's unclear if this can lead to races.

Instead, if it is possible to assume that one `CommitNavigation()` IPC
from the browser always maps to one committed navigation in the
renderer, the code can be simplified to remove multiphase navigation
commits. This means the browser would be able to mark a provisional
local frame as committed as soon as it sends a commit IPC to the
renderer for that frame.

As an initial step in this direction, this CL implements a renderer-side
`CHECK()` that a request to commit a navigation from the browser always
results in a committed navigation, with some initial exemptions for
fallback content for embedded content, et cetera.

However, this new `CHECK()` exposed some existing bugs in MHTMLx
handling. The browser side navigation code does not know enough about an
MHTML archive to determine if a commit will succeed or fail. Instead,
the browser always assumes that a subframe navigation request will
succeed and simply always calls `CommitNavigation()`. Unfortunately, the
renderer silently ignores commit requests in subframes if the associated
resource cannot be found in the MHTML archive, which triggers the new
`CHECK()`.

To fix this, committing a navigation to a non-existent MHTML resource
will now simply commit a document constructed from an empty string with
the text/html MIME type. This required updating the expectations for
some existing MHTML tests that tested loading of non-existent MHTML
resources. Before, the last committed URL would simply be empty, since
the commit would silently be dropped. After, the last committed URL is
now the URL of the non-existent resource.

Finally, NavigationMhtmlBrowserTest.IframeAboutBlankNotFound exhibited
a large number of complex interactions that still are not fully
understood. It looks like the renderer now reports load completion
*after* the about:blank fragment navigation completes, so update the
test expectations accordingly...

Bug: 999255
Change-Id: I2f9ac23b5668886b8b3bbbdcb7925f8784480c25
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2335323
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#796508}

content/browser/navigation_mhtml_browsertest.cc

@@ -158,11 +158,17 @@ IN_PROC_BROWSER_TEST_F(NavigationMhtmlBrowserTest, IframeNotFound) {
   EXPECT_TRUE(main_document->is_mhtml_document());
   EXPECT_FALSE(sub_document->is_mhtml_document());
 
-  // TODO(arthursonzogni): When the document is not found, the navigation never
-  // commit, even if we wait longer. Find out why.
-  EXPECT_FALSE(iframe_navigation_observer.has_committed());
+  // This should commit as a failed navigation, but the browser side doesn't
+  // have enough information to make that determination. On the renderer side,
+  // there's no existing way to turn `CommitNavigation()` into
+  // `CommitFailedNavigation()`.
+  // TODO(https://crbug.com/1112965): Fix this by implementing a MHTML
+  // URLLoaderFactory; then failure to find the resource can use the standard
+  // error handling path.
+  EXPECT_TRUE(iframe_navigation_observer.has_committed());
   EXPECT_FALSE(iframe_navigation_observer.is_error());
-  EXPECT_EQ(GURL(), sub_document->GetLastCommittedURL());
+  EXPECT_EQ(GURL("http://example.com/not_found.html"),
+            sub_document->GetLastCommittedURL());
 }
 
 // An MHTML document with an iframe using a data-URL. The data-URL is not
@@ -268,6 +274,8 @@ IN_PROC_BROWSER_TEST_F(NavigationMhtmlBrowserTest, IframeAboutBlankNotFound) {
   MhtmlArchive mhtml_archive;
   mhtml_archive.AddHtmlDocument(GURL("http://example.com"),
                                 "<iframe src=\"about:blank\"></iframe>"
+                                // Note: this is actually treated as a
+                                // same-document navigation!
                                 "<iframe src=\"about:blank#fragment\"></iframe>"
                                 "<iframe src=\"about:blank?query\"></iframe>");
   GURL mhtml_url = mhtml_archive.Write("index.mhtml");
@@ -280,8 +288,32 @@ IN_PROC_BROWSER_TEST_F(NavigationMhtmlBrowserTest, IframeAboutBlankNotFound) {
         ->current_frame_host()
         ->GetLastCommittedURL();
   };
+
+  // about:blank in MHTML has some very unusual behavior. When navigating to
+  // about:blank in the context of a MHTML archive, the renderer-side MHTML
+  // handler actually attempts to look up the resource for about:blank<...>" in
+  // the MHTML archive.
+  //
+  // Prior to https://crrev.com/c/2335323, failing to find the resource in the
+  // MHTML archive usually led to the commit being silently dropped (see
+  // `IframeNotFound` and `IframeContentIdNotFound`). However, about:blank
+  // behaved differently, due to a special case in frame_loader.cc's
+  // `ShouldNavigate()` for URLs that will load as an empty document.
+  //
+  // However, after https://crrev.com/c/23335323, loading about:blank without a
+  // corresponding resource in the MHTML archive will be treated as loading
+  // static data rather than loading an empty document. This affects the timing
+  // of load completion; loading an empty document synchronously completes
+  // during `CommitNavigation()`, while loading static data (even if the data is
+  // empty) completes "later".
   EXPECT_EQ(iframe_url(0), GURL("about:blank"));
-  EXPECT_EQ(iframe_url(1), GURL());  // TODO(arthursonzogni): Why is this empty?
+  // Note: unlike the other two subframe navigations, this navigation actually
+  // succeeds as a same-document navigation...
+  // Note 2: this same-document navigation is performed asynchronously. Prior to
+  // https://crrev.com/c/23335323, the test would consider the page as loaded
+  // before the fragment navigation completed, resulting in an empty last
+  // committed URL.
+  EXPECT_EQ(iframe_url(1), GURL("about:blank#fragment"));
   EXPECT_EQ(iframe_url(2), GURL("about:blank?query"));
 }
 
@@ -406,8 +438,15 @@ IN_PROC_BROWSER_TEST_F(NavigationMhtmlBrowserTest, IframeContentIdNotFound) {
   RenderFrameHostImpl* sub_document =
       main_document->child_at(0)->current_frame_host();
 
-  EXPECT_EQ(GURL(""), sub_document->GetLastCommittedURL());
-  EXPECT_FALSE(iframe_navigation.has_committed());
+  // This should commit as a failed navigation, but the browser side doesn't
+  // have enough information to make that determination. On the renderer side,
+  // there's no existing way to turn `CommitNavigation()` into
+  // `CommitFailedNavigation()`.
+  // TODO(https://crbug.com/1112965): Fix this by implementing a MHTML
+  // URLLoaderFactory; then failure to find the resource can use the standard
+  // error handling path.
+  EXPECT_EQ(GURL("cid:iframe"), sub_document->GetLastCommittedURL());
+  EXPECT_TRUE(iframe_navigation.has_committed());
   EXPECT_FALSE(iframe_navigation.is_error());
 }
 

content/renderer/render_frame_impl.h

@@ -1493,6 +1493,74 @@ class CONTENT_EXPORT RenderFrameImpl
   std::unique_ptr<blink::WebURLLoaderFactoryForTest>
       web_url_loader_factory_override_for_test_;
 
+  // When the browser asks the renderer to commit a navigation, it should always
+  // result in a committed navigation reported via DidCommitProvisionalLoad().
+  // This is important because DidCommitProvisionalLoad() is responsible for
+  // swapping in the provisional local frame during a cross-process navigation.
+  // Since this involves updating state in both the browser process and the
+  // renderer process, this assert ensures that the state remains synchronized
+  // between the two processes.
+  //
+  // Note: there is one exception that can result in no commit happening.
+  // Committing a navigation runs unload handlers, which can detach |this|. In
+  // that case, it doesn't matter that the navigation never commits, since the
+  // logical node for |this| has been removed from the DOM.
+  enum class NavigationCommitState {
+    // Represents the initial empty document. This is represented separately
+    // from |kNone| because Blink does not report the commit of the initial
+    // empty document in a newly created frame. However, note that there are
+    // some surprising quirks:
+    //
+    //   <iframe></iframe>
+    //
+    // will *not* be in the |kInitialEmptyDocument| state: while it initially
+    // starts at the initial empty document, the initial empty document is then
+    // synchronously replaced with a navigation to about:blank. In contrast:
+    //
+    //   <iframe src="https://slow.example.com"></iframe>
+    //
+    // will be in |kInitialEmptyDocument| until the navigation to
+    // https://slow.example.com commits.
+    kInitialEmptyDocument,
+    // No commit in progress. This state also implies that the frame is not
+    // displaying the initial empty document.
+    kNone,
+    // Marks that an active commit attempt is on the stack.
+    kWillCommit,
+    // Marks an active commit attempt as successful.
+    kDidCommit,
+  };
+
+  enum MayReplaceInitialEmptyDocumentTag {
+    kMayReplaceInitialEmptyDocument,
+  };
+
+  class CONTENT_EXPORT AssertNavigationCommits {
+   public:
+    // Construct a new scoper to verify that a navigation commit attempt
+    // succeeds. Asserts that:
+    // - no navigation is in progress
+    // - the frame is not displaying the initial empty document.
+    explicit AssertNavigationCommits(RenderFrameImpl* frame);
+
+    // Similar to the previous constructor but allows transitions from the
+    // initial empty document.
+    explicit AssertNavigationCommits(RenderFrameImpl* frame,
+                                     MayReplaceInitialEmptyDocumentTag);
+
+    ~AssertNavigationCommits();
+
+   private:
+    explicit AssertNavigationCommits(
+        RenderFrameImpl* frame,
+        bool allow_transition_from_initial_empty_document);
+
+    const base::WeakPtr<RenderFrameImpl> frame_;
+  };
+
+  NavigationCommitState navigation_commit_state_ =
+      NavigationCommitState::kInitialEmptyDocument;
+
   base::WeakPtrFactory<RenderFrameImpl> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(RenderFrameImpl);

content/test/test_render_frame.cc

@@ -338,6 +338,8 @@ void TestRenderFrame::Unload(
 void TestRenderFrame::BeginNavigation(
     std::unique_ptr<blink::WebNavigationInfo> info) {
   if (next_navigation_html_override_.has_value()) {
+    AssertNavigationCommits assert_navigation_commits(
+        this, kMayReplaceInitialEmptyDocument);
     auto navigation_params = blink::WebNavigationParams::CreateWithHTMLString(
         next_navigation_html_override_.value(), info->url_request.Url());
     next_navigation_html_override_ = base::nullopt;
@@ -347,6 +349,8 @@ void TestRenderFrame::BeginNavigation(
   }
   if (info->navigation_policy == blink::kWebNavigationPolicyCurrentTab &&
       GetWebFrame()->Parent() && info->form.IsNull()) {
+    AssertNavigationCommits assert_navigation_commits(
+        this, kMayReplaceInitialEmptyDocument);
     // RenderViewTest::LoadHTML immediately commits navigation for the main
     // frame. However if the loaded html has an empty or data subframe,
     // BeginNavigation will be called from Blink and we should avoid

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

@@ -893,6 +893,14 @@ static void FillStaticResponseIfNeeded(WebNavigationParams* params,
           params, archive_resource->MimeType(),
           archive_resource->TextEncoding(),
           base::make_span(archive_data->Data(), archive_data->size()));
+    } else {
+      // The requested archive resource does not exist. In an ideal world, this
+      // would commit as a failed navigation, but the browser doesn't know
+      // anything about what resources are available in the archive. Just
+      // synthesize an empty document so that something commits still.
+      // TODO(https://crbug.com/1112965): remove these special cases by adding
+      // an URLLoaderFactory implementation for MHTML archives.
+      WebNavigationParams::FillStaticResponse(params, "text/html", "UTF-8", "");
     }
   }
 }