OOPIF: Fix SetPageState for ViewSource with subframes.

With subframe FrameNavigationEntries enabled, SetPageState creates FNEs
for each session history item in the PageState.  It's not meant to be
called after the page has loaded.

However, some callers expect to call it after subframe FNEs already
exist (e.g., after a Clone in ViewSource).  For these cases, we need to
clear the current subframe FNEs.

Also update the unit test framework to support AUTO_SUBFRAME commits.

BUG=579134
TEST=No DCHECK for ViewSource with subframe in --site-per-process.

Review URL: https://codereview.chromium.org/1587363006

Cr-Commit-Position: refs/heads/master@{#370230}

3 files changed, 35 insertions, 7 deletions

diff --git a/chrome/browser/browser_commands_unittest.cc b/chrome/browser/browser_commands_unittest.cc
index 2c39bbb..3679378 100644
--- a/chrome/browser/browser_commands_unittest.cc
+++ b/chrome/browser/browser_commands_unittest.cc
@@ -21,6 +21,7 @@
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/test/test_renderer_host.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 typedef BrowserWithTestWindowTest BrowserCommandsTest;
@@ -107,10 +108,22 @@ TEST_F(BrowserCommandsTest, DuplicateTab) {
 // Tests IDC_VIEW_SOURCE (See http://crbug.com/138140).
 TEST_F(BrowserCommandsTest, ViewSource) {
   GURL url1("http://foo/1");
+  GURL url1_subframe("http://foo/subframe");
   GURL url2("http://foo/2");
 
-  // Navigate to a URL, plus a pending URL that hasn't committed.
+  // Navigate to a URL and simulate a subframe committing.
   AddTab(browser(), url1);
+  content::RenderFrameHostTester* rfh_tester =
+      content::RenderFrameHostTester::For(
+          browser()->tab_strip_model()->GetWebContentsAt(0)->GetMainFrame());
+  content::RenderFrameHost* subframe = rfh_tester->AppendChild("subframe");
+  content::RenderFrameHostTester* subframe_tester =
+      content::RenderFrameHostTester::For(subframe);
+  subframe_tester->SimulateNavigationStart(GURL(url1_subframe));
+  subframe_tester->SimulateNavigationCommit(GURL(url1_subframe));
+  subframe_tester->SimulateNavigationStop();
+
+  // Now start a pending navigation that hasn't committed.
   content::NavigationController& orig_controller =
       browser()->tab_strip_model()->GetWebContentsAt(0)->GetController();
   orig_controller.LoadURL(
diff --git a/content/browser/frame_host/navigation_entry_impl.cc b/content/browser/frame_host/navigation_entry_impl.cc
index 258429a..6b8e04a7 100644
--- a/content/browser/frame_host/navigation_entry_impl.cc
+++ b/content/browser/frame_host/navigation_entry_impl.cc
@@ -264,9 +264,17 @@ void NavigationEntryImpl::SetPageState(const PageState& state) {
     return;
   }
 
-  // This should only be called when restoring a NavigationEntry, so there
-  // should be no subframe FrameNavigationEntries yet.
-  DCHECK_EQ(0U, frame_tree_->children.size());
+  // SetPageState should only be called before the NavigationEntry has been
+  // loaded, such as for restore (when there are no subframe
+  // FrameNavigationEntries yet).  However, some callers expect to call this
+  // after a Clone but before loading the page.  Clone will copy over the
+  // subframe entries, and we should reset them before setting the state again.
+  //
+  // TODO(creis): It would be good to verify that this NavigationEntry hasn't
+  // been loaded yet in cases that SetPageState is called while subframe
+  // entries exist, but there's currently no way to check that.
+  if (!frame_tree_->children.empty())
+    frame_tree_->children.clear();
 
   // If the PageState can't be parsed or has no children, just store it on the
   // main frame's FrameNavigationEntry without recursively creating subframe
diff --git a/content/test/test_render_frame_host.cc b/content/test/test_render_frame_host.cc
index 5ae3fd8..932dbf7 100644
--- a/content/test/test_render_frame_host.cc
+++ b/content/test/test_render_frame_host.cc
@@ -120,14 +120,21 @@ void TestRenderFrameHost::SimulateNavigationCommit(const GURL& url) {
   if (frame_tree_node()->navigation_request())
     PrepareForCommit();
 
+  bool is_auto_subframe =
+      GetParent() && !frame_tree_node()->has_committed_real_load();
+
   FrameHostMsg_DidCommitProvisionalLoad_Params params;
   params.page_id = ComputeNextPageID();
   params.nav_entry_id = 0;
   params.url = url;
-  params.transition = GetParent() ? ui::PAGE_TRANSITION_MANUAL_SUBFRAME
-                                  : ui::PAGE_TRANSITION_LINK;
+  if (!GetParent())
+    params.transition = ui::PAGE_TRANSITION_LINK;
+  else if (is_auto_subframe)
+    params.transition = ui::PAGE_TRANSITION_AUTO_SUBFRAME;
+  else
+    params.transition = ui::PAGE_TRANSITION_MANUAL_SUBFRAME;
   params.should_update_history = true;
-  params.did_create_new_entry = true;
+  params.did_create_new_entry = !is_auto_subframe;
   params.gesture = NavigationGestureUser;
   params.contents_mime_type = contents_mime_type_;
   params.is_post = false;