Prevent renderer kills for in-page navigations on subframes.

content/browser/frame_host/navigation_controller_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 /*
  * Copyright (C) 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved.
  *     (http://www.torchmobile.com/)
  *
@@ skipping 1358 matching lines @@
 // However, due to reloads, even identical urls are *not* guaranteed to be
 // in-page navigations, we have to trust the renderer almost entirely.
 // The one thing we do know is that cross-origin navigations will *never* be
 // in-page. Therefore, trust the renderer if the URLs are on the same origin,
 // and assume the renderer is malicious if a cross-origin navigation claims to
 // be in-page.
 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
     // might Blink say that a navigation is in-page yet there be no last-
     // committed entry?
     if (!last_committed)
       return false;
     last_committed_url = last_committed->GetURL();
   }
 
   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
                         // about:blank is the first document that frame loaded.
                         // We don't have sufficient information to identify
                         // that case at the moment, so always allow about:blank
                         // for now.
                         last_committed_url == GURL(url::kAboutBlankURL) ||
                         last_committed_url.GetOrigin() == url.GetOrigin() ||
                         !prefs.web_security_enabled ||
@@ skipping 441 matching lines @@
   return success;
 }
 
 void NavigationControllerImpl::FindFramesToNavigate(
     FrameTreeNode* frame,
     FrameLoadVector* same_document_loads,
     FrameLoadVector* different_document_loads) {
   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)
     return;
 
   // Schedule a load in this frame if the new item isn't for the same item
   // sequence number in the same SiteInstance. Newly restored items may not have
   // a SiteInstance yet, in which case it will be assigned on first commit.
   if (!old_item ||
       new_item->item_sequence_number() != old_item->item_sequence_number() ||
@@ skipping 193 matching lines @@
     }
   }
 }
 
 void NavigationControllerImpl::SetGetTimestampCallbackForTest(
     const base::Callback<base::Time()>& get_timestamp_callback) {
   get_timestamp_callback_ = get_timestamp_callback;
 }
 
 }  // namespace content