Don't use pending NavigationEntries for navigation transfers.

content/browser/frame_host/navigator_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.
 
 #include "content/browser/frame_host/navigator_impl.h"
 
 #include <utility>
 
 #include "base/metrics/histogram.h"
 #include "base/time/time.h"
@@ skipping 708 matching lines @@
     // Note also that we hide the referrer for Web UI pages. We don't really
     // want web sites to see a referrer of "chrome://blah" (and some
     // chrome: URLs might have search terms or other stuff we don't want to
     // send to the site), so we send no referrer.
     referrer_to_use = Referrer();
 
     // Navigations in Web UI pages count as browser-initiated navigations.
     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
 void NavigatorImpl::OnBeforeUnloadACK(FrameTreeNode* frame_tree_node,
                                       bool proceed) {
   CHECK(IsBrowserSideNavigationEnabled());
   DCHECK(frame_tree_node);
 
   NavigationRequest* navigation_request = frame_tree_node->navigation_request();
 
@@ skipping 380 matching lines @@
   if (pending_entry != controller_->GetVisibleEntry() ||
       !should_preserve_entry) {
     controller_->DiscardPendingEntry(true);
 
     // Also force the UI to refresh.
     controller_->delegate()->NotifyNavigationStateChanged(INVALIDATE_TYPE_URL);
   }
 }
 
 }  // namespace content