OOPIF: History navigations for new child frames.

content/browser/frame_host/navigator_impl.cc

Index: content/browser/frame_host/navigator_impl.cc
diff --git a/content/browser/frame_host/navigator_impl.cc b/content/browser/frame_host/navigator_impl.cc
index 44bc5a8b9974266b3aa6a3b9dee813cdb8a177f1..f48795e147b5d0293dd3b5b041c141b962a1717f 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -255,7 +255,8 @@ bool NavigatorImpl::NavigateToEntry(
     const FrameNavigationEntry& frame_entry,
     const NavigationEntryImpl& entry,
     NavigationController::ReloadType reload_type,
-    bool is_same_document_history_load) {
+    bool is_same_document_history_load,
+    bool is_pending_entry) {
   TRACE_EVENT0("browser,navigation", "NavigatorImpl::NavigateToEntry");
 
   GURL dest_url = frame_entry.url();
@@ -303,8 +304,8 @@ bool NavigatorImpl::NavigateToEntry(
                       entry, reload_type, is_same_document_history_load,
                       navigation_start);
 
-    // Notify observers about navigation.
-    if (delegate_)
+    // Notify observers about navigation if this is for the pending entry.
+    if (delegate_ && is_pending_entry)
       delegate_->DidStartNavigationToPendingEntry(dest_url, reload_type);
 
     return true;
@@ -316,7 +317,8 @@ bool NavigatorImpl::NavigateToEntry(
     return false;  // Unable to create the desired RenderFrameHost.
 
   // Make sure no code called via RFHM::Navigate clears the pending entry.
-  CHECK_EQ(controller_->GetPendingEntry(), &entry);
+  if (is_pending_entry)
+    CHECK_EQ(controller_->GetPendingEntry(), &entry);
 
   // For security, we should never send non-Web-UI URLs to a Web UI renderer.
   // Double check that here.
@@ -360,7 +362,8 @@ bool NavigatorImpl::NavigateToEntry(
   }
 
   // Make sure no code called via RFH::Navigate clears the pending entry.
-  CHECK_EQ(controller_->GetPendingEntry(), &entry);
+  if (is_pending_entry)
+    CHECK_EQ(controller_->GetPendingEntry(), &entry);
 
   if (controller_->GetPendingEntryIndex() == -1 &&
       dest_url.SchemeIs(url::kJavaScriptScheme)) {
@@ -376,9 +379,8 @@ bool NavigatorImpl::NavigateToEntry(
   }
 
   // Notify observers about navigation.
-  if (delegate_) {
+  if (delegate_ && is_pending_entry)
     delegate_->DidStartNavigationToPendingEntry(dest_url, reload_type);
-  }
 
   return true;
 }
@@ -390,7 +392,30 @@ bool NavigatorImpl::NavigateToPendingEntry(
     bool is_same_document_history_load) {
   return NavigateToEntry(frame_tree_node, frame_entry,
                          *controller_->GetPendingEntry(), reload_type,
-                         is_same_document_history_load);
+                         is_same_document_history_load, true);
+}
+
+void NavigatorImpl::NavigateNewChildFrame(
+    RenderFrameHostImpl* render_frame_host,
+    const std::string& unique_name) {
+  // TODO(creis): Handle the fallback case where frame_entry isn't found, or
+  // where NavigateToEntry returns false.
+  NavigationEntryImpl* entry =
+      controller_->GetEntryWithUniqueID(render_frame_host->nav_entry_id());

[Avi (use Gerrit), 2015/11/24 22:09:47]

Boy, this "last commit" nav entry id is coming in handy...

So the way I'm understanding it, we get a request to navigate a new child frame,
so we're assuming that we need to work on the last commit. Can that id ever be
stale?

[Charlie Reis, 2015/11/24 23:05:54]

Right.  The renderer sends this IPC when it knows a back/forward is in progress
(i.e., we haven't gotten to DidStopLoading yet during a history navigation in
its parent).  That means we should use the NavigationEntry that the newly
created RenderFrame thinks it's on.  (And that nav_entry_id is inherited from
its parent, as of https://codereview.chromium.org/1437643004.)
Good question.  I think maybe yes, if the RenderFrame is about to be destroyed. 
In that case, we'll hit the early return below and it should be fine.  For
example:
1) Visit page A, then page B (with iframe B1), then C, each in a different
process.
2) Go back from C to B.
3) Quickly go back from B to A, then commit a new page D (pruning B and C from
history).
4) In the meantime, B creates its child frame and we get NavigateNewChildFrame
after D has committed and the NavEntry is gone.

Then again, that might not be possible in practice-- I think we'll delete the
FTN and RFH for B's iframe when going back to A in step 3, so the
NavigateNewChildFrame IPC won't be handled anywhere.

+  if (!entry)
+    return;
+
+  FrameNavigationEntry* frame_entry =
+      entry->GetFrameEntryByUniqueName(unique_name);
+  if (!frame_entry)
+    return;
+
+  // Update the FNE's FTN ID, since the frame will have a new one.

[Avi (use Gerrit), 2015/11/24 22:09:47]

I didn't understand this comment the first time I read it. Perhaps:

// Update the FNE's FTN ID (which is currently the ID of the old FTN that no
longer exists) to be the ID of the newly-created frame.

... or something like that.

[Charlie Reis, 2015/11/24 23:05:54]

Done.

+  frame_entry->set_frame_tree_node_id(
+      render_frame_host->frame_tree_node()->frame_tree_node_id());
+
+  NavigateToEntry(render_frame_host->frame_tree_node(), *frame_entry, *entry,
+                  NavigationControllerImpl::NO_RELOAD, false, false);
 }
 
 void NavigatorImpl::DidNavigate(