Don't use pending NavigationEntries for navigation transfers (try #2).

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 a5ff8a1a22fd52d59efe6811e24f16b7ea3ba9f6..267e6b9ebba8d1142f37747cff4134338746f020 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -746,18 +746,69 @@ 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.  See
+  // https://crbug.com/536906.
+  std::unique_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.
+      // TODO(creis): Ensure this case can't exist in https://crbug.com/524208.
+      entry = NavigationEntryImpl::FromNavigationEntry(
+          controller_->CreateNavigationEntry(
+              GURL(url::kAboutBlankURL), referrer_to_use, page_transition,
+              is_renderer_initiated, std::string(),
+              controller_->GetBrowserContext()));
+    }
+    entry->AddOrUpdateFrameEntry(node, -1, -1, nullptr, dest_url,
+                                 referrer_to_use, PageState(), "GET", -1);

[alexmos, 2016/05/03 22:07:52]

Is it ok that this is always "GET" (and also in the fix below)?  Maybe this
needs a TODO to figure out how the POST path is going to work?

[Charlie Reis, 2016/05/03 23:43:05]

Yeah, this is a TODO.

+  } else {
+    // Main frame case.
+    entry = NavigationEntryImpl::FromNavigationEntry(
+        controller_->CreateNavigationEntry(
+            dest_url, referrer_to_use, 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);
+  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);
+  if (controller_->GetLastCommittedEntry() &&
+      controller_->GetLastCommittedEntry()->GetIsOverridingUserAgent()) {
+    entry->SetIsOverridingUserAgent(true);
+  }
+  entry->set_transferred_global_request_id(transferred_global_request_id);
+  // TODO(creis): Set user gesture and intent received timestamp on Android.
+  FrameNavigationEntry* frame_entry = entry->GetFrameEntry(node);
+
+  // We may not have successfully added the FrameNavigationEntry to |entry|
+  // above (per https://crbug.com/608402), in which case we create it from
+  // scratch.  This works because we do not depend on |frame_entry| being inside
+  // |entry| during NavigateToEntry.  This will go away when we shortcut this
+  // further in https://crbug.com/536906.
+  if (!frame_entry) {

[Charlie Reis, 2016/05/02 22:35:34]

This is the fix.  It should be safe for the time being (unless anyone starts
depending on the frame entry being inside the NavEntry).  I think we can get by
with it until either 608402 or 536906 are fixed.

+    frame_entry =

[alexmos, 2016/05/03 22:07:51]

How will this get cleaned up?  

[Charlie Reis, 2016/05/03 23:43:05]

Good point-- it would leak in this branch, since |entry| doesn't claim ownership
of it.  I've changed it to be a scoped_refptr above, since that's how NavEntry
tracks it internally (and it's how we avoid a leak in
RendererDidNavigateNewSubframe as well).

+        new FrameNavigationEntry(node->unique_name(), -1, -1, nullptr, dest_url,
+                                 referrer_to_use, "GET", -1);
+  }
+  NavigateToEntry(node, *frame_entry, *entry.get(),
+                  NavigationController::NO_RELOAD, false, false);
 }
 
 // PlzNavigate