Don't use pending NavigationEntries for navigation transfers.

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 79265e443d7fefbb85301a8ce7ca4ad25c37eb0d..5e6f2ac705403f6e0fe5476def0ed1b0120c7334 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -726,18 +726,53 @@ void NavigatorImpl::RequestTransferURL(
     is_renderer_initiated = false;
   }
 
-  NavigationController::LoadURLParams load_url_params(dest_url);
+  // Create a NavigationEntry for the transfer, without making it the pending
+  // entry.  Subframe transfers should only be possible in OOPIF-enabled modes,
+  // and should have a clone of the last committed entry with a
+  // FrameNavigationEntry for the target frame.  Main frame transfers should
+  // have a new NavigationEntry.
+  // TODO(creis): Make this unnecessary by creating (and validating) the params
+  // directly, passing them to the destination RenderFrameHost.
+  scoped_ptr<NavigationEntryImpl> entry;
+  if (!node->IsMainFrame()) {
+    // Subframe case: create FrameNavigationEntry.
+    CHECK(SiteIsolationPolicy::UseSubframeNavigationEntries());
+    if (controller_->GetLastCommittedEntry()) {
+      entry = controller_->GetLastCommittedEntry()->Clone();
+      entry->SetPageID(-1);
+    } else {
+      // If there's no last committed entry, create an entry for about:blank
+      // with a subframe entry for our destination.

[alexmos, 2016/03/21 21:41:26]

Just curious, the old path through LoadURLWithParams doesn't seem to handle this
case.  Is this something you're fixing along the way?

[Charlie Reis, 2016/03/30 15:26:07]

Yes, now we can create OOPIFs in the initial blank page.  And good point-- I
added a test for this part.

+      // TODO(creis): Ensure this case can't exist in https://crbug.com/524208.
+      entry = NavigationEntryImpl::FromNavigationEntry(
+          controller_->CreateNavigationEntry(
+              GURL(url::kAboutBlankURL), referrer, page_transition,

[alexmos, 2016/03/21 21:41:26]

Should this pass referrer_to_use?  Same for two other uses below.

[Charlie Reis, 2016/03/30 15:26:07]

Good catch!

+              is_renderer_initiated, std::string(),
+              controller_->GetBrowserContext()));
+    }
+    entry->AddOrUpdateFrameEntry(node, std::string(), -1, -1, nullptr, dest_url,
+                                 referrer, PageState());
+  } else {
+    // Main frame case.
+    entry = NavigationEntryImpl::FromNavigationEntry(
+        controller_->CreateNavigationEntry(dest_url, referrer, page_transition,
+                                           is_renderer_initiated, std::string(),
+                                           controller_->GetBrowserContext()));
+  }
+
   // The source_site_instance may matter for navigations via RenderFrameProxy.
-  load_url_params.source_site_instance = source_site_instance;
-  load_url_params.transition_type = page_transition;
-  load_url_params.frame_tree_node_id = node->frame_tree_node_id();
-  load_url_params.referrer = referrer_to_use;
-  load_url_params.redirect_chain = redirect_chain;
-  load_url_params.is_renderer_initiated = is_renderer_initiated;
-  load_url_params.transferred_global_request_id = transferred_global_request_id;
-  load_url_params.should_replace_current_entry = should_replace_current_entry;
-
-  controller_->LoadURLWithParams(load_url_params);

[alexmos, 2016/03/21 21:41:26]

The old path used to return early if the target URL returned true from
HandleDebugURL and kEnableGpuBenchmarking wasn't set -- just want to make sure
that it's ok not to do this anymore?  (Perhaps we can't ever get a transfer for
a debug URL?)

[Charlie Reis, 2016/03/30 15:26:08]

Right, we can't get a transfer for a debug URL.

+  entry->set_source_site_instance(
+      static_cast<SiteInstanceImpl*>(source_site_instance));
+  entry->SetRedirectChain(redirect_chain);
+  // Don't allow an entry replacement if there is no entry to replace.
+  // http://crbug.com/457149
+  if (should_replace_current_entry && controller_->GetEntryCount() > 0)
+    entry->set_should_replace_entry(true);
+  entry->set_transferred_global_request_id(transferred_global_request_id);

[alexmos, 2016/03/21 21:41:26]

Does entry->SetIsOverridingUserAgent() also need to be set?  The
LoadURLWithParams params had the default override_user_agent value of
UA_OVERRIDE_INHERIT, which means that this used to be set to the override value
from GetLastCommittedEntry().  I'm not sure where this is used though.

[Charlie Reis, 2016/03/30 15:26:07]

Ah, nice find.  I must have assumed there was no easy way to set that value or
that it didn't apply here, but you're right.  Fixed.

+  // TODO(creis): Set user gesture and intent received timestamp on Android.
+  FrameNavigationEntry* frame_entry = entry->GetFrameEntry(node);
+  NavigateToEntry(node, *frame_entry, *entry.get(),
+                  NavigationController::NO_RELOAD, false, false);
 }
 
 // PlzNavigate