Fix subframe history loads when both old and new FrameNavigationEntry is missing.

This CL fixes a regression introduced by the refactor in
https://chromium-review.googlesource.com/c/chromium/src/+/2181973.
Apparently, it's possible that a subframe may have neither the old nor
the new (target) FrameNavigationEntry during a history navigation.
Prior to the refactor above, we returned early and avoided scheduling
any loads for such a subframe.  After the refactor, we incorrectly
attempted to schedule a different-document load, which resulted in
calling CreateNavigationRequestFromEntry() with a null frame_entry,
which it didn't expect and crashed while deferencing it.

Bug: 1088354, 1088175
Change-Id: Iaaa9743b0e897eecb1aa0317fa20d8d042c878f9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2225267
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#773863}

content/browser/frame_host/navigation_controller_impl.cc

@@ -2772,6 +2772,22 @@ NavigationControllerImpl::DetermineActionForHistoryNavigation(
     return HistoryNavigationAction::kDifferentDocument;
   }
 
+  // If there is no new FrameNavigationEntry for the frame, ignore the
+  // load.  For example, this may happen when going back to an entry before a
+  // frame was created.  Suppose we commit a same-document navigation that also
+  // results in adding a new subframe somewhere in the tree.  If we go back,
+  // the new subframe will be missing a FrameNavigationEntry in the previous
+  // NavigationEntry, but we shouldn't delete or change what's loaded in
+  // it.
+  //
+  // It's important to check this before checking |old_item| below, since both
+  // might be null, and in that case we still shouldn't change what's loaded in
+  // this frame.  Note that scheduling any loads assumes that |new_item| is
+  // non-null.  See https://crbug.com/1088354.
+  FrameNavigationEntry* new_item = pending_entry_->GetFrameEntry(frame);
+  if (!new_item)
+    return HistoryNavigationAction::kNone;
+
   // If there is no old FrameNavigationEntry, schedule a different-document
   // load.
   //
@@ -2788,17 +2804,6 @@ NavigationControllerImpl::DetermineActionForHistoryNavigation(
   if (!old_item)
     return HistoryNavigationAction::kDifferentDocument;
 
-  // If there is no new FrameNavigationEntry for the frame, ignore the
-  // load.  For example, this may happen when going back to an entry before a
-  // frame was created.  Suppose we commit a same-document navigation that also
-  // results in adding a new subframe somewhere in the tree.  If we go back,
-  // the new subframe will be missing a FrameNavigationEntry in the previous
-  // NavigationEntry, but we shouldn't delete or change what's loaded in
-  // it.
-  FrameNavigationEntry* new_item = pending_entry_->GetFrameEntry(frame);
-  if (!new_item)
-    return HistoryNavigationAction::kNone;
-
   // If the new item is not in the same SiteInstance, schedule a
   // different-document load.  Newly restored items may not have a SiteInstance
   // yet, in which case it will be assigned on first commit.
@@ -3311,6 +3316,7 @@ NavigationControllerImpl::CreateNavigationRequestFromEntry(
     ReloadType reload_type,
     bool is_same_document_history_load,
     bool is_history_navigation_in_new_child_frame) {
+  DCHECK(frame_entry);
   GURL dest_url = frame_entry->url();
   base::Optional<url::Origin> origin_to_commit =
       frame_entry->committed_origin();

content/browser/frame_host/navigation_controller_impl_browsertest.cc

@@ -11048,4 +11048,51 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
   EXPECT_TRUE(ftn_c->current_frame_host()->IsRenderFrameLive());
 }
 
+// Regression test for https://crbug.com/1088354, where a different-document
+// load was incorrectly scheduled for a history navigation in a subframe that
+// had no existing and no target FrameNavigationEntry.
+IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
+                       SubframeGoesBackAndSiblingHasNoFrameEntry) {
+  // Start on a page with a same-site iframe.
+  GURL main_url =
+      embedded_test_server()->GetURL("a.com", "/page_with_iframe.html");
+  ASSERT_TRUE(NavigateToURL(shell(), main_url));
+  NavigationControllerImpl& controller = contents()->GetController();
+  FrameTreeNode* ftn_a = contents()->GetFrameTree()->root();
+  FrameTreeNode* ftn_b = ftn_a->child_at(0);
+  EXPECT_EQ(embedded_test_server()->GetURL("a.com", "/title1.html"),
+            ftn_b->current_frame_host()->GetLastCommittedURL());
+
+  // Add a second subframe dynamically and set some state on it to ensure it's
+  // not reloaded.  Using a javascript: URL results in the renderer not sending
+  // a DidCommitNavigation IPC back for the new frame, leaving it without a
+  // FrameNavigationEntry.
+  EXPECT_TRUE(ExecJs(ftn_a,
+                     "var f = document.createElement('iframe');"
+                     "f.src = 'javascript:void(0)';"
+                     "document.body.appendChild(f);"));
+  FrameTreeNode* ftn_c = ftn_a->child_at(1);
+  EXPECT_TRUE(ExecJs(ftn_c, "window.state='c';"));
+
+  // Navigate first subframe same-document.
+  {
+    FrameNavigateParamsCapturer capturer(ftn_b);
+    EXPECT_TRUE(ExecJs(ftn_b, "location.hash = 'foo'"));
+    capturer.Wait();
+    EXPECT_TRUE(capturer.is_same_document());
+  }
+
+  // Go back in the first subframe.  This should navigate the first subframe
+  // back same-document, while the second subframe shouldn't be reloaded, and
+  // the history navigation shouldn't crash while processing it.
+  {
+    FrameNavigateParamsCapturer capturer(ftn_b);
+    controller.GoBack();
+    capturer.Wait();
+    EXPECT_TRUE(capturer.is_same_document());
+    EXPECT_TRUE(WaitForLoadStop(contents()));
+    EXPECT_EQ("c", EvalJs(ftn_c, "window.state"));
+  }
+}
+
 }  // namespace content