Prevent renderer kills for in-page navigations on subframes.

content/browser/frame_host/navigation_controller_impl.cc

Index: content/browser/frame_host/navigation_controller_impl.cc
diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc
index fe5cdcd0f764db3adaa7b8a7932f6907fb31d66d..57ca3613a0a79afe85d72e2bc914e81f5e392ece 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -1376,9 +1376,10 @@ bool NavigationControllerImpl::IsURLInPageNavigation(
     const GURL& url,
     bool renderer_says_in_page,
     RenderFrameHost* rfh) const {
+  RenderFrameHostImpl* rfhi = static_cast<RenderFrameHostImpl*>(rfh);
   GURL last_committed_url;
   if (rfh->GetParent()) {
-    last_committed_url = rfh->GetLastCommittedURL();

[Charlie Reis, 2016/06/03 19:20:35]

This is wrong after a process swap, when the newly created RFH might not have a
last committed URL even though the FrameTreeNode does.  I noticed it when we
unexpectedly weren't doing the NC_IN_PAGE_NAVIGATION_KILL in --site-per-process
in one of the tests.

(This is somewhat tangential to the rest of the CL, but definitely fixes broken
behavior in this method.)

[Avi (use Gerrit), 2016/06/03 20:26:57]

Interesting; worthy of a comment why we don't do it that way?

Also, would this be a problem in general? Would we want to reimplement
RFH::GetLastCommittedURL() through FrameTreeNode?

[Charlie Reis, 2016/06/03 20:53:28]

Good idea.  Done.
Quite the opposite-- that's how it used to work (before
https://crbug.com/590034), which led to problems when you called
RFH::GetLastCommittedURL() and got a different frame's URL.  That matters for
things like security checks relevant to the RFH itself.

In this case, we care more about what the last page in the FrameTreeNode was,
regardless of which RFH it committed in.

+    last_committed_url = rfhi->frame_tree_node()->current_url();
   } else {
     NavigationEntry* last_committed = GetLastCommittedEntry();
     // There must be a last-committed entry to compare URLs to. TODO(avi): When
@@ -1390,9 +1391,8 @@ bool NavigationControllerImpl::IsURLInPageNavigation(
   }
 
   WebPreferences prefs = rfh->GetRenderViewHost()->GetWebkitPreferences();
-  const url::Origin& committed_origin = static_cast<RenderFrameHostImpl*>(rfh)
-                                            ->frame_tree_node()
-                                            ->current_origin();
+  const url::Origin& committed_origin =
+      rfhi->frame_tree_node()->current_origin();
   bool is_same_origin = last_committed_url.is_empty() ||
                         // TODO(japhet): We should only permit navigations
                         // originating from about:blank to be in-page if the
@@ -1854,6 +1854,8 @@ void NavigationControllerImpl::FindFramesToNavigate(
   DCHECK(pending_entry_);
   DCHECK_GE(last_committed_entry_index_, 0);
   FrameNavigationEntry* new_item = pending_entry_->GetFrameEntry(frame);
+  // TODO(creis): Store the last committed FrameNavigationEntry to use here,
+  // rather than assuming the NavigationEntry has up to date info on subframes.

[Charlie Reis, 2016/06/03 19:20:35]

I decided to punt this for another CL, since it will take a bit more work to fix
and we won't hit this on M52 beta.

   FrameNavigationEntry* old_item =
       GetLastCommittedEntry()->GetFrameEntry(frame);
   if (!new_item)