InstantExtended: Send search URLs to renderers.

                                                                                
Previously, navigations initiated by an instant renderer were bounced to
the browser to be rebucketed into an instant or non-instant renderer.
But navigations from non-instant renderers to instant URLs (i.e.
privileged search page URLs) were not rebucketed, because the renderer
had no knowledge of which URLs were instant URLs, and it would be too
expensive to bounce ALL URLs. As a result, non-instant pages like
google.com/setprefs could not redirect to instant pages like
google.com/search.
                                                                        
This change has InstantService tell renderers about URLs associated with
the default search engine, and uses this knowledge to bounce
renderer-initiated navigations to likely instant URLs from non-instant
processes back into the browser for rebucketing. So now link clicks from
non-instant pages to instant pages will put the destination pages into
an instant process. Unfortunately, though, redirects are still broken.
For example,
                                                                                
0. User clicks "Set preferences" button on an instant page.
1. Instant renderer bounces google.com/setprefs to the browser.
2. Browser says google.com/setprefs is not an instant URL,
   and assigns it to a non-instant renderer.
3. After rebucketing to the non-instant renderer, google.com/setprefs
   is marked as a browser initiated request(!)
4. /setprefs redirects to /search, which _is_ an instant URL, but
   because /setprefs is marked as browser initiated the non-instant
   renderer does not get a chance to bounce it back to the browser.
                                                                                
I tried working around this by giving redirects a chance to bounce back
to the browser in step #4, but this broke the signin process model in a
scary way that I don't fully understand. It seems like the right fix
here is going to need to EITHER follow the redirect chain in step #2
when determining if an URL is an instant URL, OR somehow flag the
bounced redirect request for further inspection in step #3.

This change also includes a small side benefit. Since renderers now know
about instant URLs, we can suppress a flash of a baked-in error page in
the event of an error while loading the InstantExtended new tab page.

TEST=manual,unit,browsertest
BUG=271088,223754

Review URL: https://codereview.chromium.org/23455047

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@226103 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chrome_content_browser_client_browsertest.cc

@@ -94,7 +94,19 @@ IN_PROC_BROWSER_TEST_F(ChromeContentBrowserClientBrowserTest,
   EXPECT_EQ(url, entry->GetVirtualURL());
 }
 
-IN_PROC_BROWSER_TEST_F(ChromeContentBrowserClientBrowserTest,
+class InstantNTPURLRewriteTest : public ChromeContentBrowserClientBrowserTest {
+ public:
+  virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
+    // browsertest initially spins up a non-Instant renderer for about:blank.
+    // In a real browser, navigating this renderer to the Instant new tab page
+    // forks a new privileged Instant render process, but that warps
+    // browsertest's fragile little mind; it just never navigates. We aren't
+    // trying to test the process model here, so turn it off.
+    CommandLine::ForCurrentProcess()->AppendSwitch(switches::kSingleProcess);
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(InstantNTPURLRewriteTest,
                        UberURLHandler_InstantExtendedNewTabPage) {
   const GURL url_original("chrome://newtab");
   const GURL url_rewritten("http://example.com/newtab");
@@ -113,6 +125,8 @@ IN_PROC_BROWSER_TEST_F(ChromeContentBrowserClientBrowserTest,
 
 IN_PROC_BROWSER_TEST_F(ChromeContentBrowserClientBrowserTest,
                        UberURLHandler_InstantExtendedNewTabPageDisabled) {
+  // Don't do the kSingleProcess shenanigans here (see the dual test) because
+  // otherwise RenderViewImpl crashes in a paranoid fit on startup.
   const GURL url_original("chrome://newtab");
   const GURL url_rewritten("http://example.com/newtab");
   InstallTemplateURLWithNewTabPage(url_rewritten);

chrome/browser/search/instant_service.cc

@@ -30,6 +30,7 @@
 #include "chrome/browser/ui/webui/ntp/thumbnail_source.h"
 #include "chrome/browser/ui/webui/theme_source.h"
 #include "chrome/common/pref_names.h"
+#include "chrome/common/render_messages.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_service.h"
@@ -72,6 +73,9 @@ InstantService::InstantService(Profile* profile)
   if (!BrowserThread::CurrentlyOn(BrowserThread::UI))
     return;
 
+  registrar_.Add(this,
+                 content::NOTIFICATION_RENDERER_PROCESS_CREATED,
+                 content::NotificationService::AllSources());
   registrar_.Add(this,
                  content::NOTIFICATION_RENDERER_PROCESS_TERMINATED,
                  content::NotificationService::AllSources());
@@ -233,21 +237,14 @@ void InstantService::Observe(int type,
                              const content::NotificationSource& source,
                              const content::NotificationDetails& details) {
   switch (type) {
-    case content::NOTIFICATION_RENDERER_PROCESS_TERMINATED: {
-      int process_id =
-          content::Source<content::RenderProcessHost>(source)->GetID();
-      process_ids_.erase(process_id);
-
-      if (instant_io_context_.get()) {
-        BrowserThread::PostTask(
-            BrowserThread::IO,
-            FROM_HERE,
-            base::Bind(&InstantIOContext::RemoveInstantProcessOnIO,
-                       instant_io_context_,
-                       process_id));
-      }
+    case content::NOTIFICATION_RENDERER_PROCESS_CREATED:
+      SendSearchURLsToRenderer(
+          content::Source<content::RenderProcessHost>(source).ptr());
+      break;
+    case content::NOTIFICATION_RENDERER_PROCESS_TERMINATED:
+      OnRendererProcessTerminated(
+          content::Source<content::RenderProcessHost>(source)->GetID());
       break;
-    }
     case chrome::NOTIFICATION_TOP_SITES_CHANGED: {
       history::TopSites* top_sites = profile_->GetTopSites();
       if (top_sites) {
@@ -284,6 +281,24 @@ void InstantService::Observe(int type,
   }
 }
 
+void InstantService::SendSearchURLsToRenderer(content::RenderProcessHost* rph) {
+  rph->Send(new ChromeViewMsg_SetSearchURLs(
+      chrome::GetSearchURLs(profile_), chrome::GetNewTabPageURL(profile_)));
+}
+
+void InstantService::OnRendererProcessTerminated(int process_id) {
+  process_ids_.erase(process_id);
+
+  if (instant_io_context_.get()) {
+    BrowserThread::PostTask(
+        BrowserThread::IO,
+        FROM_HERE,
+        base::Bind(&InstantIOContext::RemoveInstantProcessOnIO,
+                   instant_io_context_,
+                   process_id));
+  }
+}
+
 void InstantService::OnMostVisitedItemsReceived(
     const history::MostVisitedURLList& data) {
   history::MostVisitedURLList reordered_data(data);

chrome/browser/search/instant_service.h

@@ -29,6 +29,8 @@
 class GURL;
 class InstantIOContext;
 class InstantServiceObserver;
+class InstantTestBase;
+class InstantServiceTest;
 class Profile;
 class ThemeService;
 
@@ -102,6 +104,9 @@ class InstantService : public BrowserContextKeyedService,
   // object. Used to destroy the preloaded InstantNTP.
   void OnBrowserInstantControllerDestroyed();
 
+  // Sends the current set of search URLs to a renderer process.
+  void SendSearchURLsToRenderer(content::RenderProcessHost* rph);
+
  private:
   friend class InstantExtendedTest;
   friend class InstantServiceTest;
@@ -115,6 +120,7 @@ class InstantService : public BrowserContextKeyedService,
   FRIEND_TEST_ALL_PREFIXES(InstantExtendedManualTest,
                            MANUAL_SearchesFromFakebox);
   FRIEND_TEST_ALL_PREFIXES(InstantExtendedTest, ProcessIsolation);
+  FRIEND_TEST_ALL_PREFIXES(InstantServiceTest, SendsSearchURLsToRenderer);
 
   // Overridden from BrowserContextKeyedService:
   virtual void Shutdown() OVERRIDE;
@@ -124,6 +130,9 @@ class InstantService : public BrowserContextKeyedService,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
 
+  // Called when a renderer process is terminated.
+  void OnRendererProcessTerminated(int process_id);
+
   // Called when we get new most visited items from TopSites, registered as an
   // async callback. Parses them and sends them to the renderer via
   // SendMostVisitedItems.

chrome/browser/search/instant_service_unittest.cc

@@ -6,6 +6,7 @@
 #include <vector>
 
 #include "base/memory/scoped_ptr.h"
+#include "base/metrics/field_trial.h"
 #include "base/strings/string_util.h"
 #include "chrome/browser/search/instant_service.h"
 #include "chrome/browser/search/instant_service_observer.h"
@@ -14,13 +15,17 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/host_desktop.h"
+#include "chrome/common/render_messages.h"
+#include "components/variations/entropy_provider.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/notification_types.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
+#include "content/public/test/mock_render_process_host.h"
+#include "ipc/ipc_message.h"
+#include "ipc/ipc_test_sink.h"
 #include "testing/gmock/include/gmock/gmock.h"
-
-namespace chrome {
-
-namespace {
+#include "url/gurl.h"
 
 class MockInstantServiceObserver : public InstantServiceObserver {
  public:
@@ -48,6 +53,8 @@ class InstantServiceTest : public InstantUnitTestBase {
  protected:
   virtual void SetUp() OVERRIDE {
     InstantUnitTestBase::SetUp();
+    field_trial_list_.reset(new base::FieldTrialList(
+        new metrics::SHA1EntropyProvider("42")));
 
     instant_service_observer_.reset(new MockInstantServiceObserver());
     instant_service_->AddObserver(instant_service_observer_.get());
@@ -65,6 +72,7 @@ class InstantServiceTest : public InstantUnitTestBase {
 
   scoped_ptr<MockInstantServiceObserver> instant_service_observer_;
   scoped_ptr<MockWebContentsObserver> instant_ntp_contents_observer_;
+  scoped_ptr<base::FieldTrialList> field_trial_list_;
 };
 
 TEST_F(InstantServiceTest, DispatchDefaultSearchProviderChanged) {
@@ -96,6 +104,24 @@ TEST_F(InstantServiceTest, DispatchGoogleURLUpdated) {
   EXPECT_TRUE(StartsWithASCII(new_ntp_url.spec(), new_base_url, true));
 }
 
-}  // namespace
-
-}  // namespace chrome
+TEST_F(InstantServiceTest, SendsSearchURLsToRenderer) {
+  ASSERT_TRUE(base::FieldTrialList::CreateFieldTrial("InstantExtended",
+      "Group1 use_cacheable_ntp:1"));
+  scoped_ptr<content::MockRenderProcessHost> rph(
+      new content::MockRenderProcessHost(profile()));
+  rph->sink().ClearMessages();
+  instant_service_->Observe(
+      content::NOTIFICATION_RENDERER_PROCESS_CREATED,
+      content::Source<content::MockRenderProcessHost>(rph.get()),
+      content::NotificationService::NoDetails());
+  EXPECT_EQ(1U, rph->sink().message_count());
+  const IPC::Message* msg = rph->sink().GetMessageAt(0);
+  ASSERT_TRUE(msg);
+  std::vector<GURL> search_urls;
+  GURL new_tab_page_url;
+  ChromeViewMsg_SetSearchURLs::Read(msg, &search_urls, &new_tab_page_url);
+  EXPECT_EQ(2U, search_urls.size());
+  EXPECT_EQ("https://www.google.com/alt#quux=", search_urls[0].spec());
+  EXPECT_EQ("https://www.google.com/url?bar=", search_urls[1].spec());
+  EXPECT_EQ("https://www.google.com/newtab", new_tab_page_url.spec());
+}

chrome/browser/search/instant_unittest_base.cc

@@ -52,6 +52,7 @@ void InstantUnitTestBase::SetDefaultSearchProvider(
   data.SetURL(base_url + "url?bar={searchTerms}");
   data.instant_url = base_url + "instant?"
                      "{google:omniboxStartMarginParameter}foo=foo#foo=foo&strk";
+  data.new_tab_url = base_url + "newtab";
   data.alternate_urls.push_back(base_url + "alt#quux={searchTerms}");
   data.search_terms_replacement_key = "strk";
 

chrome/browser/search/search.cc

@@ -25,6 +25,7 @@
 #include "chrome/browser/ui/browser_iterator.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
+#include "chrome/common/search_urls.h"
 #include "chrome/common/url_constants.h"
 #include "components/sessions/serialized_navigation_entry.h"
 #include "components/user_prefs/pref_registry_syncable.h"
@@ -129,25 +130,19 @@ GURL TemplateURLRefToGURL(const TemplateURLRef& ref,
   return GURL(ref.ReplaceSearchTerms(search_terms_args));
 }
 
-bool MatchesOrigin(const GURL& my_url, const GURL& other_url) {
-  return my_url.host() == other_url.host() &&
-         my_url.port() == other_url.port() &&
-         (my_url.scheme() == other_url.scheme() ||
-          (my_url.SchemeIs(content::kHttpsScheme) &&
-           other_url.SchemeIs(content::kHttpScheme)));
-}
-
 bool MatchesAnySearchURL(const GURL& url, TemplateURL* template_url) {
   GURL search_url =
       TemplateURLRefToGURL(template_url->url_ref(), kDisableStartMargin, false);
-  if (search_url.is_valid() && MatchesOriginAndPath(url, search_url))
+  if (search_url.is_valid() &&
+      search::MatchesOriginAndPath(url, search_url))
     return true;
 
   // "URLCount() - 1" because we already tested url_ref above.
   for (size_t i = 0; i < template_url->URLCount() - 1; ++i) {
     TemplateURLRef ref(template_url, i);
     search_url = TemplateURLRefToGURL(ref, kDisableStartMargin, false);
-    if (search_url.is_valid() && MatchesOriginAndPath(url, search_url))
+    if (search_url.is_valid() &&
+        search::MatchesOriginAndPath(url, search_url))
       return true;
   }
 
@@ -209,7 +204,8 @@ bool IsInstantURL(const GURL& url, Profile* profile) {
     return false;
 
   const GURL new_tab_url(GetNewTabPageURL(profile));
-  if (new_tab_url.is_valid() && MatchesOriginAndPath(url, new_tab_url))
+  if (new_tab_url.is_valid() &&
+      search::MatchesOriginAndPath(url, new_tab_url))
     return true;
 
   TemplateURL* template_url = GetDefaultSearchProviderTemplateURL(profile);
@@ -225,7 +221,7 @@ bool IsInstantURL(const GURL& url, Profile* profile) {
   if (!instant_url.is_valid())
     return false;
 
-  if (MatchesOriginAndPath(url, instant_url))
+  if (search::MatchesOriginAndPath(url, instant_url))
     return true;
 
   return !ShouldSuppressInstantExtendedOnSRP() &&
@@ -394,7 +390,7 @@ bool NavEntryIsInstantNTP(const content::WebContents* contents,
   if (ShouldUseCacheableNTP()) {
     GURL new_tab_url(GetNewTabPageURL(profile));
     return new_tab_url.is_valid() &&
-        MatchesOriginAndPath(entry->GetURL(), new_tab_url);
+        search::MatchesOriginAndPath(entry->GetURL(), new_tab_url);
   }
 
   return IsInstantURL(entry->GetVirtualURL(), profile) &&
@@ -432,6 +428,32 @@ GURL GetInstantURL(Profile* profile, int start_margin) {
   return instant_url.ReplaceComponents(replacements);
 }
 
+// Returns URLs associated with the default search engine for |profile|.
+std::vector<GURL> GetSearchURLs(Profile* profile) {
+  std::vector<GURL> result;
+  TemplateURL* template_url = GetDefaultSearchProviderTemplateURL(profile);
+  for (size_t i = 0; i < template_url->URLCount(); ++i) {
+    TemplateURLRef ref(template_url, i);
+    result.push_back(TemplateURLRefToGURL(ref, kDisableStartMargin, false));
+  }
+  return result;
+}
+
+GURL GetNewTabPageURL(Profile* profile) {
+  if (!ShouldUseCacheableNTP())
+    return GURL();
+
+  if (!profile || !IsSuggestPrefEnabled(profile))
+    return GURL();
+
+  TemplateURL* template_url = GetDefaultSearchProviderTemplateURL(profile);
+  if (!template_url)
+    return GURL();
+
+  return TemplateURLRefToGURL(template_url->new_tab_url_ref(),
+                              kDisableStartMargin, false);
+}
+
 GURL GetLocalInstantURL(Profile* profile) {
   return GURL(chrome::kChromeSearchLocalNtpUrl);
 }
@@ -508,10 +530,6 @@ bool ShouldSuppressInstantExtendedOnSRP() {
   return false;
 }
 
-bool MatchesOriginAndPath(const GURL& my_url, const GURL& other_url) {
-  return MatchesOrigin(my_url, other_url) && my_url.path() == other_url.path();
-}
-
 GURL GetEffectiveURLForInstant(const GURL& url, Profile* profile) {
   CHECK(ShouldAssignURLToInstantRenderer(url, profile))
       << "Error granting Instant access.";
@@ -534,7 +552,8 @@ GURL GetEffectiveURLForInstant(const GURL& url, Profile* profile) {
   if (template_url) {
     const GURL instant_url = TemplateURLRefToGURL(
         template_url->instant_url_ref(), kDisableStartMargin, false);
-    if (instant_url.is_valid() && MatchesOriginAndPath(url, instant_url)) {
+    if (instant_url.is_valid() &&
+        search::MatchesOriginAndPath(url, instant_url)) {
       replacements.SetHost(online_ntp_host.c_str(),
                            url_parse::Component(0, online_ntp_host.length()));
     }
@@ -606,7 +625,8 @@ bool HandleNewTabURLReverseRewrite(GURL* url,
 
   Profile* profile = Profile::FromBrowserContext(browser_context);
   GURL new_tab_url(GetNewTabPageURL(profile));
-  if (!new_tab_url.is_valid() || !MatchesOriginAndPath(new_tab_url, *url))
+  if (!new_tab_url.is_valid() ||
+      !search::MatchesOriginAndPath(new_tab_url, *url))
     return false;
 
   *url = GURL(chrome::kChromeUINewTabURL);
@@ -739,21 +759,6 @@ bool GetBoolValueForFlagWithDefault(const std::string& flag,
   return !!GetUInt64ValueForFlagWithDefault(flag, default_value ? 1 : 0, flags);
 }
 
-GURL GetNewTabPageURL(Profile* profile) {
-  if (!ShouldUseCacheableNTP())
-    return GURL();
-
-  if (!profile || !IsSuggestPrefEnabled(profile))
-    return GURL();
-
-  TemplateURL* template_url = GetDefaultSearchProviderTemplateURL(profile);
-  if (!template_url)
-    return GURL();
-
-  return TemplateURLRefToGURL(template_url->new_tab_url_ref(),
-                              kDisableStartMargin, false);
-}
-
 void ResetInstantExtendedOptInStateGateForTest() {
   instant_extended_opt_in_state_gate = false;
 }