NTP-initiated navigations should show-up as "visible" NavigationEntry.

r705290 moved when exactly a renderer/NTP-initiated navigation starts to
be treated as if it was a browser/bookmark-initiated navigation.  This
regressed omnibox behavior which stopped showing the URL of a navigation
initiated by an NTP click.  The main cause of the regression is that
after r705290 the NavigationEntryImpl::is_renderer_initiated() would
return true (inconsistent with NavigationRequest).

This CL makes sure that NavigationEntry associated with a
renderer/NTP-initiated navigation also indicates that this navigation
should be treated as browser-initiated.  This is achieved by adding an
extra call to ContentBrowserClient::OverrideNavigationParams *before*
creating the NavigationEntry and *before* the new Omnibox display text
is calculated by the following callstack:
    LocationBarModelImpl::GetFormattedURL()
    LocationBarModelImpl::GetURLForDisplay()
    OmniboxEditModel::ResetDisplayTexts()
    OmniboxViewViews::Update()
    LocationBarView::Update()
    ToolbarView::Update()
    Browser::UpdateToolbar()
    Browser::ScheduleUIUpdate()
    Browser::NavigationStateChanged()
    content::WebContentsImpl::NotifyNavigationStateChanged()
    content::NavigatorImpl::GetNavigationEntryForRendererInitiatedNavigation()
    content::NavigatorImpl::OnBeginNavigation()
    content::RenderFrameHostImpl::BeginNavigation()

The decision whether a navigation is NTP-initiated needs to be based on
the |source_site_instance|.  The decision cannot be based on the
|initiator_origin|, because we want https://example.com to be treated
differently from https://example.com/chrome/ntp (the latter might have a
different effective Site URL when it is designated as a "remote NTP").
This requirement necessitates adding a |source_site_instance| parameter
to NavigationController::CreateNavigationEntry().  In the NTP scenario,
the NavigationEntry is created by the following callstack:
    NavigationEntryImpl::NavigationEntryImpl()
    NavigationController::CreateNavigationEntry()
    NavigatorImpl::GetNavigationEntryForRendererInitiatedNavigation()
    NavigatorImpl::OnBeginNavigation()
    RenderFrameHostImpl::BeginNavigation()

To account for DCHECKs that ensure that NavigationRequest and
NavigationEntryImpl are compatible, the CL also had to move the older
OverrideNavigationParams call from NavigationRequest::StartNavigation to
the NavigationRequest's constructor.

This CL adds a regression test: LocalNTPTest.PendingNavigations
Additionally, I've manually tested that repro steps from
https://crbug.com/1020610#c1 have expected behavior after this CL.

Bug: 1020610
Change-Id: I7a16b28eb9426c348f37f432e7b4813bee55ddd6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1896295Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Dan Beam <dbeam@chromium.org>
Auto-Submit: Łukasz Anforowicz <lukasza@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#713592}

chrome/browser/ui/search/local_ntp_browsertest.cc

@@ -26,6 +26,8 @@
 #include "chrome/browser/search/search.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
+#include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/location_bar/location_bar.h"
 #include "chrome/browser/ui/search/instant_test_utils.h"
 #include "chrome/browser/ui/search/local_ntp_browsertest_base.h"
 #include "chrome/browser/ui/search/local_ntp_test_utils.h"
@@ -37,6 +39,7 @@
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "components/ntp_tiles/constants.h"
+#include "components/omnibox/browser/omnibox_view.h"
 #include "components/prefs/pref_service.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/interstitial_page.h"
@@ -1190,6 +1193,53 @@ IN_PROC_BROWSER_TEST_F(LocalNTPTest,
                                            handle_observer.page_transition()));
 }
 
+// This is a regression test for https://crbug.com/1020610 - it verifies that
+// NTP navigations do show up as a pending navigation.
+IN_PROC_BROWSER_TEST_F(LocalNTPTest, PendingNavigations) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  // Open an NTP.
+  content::WebContents* ntp_tab = local_ntp_test_utils::OpenNewTab(
+      browser(), GURL(chrome::kChromeUINewTabURL));
+
+  // Inject and click a link to foo.com/hung and wait for the navigation to
+  // start.
+  GURL slow_url(embedded_test_server()->GetURL("/hung"));
+  const char* kNavScriptTemplate = R"(
+      var a = document.createElement('a');
+      a.href = $1;
+      a.innerText = 'Simulated most-visited link';
+      document.body.appendChild(a);
+      a.click();
+  )";
+  content::TestNavigationManager nav_manager(ntp_tab, slow_url);
+  ASSERT_TRUE(content::ExecuteScript(
+      ntp_tab, content::JsReplace(kNavScriptTemplate, slow_url)));
+  ASSERT_TRUE(nav_manager.WaitForRequestStart());
+
+  // Verify that the visible entry points at the |slow_url|.
+  content::NavigationEntry* pending_entry =
+      ntp_tab->GetController().GetPendingEntry();
+  ASSERT_TRUE(pending_entry);
+  content::NavigationEntry* visible_entry =
+      ntp_tab->GetController().GetVisibleEntry();
+  ASSERT_TRUE(visible_entry);
+  content::NavigationEntry* committed_entry =
+      ntp_tab->GetController().GetLastCommittedEntry();
+  ASSERT_TRUE(committed_entry);
+  EXPECT_EQ(visible_entry, pending_entry);
+  EXPECT_EQ(slow_url, visible_entry->GetURL());
+  EXPECT_NE(pending_entry, committed_entry);
+  EXPECT_NE(slow_url, committed_entry->GetURL());
+
+  // Verify that the omnibox displays |slow_url|.
+  OmniboxView* view = browser()->window()->GetLocationBar()->GetOmniboxView();
+  std::string omnibox_text = base::UTF16ToUTF8(view->GetText());
+  EXPECT_THAT(omnibox_text, ::testing::StartsWith(slow_url.host()));
+  EXPECT_THAT(omnibox_text, ::testing::EndsWith(slow_url.path()));
+  EXPECT_THAT(slow_url.spec(), ::testing::EndsWith(omnibox_text));
+}
+
 // Verifies that Chrome won't spawn a separate renderer process for
 // every single NTP tab.  This behavior goes all the way back to
 // the initial commit [1] which achieved that behavior by forcing

content/browser/frame_host/navigation_controller_impl.cc

@@ -456,8 +456,26 @@ static bool g_check_for_repost = true;
 // static
 std::unique_ptr<NavigationEntry> NavigationController::CreateNavigationEntry(
     const GURL& url,
-    const Referrer& referrer,
-    const base::Optional<url::Origin>& initiator_origin,
+    Referrer referrer,
+    base::Optional<url::Origin> initiator_origin,
+    ui::PageTransition transition,
+    bool is_renderer_initiated,
+    const std::string& extra_headers,
+    BrowserContext* browser_context,
+    scoped_refptr<network::SharedURLLoaderFactory> blob_url_loader_factory) {
+  return NavigationControllerImpl::CreateNavigationEntry(
+      url, referrer, std::move(initiator_origin),
+      nullptr /* source_site_instance */, transition, is_renderer_initiated,
+      extra_headers, browser_context, std::move(blob_url_loader_factory));
+}
+
+// static
+std::unique_ptr<NavigationEntryImpl>
+NavigationControllerImpl::CreateNavigationEntry(
+    const GURL& url,
+    Referrer referrer,
+    base::Optional<url::Origin> initiator_origin,
+    SiteInstance* source_site_instance,
     ui::PageTransition transition,
     bool is_renderer_initiated,
     const std::string& extra_headers,
@@ -469,6 +487,12 @@ std::unique_ptr<NavigationEntry> NavigationController::CreateNavigationEntry(
   RewriteUrlForNavigation(url, browser_context, &url_to_load, &virtual_url,
                           &reverse_on_redirect);
 
+  // Let the NTP override the navigation params and pretend that this is a
+  // browser-initiated, bookmark-like navigation.
+  GetContentClient()->browser()->OverrideNavigationParams(
+      source_site_instance, &transition, &is_renderer_initiated, &referrer,
+      &initiator_origin);
+
   auto entry = std::make_unique<NavigationEntryImpl>(
       nullptr,  // The site instance for tabs is sent on navigation
                 // (WebContents::GetSiteInstance).
@@ -2231,8 +2255,9 @@ void NavigationControllerImpl::NavigateFromFrameProxy(
       // TODO(creis): Ensure this case can't exist in https://crbug.com/524208.
       entry = NavigationEntryImpl::FromNavigationEntry(CreateNavigationEntry(
           GURL(url::kAboutBlankURL), referrer, initiator_origin,
-          page_transition, is_renderer_initiated, extra_headers,
-          browser_context_, nullptr /* blob_url_loader_factory */));
+          source_site_instance, page_transition, is_renderer_initiated,
+          extra_headers, browser_context_,
+          nullptr /* blob_url_loader_factory */));
     }
     entry->AddOrUpdateFrameEntry(
         node, -1, -1, nullptr,
@@ -2242,8 +2267,9 @@ void NavigationControllerImpl::NavigateFromFrameProxy(
   } else {
     // Main frame case.
     entry = NavigationEntryImpl::FromNavigationEntry(CreateNavigationEntry(
-        url, referrer, initiator_origin, page_transition, is_renderer_initiated,
-        extra_headers, browser_context_, blob_url_loader_factory));
+        url, referrer, initiator_origin, source_site_instance, page_transition,
+        is_renderer_initiated, extra_headers, browser_context_,
+        blob_url_loader_factory));
     entry->root_node()->frame_entry->set_source_site_instance(
         static_cast<SiteInstanceImpl*>(source_site_instance));
     entry->root_node()->frame_entry->set_method(method);
@@ -2989,8 +3015,9 @@ NavigationControllerImpl::CreateNavigationEntryFromLoadParams(
       // TODO(creis): Ensure this case can't exist in https://crbug.com/524208.
       entry = NavigationEntryImpl::FromNavigationEntry(CreateNavigationEntry(
           GURL(url::kAboutBlankURL), params.referrer, params.initiator_origin,
-          params.transition_type, params.is_renderer_initiated,
-          extra_headers_crlf, browser_context_, blob_url_loader_factory));
+          params.source_site_instance.get(), params.transition_type,
+          params.is_renderer_initiated, extra_headers_crlf, browser_context_,
+          blob_url_loader_factory));
     }
 
     entry->AddOrUpdateFrameEntry(
@@ -3002,8 +3029,9 @@ NavigationControllerImpl::CreateNavigationEntryFromLoadParams(
     // Otherwise, create a pending entry for the main frame.
     entry = NavigationEntryImpl::FromNavigationEntry(CreateNavigationEntry(
         params.url, params.referrer, params.initiator_origin,
-        params.transition_type, params.is_renderer_initiated,
-        extra_headers_crlf, browser_context_, blob_url_loader_factory));
+        params.source_site_instance.get(), params.transition_type,
+        params.is_renderer_initiated, extra_headers_crlf, browser_context_,
+        blob_url_loader_factory));
     entry->set_source_site_instance(
         static_cast<SiteInstanceImpl*>(params.source_site_instance.get()));
     entry->SetRedirectChain(params.redirect_chain);

content/browser/frame_host/navigation_controller_impl.h

@@ -305,6 +305,19 @@ class CONTENT_EXPORT NavigationControllerImpl : public NavigationController {
   // requests corresponding to the current pending entry.
   std::unique_ptr<PendingEntryRef> ReferencePendingEntry();
 
+  // Like NavigationController::CreateNavigationEntry, but takes an extra
+  // |source_site_instance| argument.
+  static std::unique_ptr<NavigationEntryImpl> CreateNavigationEntry(
+      const GURL& url,
+      Referrer referrer,
+      base::Optional<url::Origin> initiator_origin,
+      SiteInstance* source_site_instance,
+      ui::PageTransition transition,
+      bool is_renderer_initiated,
+      const std::string& extra_headers,
+      BrowserContext* browser_context,
+      scoped_refptr<network::SharedURLLoaderFactory> blob_url_loader_factory);
+
  private:
   friend class RestoreHelper;
 

content/browser/frame_host/navigation_request.cc

@@ -898,7 +898,7 @@ NavigationRequest::NavigationRequest(
       commit_navigation_client_(mojo::NullAssociatedRemote()),
       rfh_restored_from_back_forward_cache_(
           rfh_restored_from_back_forward_cache) {
-  DCHECK(browser_initiated || common_params_->initiator_origin.has_value());
+  DCHECK(browser_initiated_ || common_params_->initiator_origin.has_value());
   DCHECK(!IsRendererDebugURL(common_params_->url));
   DCHECK(common_params_->method == "POST" || !common_params_->post_data);
   TRACE_EVENT_ASYNC_BEGIN2("navigation", "NavigationRequest", this,
@@ -957,6 +957,20 @@ NavigationRequest::NavigationRequest(
     DCHECK(!RequiresSourceSiteInstance() || source_site_instance_);
   }
 
+  // Let the NTP override the navigation params and pretend that this is a
+  // browser-initiated, bookmark-like navigation.
+  if (!browser_initiated_ && source_site_instance_) {
+    bool is_renderer_initiated = !browser_initiated_;
+    Referrer referrer(*common_params_->referrer);
+    GetContentClient()->browser()->OverrideNavigationParams(
+        source_site_instance_.get(), &common_params_->transition,
+        &is_renderer_initiated, &referrer, &common_params_->initiator_origin);
+    common_params_->referrer =
+        blink::mojom::Referrer::New(referrer.url, referrer.policy);
+    browser_initiated_ = !is_renderer_initiated;
+    commit_params_->is_browser_initiated = browser_initiated_;
+  }
+
   // Store the old RenderFrameHost id at request creation to be used later.
   previous_render_frame_host_id_ = GlobalFrameRoutingId(
       frame_tree_node->current_frame_host()->GetProcess()->GetID(),
@@ -1026,11 +1040,11 @@ NavigationRequest::NavigationRequest(
         common_params_->referrer->policy, frame_tree_node);
 
     if (begin_params_->is_form_submission) {
-      if (browser_initiated && !commit_params_->post_content_type.empty()) {
+      if (browser_initiated_ && !commit_params_->post_content_type.empty()) {
         // This is a form resubmit, so make sure to set the Content-Type header.
         headers.SetHeaderIfMissing(net::HttpRequestHeaders::kContentType,
                                    commit_params_->post_content_type);
-      } else if (!browser_initiated) {
+      } else if (!browser_initiated_) {
         // Save the Content-Type in case the form is resubmitted. This will get
         // sent back to the renderer in the CommitNavigation IPC. The renderer
         // will then send it back with the post body so that we can access it
@@ -1241,18 +1255,6 @@ void NavigationRequest::StartNavigation(bool is_for_commit) {
       frame_tree_node->current_frame_host()->GetSiteInstance();
   site_url_ = GetSiteForCommonParamsURL();
 
-  // Let the NTP override the navigation params and pretend that this is a
-  // browser-initiated, bookmark-like navigation.
-  bool is_renderer_initiated = !browser_initiated_;
-  Referrer referrer(*common_params_->referrer);
-  GetContentClient()->browser()->OverrideNavigationParams(
-      starting_site_instance_.get(), &common_params_->transition,
-      &is_renderer_initiated, &referrer, &common_params_->initiator_origin);
-  common_params_->referrer =
-      blink::mojom::Referrer::New(referrer.url, referrer.policy);
-  browser_initiated_ = !is_renderer_initiated;
-  commit_params_->is_browser_initiated = browser_initiated_;
-
   // Compute the redirect chain.
   // TODO(clamy): Try to simplify this and have the redirects be part of
   // CommonNavigationParams.

content/browser/frame_host/navigator_impl.cc

@@ -749,12 +749,20 @@ NavigatorImpl::GetNavigationEntryForRendererInitiatedNavigation(
   if (has_transient_entry)
     return nullptr;
 
+  // Since GetNavigationEntryForRendererInitiatedNavigation is called from
+  // OnBeginNavigation, we can assume that no frame proxies are involved and
+  // therefore that |current_site_instance| is also the |source_site_instance|.
+  SiteInstance* current_site_instance =
+      frame_tree_node->current_frame_host()->GetSiteInstance();
+  SiteInstance* source_site_instance = current_site_instance;
+
   std::unique_ptr<NavigationEntryImpl> entry =
       NavigationEntryImpl::FromNavigationEntry(
-          NavigationController::CreateNavigationEntry(
+          NavigationControllerImpl::CreateNavigationEntry(
               common_params.url, content::Referrer(),
-              common_params.initiator_origin, ui::PAGE_TRANSITION_LINK,
-              true /* is_renderer_initiated */, std::string(),
+              common_params.initiator_origin, source_site_instance,
+              ui::PAGE_TRANSITION_LINK, true /* is_renderer_initiated */,
+              std::string() /* extra_headers */,
               controller_->GetBrowserContext(),
               nullptr /* blob_url_loader_factory */));
 

content/public/browser/navigation_controller.h

@@ -105,8 +105,8 @@ class NavigationController {
   // Extra headers are separated by \n.
   CONTENT_EXPORT static std::unique_ptr<NavigationEntry> CreateNavigationEntry(
       const GURL& url,
-      const Referrer& referrer,
-      const base::Optional<url::Origin>& initiator_origin,
+      Referrer referrer,
+      base::Optional<url::Origin> initiator_origin,
       ui::PageTransition transition,
       bool is_renderer_initiated,
       const std::string& extra_headers,