Fill in FrameNavigationEntries for auto subframe navigations.

BUG=236848
TEST=NavigationController* tests pass in --site-per-process mode.

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

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

7 files changed, 222 insertions, 33 deletions

diff --git a/content/browser/frame_host/frame_navigation_entry.cc b/content/browser/frame_host/frame_navigation_entry.cc
index a33f973..02af2af 100644
--- a/content/browser/frame_host/frame_navigation_entry.cc
+++ b/content/browser/frame_host/frame_navigation_entry.cc
@@ -6,26 +6,35 @@
 
 namespace content {
 
-FrameNavigationEntry::FrameNavigationEntry() {
+FrameNavigationEntry::FrameNavigationEntry(int64 frame_tree_node_id)
+    : frame_tree_node_id_(frame_tree_node_id) {
 }
 
-FrameNavigationEntry::FrameNavigationEntry(SiteInstanceImpl* site_instance,
+FrameNavigationEntry::FrameNavigationEntry(int64 frame_tree_node_id,
+                                           SiteInstanceImpl* site_instance,
                                            const GURL& url,
                                            const Referrer& referrer)
-    : site_instance_(site_instance), url_(url), referrer_(referrer) {
+    : frame_tree_node_id_(frame_tree_node_id),
+      site_instance_(site_instance),
+      url_(url),
+      referrer_(referrer) {
 }
 
 FrameNavigationEntry::~FrameNavigationEntry() {
 }
 
 FrameNavigationEntry* FrameNavigationEntry::Clone() const {
-  FrameNavigationEntry* copy = new FrameNavigationEntry();
-
-  copy->site_instance_ = site_instance_;
-  copy->url_ = url_;
-  copy->referrer_ = referrer_;
-
+  FrameNavigationEntry* copy = new FrameNavigationEntry(frame_tree_node_id_);
+  copy->UpdateEntry(site_instance_.get(), url_, referrer_);
   return copy;
 }
 
+void FrameNavigationEntry::UpdateEntry(SiteInstanceImpl* site_instance,
+                                       const GURL& url,
+                                       const Referrer& referrer) {
+  site_instance_ = site_instance;
+  url_ = url;
+  referrer_ = referrer;
+}
+
 }  // namespace content
diff --git a/content/browser/frame_host/frame_navigation_entry.h b/content/browser/frame_host/frame_navigation_entry.h
index 2d38a25..15d224c 100644
--- a/content/browser/frame_host/frame_navigation_entry.h
+++ b/content/browser/frame_host/frame_navigation_entry.h
@@ -24,8 +24,13 @@ namespace content {
 class CONTENT_EXPORT FrameNavigationEntry
     : public base::RefCounted<FrameNavigationEntry> {
  public:
-  FrameNavigationEntry();
-  FrameNavigationEntry(SiteInstanceImpl* site_instance,
+  // TODO(creis): We should not use FTN IDs here, since they will change if you
+  // leave a page and come back later.  We should evaluate whether Blink's
+  // frame sequence numbers or unique names would work instead, similar to
+  // HistoryNode.
+  explicit FrameNavigationEntry(int64 frame_tree_node_id);
+  FrameNavigationEntry(int64 frame_tree_node_id,
+                       SiteInstanceImpl* site_instance,
                        const GURL& url,
                        const Referrer& referrer);
 
@@ -33,6 +38,17 @@ class CONTENT_EXPORT FrameNavigationEntry
   // independently from the original.
   FrameNavigationEntry* Clone() const;
 
+  // Updates all the members of this entry.
+  void UpdateEntry(SiteInstanceImpl* site_instance,
+                   const GURL& url,
+                   const Referrer& referrer);
+
+  // The ID of the FrameTreeNode this entry is for.  -1 for the main frame,
+  // since we don't always know the FrameTreeNode ID when creating the overall
+  // NavigationEntry.
+  // TODO(creis): Replace with frame sequence number or unique name.
+  int64 frame_tree_node_id() const { return frame_tree_node_id_; }
+
   // The SiteInstance responsible for rendering this frame.  All frames sharing
   // a SiteInstance must live in the same process.  This is a refcounted pointer
   // that keeps the SiteInstance (not necessarily the process) alive as long as
@@ -62,6 +78,7 @@ class CONTENT_EXPORT FrameNavigationEntry
   // WARNING WARNING WARNING WARNING WARNING WARNING WARNING WARNING WARNING
 
   // See the accessors above for descriptions.
+  int64 frame_tree_node_id_;
   scoped_refptr<SiteInstanceImpl> site_instance_;
   GURL url_;
   Referrer referrer_;
diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc
index 95f5f13..110cebf 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -1473,11 +1473,9 @@ bool NavigationControllerImpl::RendererDidNavigateAutoSubframe(
           switches::kSitePerProcess)) {
     // This may be a "new auto" case where we add a new FrameNavigationEntry, or
     // it may be a "history auto" case where we update an existing one.
-    int frame_tree_node_id = rfh->frame_tree_node()->frame_tree_node_id();
     NavigationEntryImpl* last_committed = GetLastCommittedEntry();
-    last_committed->AddOrUpdateFrameEntry(frame_tree_node_id,
-                                          rfh->GetSiteInstance(),
-                                          params.url,
+    last_committed->AddOrUpdateFrameEntry(rfh->frame_tree_node(),
+                                          rfh->GetSiteInstance(), params.url,
                                           params.referrer);
   }
 
diff --git a/content/browser/frame_host/navigation_controller_impl_browsertest.cc b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
index 3ad1d3d..6cc541e 100644
--- a/content/browser/frame_host/navigation_controller_impl_browsertest.cc
+++ b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
@@ -950,7 +950,7 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
 
 // Verify the tree of FrameNavigationEntries after NAVIGATION_TYPE_AUTO_SUBFRAME
 // commits.
-// TODO(creis): Test cross-site and nested iframes.
+// TODO(creis): Test cross-site iframes.
 // TODO(creis): Test updating entries for history auto subframe navigations.
 IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
                        FrameNavigationEntry_AutoSubframe) {
@@ -996,6 +996,70 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
     // There are no subframe FrameNavigationEntries by default.
     EXPECT_EQ(0U, entry->root_node()->children.size());
   }
+
+  // Create a second iframe.
+  {
+    LoadCommittedCapturer capturer(shell()->web_contents());
+    std::string script = "var iframe = document.createElement('iframe');"
+                         "iframe.src = '" + frame_url.spec() + "';"
+                         "document.body.appendChild(iframe);";
+    EXPECT_TRUE(content::ExecuteScript(root->current_frame_host(), script));
+    capturer.Wait();
+    EXPECT_EQ(ui::PAGE_TRANSITION_AUTO_SUBFRAME, capturer.transition_type());
+  }
+
+  // The last committed NavigationEntry shouldn't have changed.
+  EXPECT_EQ(1, controller.GetEntryCount());
+  entry = controller.GetLastCommittedEntry();
+  EXPECT_EQ(main_url, entry->GetURL());
+  root_entry = entry->root_node()->frame_entry.get();
+  EXPECT_EQ(main_url, root_entry->url());
+
+  // Verify subframe entries if we're in --site-per-process mode.
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kSitePerProcess)) {
+    // The entry should now have 2 subframe FrameNavigationEntries.
+    ASSERT_EQ(2U, entry->root_node()->children.size());
+    FrameNavigationEntry* frame_entry =
+        entry->root_node()->children[1]->frame_entry.get();
+    EXPECT_EQ(frame_url, frame_entry->url());
+  } else {
+    // There are no subframe FrameNavigationEntries by default.
+    EXPECT_EQ(0U, entry->root_node()->children.size());
+  }
+
+  // Create a nested iframe in the second subframe.
+  {
+    LoadCommittedCapturer capturer(shell()->web_contents());
+    std::string script = "var iframe = document.createElement('iframe');"
+                         "iframe.src = '" + frame_url.spec() + "';"
+                         "document.body.appendChild(iframe);";
+    EXPECT_TRUE(content::ExecuteScript(root->child_at(1)->current_frame_host(),
+                                       script));
+    capturer.Wait();
+    EXPECT_EQ(ui::PAGE_TRANSITION_AUTO_SUBFRAME, capturer.transition_type());
+  }
+
+  // The last committed NavigationEntry shouldn't have changed.
+  EXPECT_EQ(1, controller.GetEntryCount());
+  entry = controller.GetLastCommittedEntry();
+  EXPECT_EQ(main_url, entry->GetURL());
+  root_entry = entry->root_node()->frame_entry.get();
+  EXPECT_EQ(main_url, root_entry->url());
+
+  // Verify subframe entries if we're in --site-per-process mode.
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kSitePerProcess)) {
+    // The entry should now have 2 subframe FrameNavigationEntries.
+    ASSERT_EQ(2U, entry->root_node()->children.size());
+    ASSERT_EQ(1U, entry->root_node()->children[1]->children.size());
+    FrameNavigationEntry* frame_entry =
+        entry->root_node()->children[1]->children[0]->frame_entry.get();
+    EXPECT_EQ(frame_url, frame_entry->url());
+  } else {
+    // There are no subframe FrameNavigationEntries by default.
+    EXPECT_EQ(0U, entry->root_node()->children.size());
+  }
 }
 
 namespace {
diff --git a/content/browser/frame_host/navigation_controller_impl_unittest.cc b/content/browser/frame_host/navigation_controller_impl_unittest.cc
index fc12fb5..cd10a01 100644
--- a/content/browser/frame_host/navigation_controller_impl_unittest.cc
+++ b/content/browser/frame_host/navigation_controller_impl_unittest.cc
@@ -2104,7 +2104,6 @@ TEST_F(NavigationControllerTest, NewSubframe) {
 
 // Auto subframes are ones the page loads automatically like ads. They should
 // not create new navigation entries.
-// TODO(creis): Test cross-site and nested iframes.
 // TODO(creis): Test updating entries for history auto subframe navigations.
 TEST_F(NavigationControllerTest, AutoSubframe) {
   NavigationControllerImpl& controller = controller_impl();
@@ -2170,6 +2169,8 @@ TEST_F(NavigationControllerTest, AutoSubframe) {
   {
     FrameHostMsg_DidCommitProvisionalLoad_Params params;
     params.page_id = 1;
+    params.nav_entry_id = 0;
+    params.did_create_new_entry = false;
     params.url = url3;
     params.transition = ui::PAGE_TRANSITION_AUTO_SUBFRAME;
     params.should_update_history = false;
@@ -2203,6 +2204,56 @@ TEST_F(NavigationControllerTest, AutoSubframe) {
     // There are no subframe FrameNavigationEntries by default.
     EXPECT_EQ(0U, entry->root_node()->children.size());
   }
+
+  // Add a nested subframe and navigate.
+  subframe->OnCreateChildFrame(MSG_ROUTING_NONE, std::string(),
+                               SandboxFlags::NONE);
+  RenderFrameHostImpl* subframe3 = contents()
+                                       ->GetFrameTree()
+                                       ->root()
+                                       ->child_at(0)
+                                       ->child_at(0)
+                                       ->current_frame_host();
+  const GURL url4("http://foo/4");
+  {
+    FrameHostMsg_DidCommitProvisionalLoad_Params params;
+    params.page_id = 1;
+    params.nav_entry_id = 0;
+    params.did_create_new_entry = false;
+    params.url = url4;
+    params.transition = ui::PAGE_TRANSITION_AUTO_SUBFRAME;
+    params.should_update_history = false;
+    params.gesture = NavigationGestureUser;
+    params.is_post = false;
+    params.page_state = PageState::CreateFromURL(url4);
+
+    // Navigating should do nothing.
+    LoadCommittedDetails details;
+    EXPECT_FALSE(controller.RendererDidNavigate(subframe3, params, &details));
+    EXPECT_EQ(0U, notifications.size());
+  }
+
+  // There should still be only one entry, mostly unchanged.
+  EXPECT_EQ(1, controller.GetEntryCount());
+  EXPECT_EQ(entry, controller.GetLastCommittedEntry());
+  EXPECT_EQ(url1, entry->GetURL());
+  EXPECT_EQ(1, entry->GetPageID());
+  EXPECT_EQ(root_entry, entry->root_node()->frame_entry.get());
+  EXPECT_EQ(url1, root_entry->url());
+
+  // Verify subframe entries if we're in --site-per-process mode.
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kSitePerProcess)) {
+    // The entry should now have a nested FrameNavigationEntry.
+    EXPECT_EQ(2U, entry->root_node()->children.size());
+    ASSERT_EQ(1U, entry->root_node()->children[0]->children.size());
+    FrameNavigationEntry* new_frame_entry =
+        entry->root_node()->children[0]->children[0]->frame_entry.get();
+    EXPECT_EQ(url4, new_frame_entry->url());
+  } else {
+    // There are no subframe FrameNavigationEntries by default.
+    EXPECT_EQ(0U, entry->root_node()->children.size());
+  }
 }
 
 // Tests navigation and then going back to a subframe navigation.
@@ -3564,21 +3615,24 @@ TEST_F(NavigationControllerTest, SameSubframe) {
   EXPECT_EQ(controller.GetEntryCount(), 1);
   EXPECT_EQ(controller.GetLastCommittedEntryIndex(), 0);
 
-  // Navigate a subframe that would normally count as in-page.
-  const GURL subframe("http://www.google.com/#");
+  // Add and navigate a subframe that would normally count as in-page.
+  main_test_rfh()->OnCreateChildFrame(MSG_ROUTING_NONE, std::string(),
+                                      SandboxFlags::NONE);
+  RenderFrameHostImpl* subframe =
+      contents()->GetFrameTree()->root()->child_at(0)->current_frame_host();
+  const GURL subframe_url("http://www.google.com/#");
   FrameHostMsg_DidCommitProvisionalLoad_Params params;
   params.page_id = 0;
   params.nav_entry_id = 0;
   params.did_create_new_entry = false;
-  params.url = subframe;
+  params.url = subframe_url;
   params.transition = ui::PAGE_TRANSITION_AUTO_SUBFRAME;
   params.should_update_history = false;
   params.gesture = NavigationGestureAuto;
   params.is_post = false;
-  params.page_state = PageState::CreateFromURL(subframe);
+  params.page_state = PageState::CreateFromURL(subframe_url);
   LoadCommittedDetails details;
-  EXPECT_FALSE(controller.RendererDidNavigate(main_test_rfh(), params,
-                                              &details));
+  EXPECT_FALSE(controller.RendererDidNavigate(subframe, params, &details));
 
   // Nothing should have changed.
   EXPECT_EQ(controller.GetEntryCount(), 1);
@@ -3712,6 +3766,10 @@ TEST_F(NavigationControllerTest, SubframeWhilePending) {
 
   // Send a subframe update from the first page, as if one had just
   // automatically loaded. Auto subframes don't increment the page ID.
+  main_test_rfh()->OnCreateChildFrame(MSG_ROUTING_NONE, std::string(),
+                                      SandboxFlags::NONE);
+  RenderFrameHostImpl* subframe =
+      contents()->GetFrameTree()->root()->child_at(0)->current_frame_host();
   const GURL url1_sub("http://foo/subframe");
   FrameHostMsg_DidCommitProvisionalLoad_Params params;
   params.page_id = controller.GetLastCommittedEntry()->GetPageID();
@@ -3726,8 +3784,7 @@ TEST_F(NavigationControllerTest, SubframeWhilePending) {
   LoadCommittedDetails details;
 
   // This should return false meaning that nothing was actually updated.
-  EXPECT_FALSE(controller.RendererDidNavigate(main_test_rfh(), params,
-                                              &details));
+  EXPECT_FALSE(controller.RendererDidNavigate(subframe, params, &details));
 
   // The notification should have updated the last committed one, and not
   // the pending load.
diff --git a/content/browser/frame_host/navigation_entry_impl.cc b/content/browser/frame_host/navigation_entry_impl.cc
index 5a93b0f..c2b7675 100644
--- a/content/browser/frame_host/navigation_entry_impl.cc
+++ b/content/browser/frame_host/navigation_entry_impl.cc
@@ -4,6 +4,8 @@
 
 #include "content/browser/frame_host/navigation_entry_impl.h"
 
+#include <queue>
+
 #include "base/metrics/histogram.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ -62,7 +64,7 @@ NavigationEntryImpl::NavigationEntryImpl(SiteInstanceImpl* instance,
                                          ui::PageTransition transition_type,
                                          bool is_renderer_initiated)
     : frame_tree_(
-          new TreeNode(new FrameNavigationEntry(instance, url, referrer))),
+          new TreeNode(new FrameNavigationEntry(-1, instance, url, referrer))),
       unique_id_(GetUniqueIDInConstructor()),
       bindings_(kInvalidBindings),
       page_type_(PAGE_TYPE_NORMAL),
@@ -463,15 +465,56 @@ void NavigationEntryImpl::ResetForCommit() {
 #endif
 }
 
-void NavigationEntryImpl::AddOrUpdateFrameEntry(int frame_tree_node_id,
+void NavigationEntryImpl::AddOrUpdateFrameEntry(FrameTreeNode* frame_tree_node,
                                                 SiteInstanceImpl* site_instance,
                                                 const GURL& url,
                                                 const Referrer& referrer) {
-  // TODO(creis): Walk tree to find the node to update.
-  // TODO(creis): Only create a new entry if one doesn't exist yet.
-  FrameNavigationEntry* frame_entry =
-      new FrameNavigationEntry(site_instance, url, referrer);
-  root_node()->children.push_back(
+  // We should already have a TreeNode for the parent node by the time this node
+  // commits.  Find it first.
+  DCHECK(frame_tree_node->parent());
+  int parent_ftn_id = frame_tree_node->parent()->frame_tree_node_id();
+  bool found = false;
+  NavigationEntryImpl::TreeNode* parent_node = nullptr;
+  std::queue<NavigationEntryImpl::TreeNode*> work_queue;
+  work_queue.push(root_node());
+  while (!found && !work_queue.empty()) {
+    parent_node = work_queue.front();
+    work_queue.pop();
+    // The root FNE will have an ID of -1, so check for that as well.
+    if (parent_node->frame_entry->frame_tree_node_id() == parent_ftn_id ||
+        (parent_node->frame_entry->frame_tree_node_id() == -1 &&
+         parent_node == root_node() &&
+         frame_tree_node->parent()->IsMainFrame())) {
+      found = true;
+      break;
+    }
+    // Enqueue any children and keep looking.
+    for (auto& child : parent_node->children)
+      work_queue.push(child);
+  }
+  if (!found) {
+    // The renderer should not send a commit for a subframe before its parent.
+    // TODO(creis): This can currently happen because we don't yet clone the
+    // FrameNavigationEntry tree on manual subframe navigations.  Once that's
+    // added, we should kill the renderer if we get here.
+    return;
+  }
+
+  // Now check whether we have a TreeNode for the node itself.
+  int frame_tree_node_id = frame_tree_node->frame_tree_node_id();
+  for (TreeNode* child : parent_node->children) {
+    if (child->frame_entry->frame_tree_node_id() == frame_tree_node_id) {
+      // Update the existing FrameNavigationEntry.
+      child->frame_entry->UpdateEntry(site_instance, url, referrer);
+      return;
+    }
+  }
+
+  // No entry exists yet, so create a new one.  Unordered list, since we expect
+  // to look up entries by frame sequence number or unique name.
+  FrameNavigationEntry* frame_entry = new FrameNavigationEntry(
+      frame_tree_node_id, site_instance, url, referrer);
+  parent_node->children.push_back(
       new NavigationEntryImpl::TreeNode(frame_entry));
 }
 
diff --git a/content/browser/frame_host/navigation_entry_impl.h b/content/browser/frame_host/navigation_entry_impl.h
index 7e99d13..8c3fbb5 100644
--- a/content/browser/frame_host/navigation_entry_impl.h
+++ b/content/browser/frame_host/navigation_entry_impl.h
@@ -10,6 +10,7 @@
 #include "base/memory/scoped_vector.h"
 #include "base/time/time.h"
 #include "content/browser/frame_host/frame_navigation_entry.h"
+#include "content/browser/frame_host/frame_tree_node.h"
 #include "content/browser/site_instance_impl.h"
 #include "content/common/frame_message_enums.h"
 #include "content/public/browser/favicon_status.h"
@@ -153,7 +154,7 @@ class CONTENT_EXPORT NavigationEntryImpl
   // its FrameNavigationEntry.  A new FrameNavigationEntry is added if none
   // exists, or else the existing one (which might be shared with other
   // NavigationEntries) is updated with the given parameters.
-  void AddOrUpdateFrameEntry(int frame_tree_node_id,
+  void AddOrUpdateFrameEntry(FrameTreeNode* frame_tree_node,
                              SiteInstanceImpl* site_instance,
                              const GURL& url,
                              const Referrer& referrer);