Fixed NTP site instance sharing bug

This fixes the bug when every window opened from remote NTP tab with
same host as remote NTP but different path would be considered to belong
to same SiteInstance as NTP. That could lead to nasty effects, such as
referrer reset for transfers from Yandex remote NTP.
Basically its a rewrite of
https://chromium-review.googlesource.com/c/chromium/src/+/998178 patch
so it would apply to hosted apps only, as intended, IMHO.

Bug: 859062,718516
Change-Id: I76e90c2ff6311ca99a234e75424f8cba9932c05a
Reviewed-on: https://chromium-review.googlesource.com/1141574
Commit-Queue: Alexander Yashkin <a-v-y@yandex-team.ru>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583199}

chrome/browser/chrome_content_browser_client.cc

@@ -1332,6 +1332,25 @@ GURL ChromeContentBrowserClient::GetEffectiveURL(
 #endif
 }
 
+bool ChromeContentBrowserClient::
+    ShouldCompareEffectiveURLsForSiteInstanceSelection(
+        content::BrowserContext* browser_context,
+        content::SiteInstance* candidate_site_instance,
+        bool is_main_frame,
+        const GURL& candidate_url,
+        const GURL& destination_url) {
+  DCHECK(browser_context);
+  DCHECK(candidate_site_instance);
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+  return ChromeContentBrowserClientExtensionsPart::
+      ShouldCompareEffectiveURLsForSiteInstanceSelection(
+          browser_context, candidate_site_instance, is_main_frame,
+          candidate_url, destination_url);
+#else
+  return true;
+#endif
+}
+
 bool ChromeContentBrowserClient::ShouldUseMobileFlingCurve() const {
 #if defined(OS_ANDROID)
   return true;

chrome/browser/chrome_content_browser_client.h

@@ -109,6 +109,12 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient {
   bool AllowGpuLaunchRetryOnIOThread() override;
   GURL GetEffectiveURL(content::BrowserContext* browser_context,
                        const GURL& url) override;
+  bool ShouldCompareEffectiveURLsForSiteInstanceSelection(
+      content::BrowserContext* browser_context,
+      content::SiteInstance* candidate_site_instance,
+      bool is_main_frame,
+      const GURL& candidate_url,
+      const GURL& destination_url) override;
   bool ShouldUseMobileFlingCurve() const override;
   bool ShouldUseProcessPerSite(content::BrowserContext* browser_context,
                                const GURL& effective_url) override;

chrome/browser/chrome_content_browser_client_browsertest.cc

@@ -32,6 +32,7 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/browser_test_utils.h"
+#include "content/public/test/test_navigation_observer.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/http/http_status_code.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
@@ -399,4 +400,72 @@ IN_PROC_BROWSER_TEST_F(PolicyHeaderServiceBrowserTest,
   EXPECT_EQ(iter->second, kTestPolicyHeader);
 }
 
+// Helper class to test window creation from NTP.
+class OpenWindowFromNTPBrowserTest : public InProcessBrowserTest,
+                                     public InstantTestBase {
+ public:
+  OpenWindowFromNTPBrowserTest() {}
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendSwitch(switches::kIgnoreCertificateErrors);
+  }
+
+  void SetUpOnMainThread() override {
+    InProcessBrowserTest::SetUpOnMainThread();
+    host_resolver()->AddRule("*", "127.0.0.1");
+    ASSERT_TRUE(https_test_server().InitializeAndListen());
+    https_test_server().StartAcceptingConnections();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(OpenWindowFromNTPBrowserTest);
+};
+
+// Test checks that navigations from NTP tab to URLs with same host as NTP but
+// different path do not reuse NTP SiteInstance. See https://crbug.com/859062
+// for details.
+IN_PROC_BROWSER_TEST_F(OpenWindowFromNTPBrowserTest,
+                       TransferFromNTPCreateNewTab) {
+  GURL search_url =
+      https_test_server().GetURL("ntp.com", "/instant_extended.html");
+  GURL ntp_url =
+      https_test_server().GetURL("ntp.com", "/instant_extended_ntp.html");
+  InstantTestBase::Init(search_url, ntp_url, false);
+
+  SetupInstant(browser());
+
+  // Navigate to the NTP URL and verify that the resulting process is marked as
+  // an Instant process.
+  ui_test_utils::NavigateToURL(browser(), ntp_url);
+  content::WebContents* ntp_tab =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  InstantService* instant_service =
+      InstantServiceFactory::GetForProfile(browser()->profile());
+  EXPECT_TRUE(instant_service->IsInstantProcess(
+      ntp_tab->GetMainFrame()->GetProcess()->GetID()));
+
+  // Execute script that creates new window from ntp tab with
+  // ntp.com/title1.html as target url. Host is same as remote-ntp host, yet
+  // path is different.
+  GURL generic_url(https_test_server().GetURL("ntp.com", "/title1.html"));
+  content::TestNavigationObserver opened_tab_observer(nullptr);
+  opened_tab_observer.StartWatchingNewWebContents();
+  EXPECT_TRUE(
+      ExecuteScript(ntp_tab, "window.open('" + generic_url.spec() + "');"));
+  opened_tab_observer.Wait();
+  ASSERT_EQ(2, browser()->tab_strip_model()->count());
+
+  content::WebContents* opened_tab =
+      browser()->tab_strip_model()->GetActiveWebContents();
+
+  // Wait until newly opened tab is fully loaded.
+  EXPECT_TRUE(WaitForLoadStop(opened_tab));
+
+  EXPECT_NE(opened_tab, ntp_tab);
+  EXPECT_EQ(generic_url, opened_tab->GetLastCommittedURL());
+  // New created tab should not reside in an Instant process.
+  EXPECT_FALSE(instant_service->IsInstantProcess(
+      opened_tab->GetMainFrame()->GetProcess()->GetID()));
+}
+
 }  // namespace content

chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc

@@ -268,6 +268,12 @@ const Extension* GetEnabledExtensionFromEffectiveURL(
   return registry->enabled_extensions().GetByID(effective_url.host());
 }
 
+bool HasEffectiveUrl(content::BrowserContext* browser_context,
+                     const GURL& url) {
+  return ChromeContentBrowserClientExtensionsPart::GetEffectiveURL(
+             Profile::FromBrowserContext(browser_context), url) != url;
+}
+
 }  // namespace
 
 ChromeContentBrowserClientExtensionsPart::
@@ -329,6 +335,35 @@ GURL ChromeContentBrowserClientExtensionsPart::GetEffectiveURL(
   return extension->GetResourceURL(url.path());
 }
 
+// static
+bool ChromeContentBrowserClientExtensionsPart::
+    ShouldCompareEffectiveURLsForSiteInstanceSelection(
+        content::BrowserContext* browser_context,
+        content::SiteInstance* candidate_site_instance,
+        bool is_main_frame,
+        const GURL& candidate_url,
+        const GURL& destination_url) {
+  // Don't compare effective URLs for any subframe navigations, since we don't
+  // want to create OOPIFs based on that mechanism (e.g., for hosted apps). For
+  // main frames, don't compare effective URLs when transitioning from app to
+  // non-app URLs if there exists another app WebContents that might script
+  // this one.  These navigations should stay in the app process to not break
+  // scripting when a hosted app opens a same-site popup. See
+  // https://crbug.com/718516 and https://crbug.com/828720 and
+  // https://crbug.com/859062.
+  if (!is_main_frame)
+    return false;
+  size_t candidate_active_contents_count =
+      candidate_site_instance->GetRelatedActiveContentsCount();
+  bool src_has_effective_url = HasEffectiveUrl(browser_context, candidate_url);
+  bool dest_has_effective_url =
+      HasEffectiveUrl(browser_context, destination_url);
+  if (src_has_effective_url && !dest_has_effective_url &&
+      candidate_active_contents_count > 1u)
+    return false;
+  return true;
+}
+
 // static
 bool ChromeContentBrowserClientExtensionsPart::ShouldUseProcessPerSite(
     Profile* profile, const GURL& effective_url) {

chrome/browser/extensions/chrome_content_browser_client_extensions_part.h

@@ -36,6 +36,12 @@ class ChromeContentBrowserClientExtensionsPart
 
   // Corresponds to the ChromeContentBrowserClient function of the same name.
   static GURL GetEffectiveURL(Profile* profile, const GURL& url);
+  static bool ShouldCompareEffectiveURLsForSiteInstanceSelection(
+      content::BrowserContext* browser_context,
+      content::SiteInstance* candidate_site_instance,
+      bool is_main_frame,
+      const GURL& candidate_url,
+      const GURL& destination_url);
   static bool ShouldUseProcessPerSite(Profile* profile,
                                       const GURL& effective_url);
   static bool ShouldUseSpareRenderProcessHost(Profile* profile,

content/browser/frame_host/render_frame_host_manager.cc

@@ -1527,22 +1527,22 @@ bool RenderFrameHostManager::IsCurrentlySameSite(RenderFrameHostImpl* candidate,
                                                  const GURL& dest_url) {
   BrowserContext* browser_context =
       delegate_->GetControllerForRenderManager().GetBrowserContext();
-  // Don't compare effective URLs for all subframe navigations, since we don't
-  // want to create OOPIFs based on that mechanism (e.g., for hosted apps). For
-  // main frames, don't compare effective URLs when transitioning from app to
-  // non-app URLs if there exists another app WebContents that might script
-  // this one.  These navigations should stay in the app process to not break
-  // scripting when a hosted app opens a same-site popup. See
-  // https://crbug.com/718516 and https://crbug.com/828720.
+
+  // Ask embedder whether effective URLs should be used when determining if
+  // |dest_url| should end up in |candidate|'s SiteInstance.
+  // This is used to keep same-site scripting working for hosted apps.
+  bool should_compare_effective_urls =
+      GetContentClient()
+          ->browser()
+          ->ShouldCompareEffectiveURLsForSiteInstanceSelection(
+              browser_context, candidate->GetSiteInstance(),
+              frame_tree_node_->IsMainFrame(),
+              candidate->GetSiteInstance()->original_url(), dest_url);
+
   bool src_has_effective_url = SiteInstanceImpl::HasEffectiveURL(
       browser_context, candidate->GetSiteInstance()->original_url());
   bool dest_has_effective_url =
       SiteInstanceImpl::HasEffectiveURL(browser_context, dest_url);
-  bool should_compare_effective_urls = true;
-  if (!frame_tree_node_->IsMainFrame() ||
-      (src_has_effective_url && !dest_has_effective_url &&
-       candidate->GetSiteInstance()->GetRelatedActiveContentsCount() > 1u))
-    should_compare_effective_urls = false;
 
   // If the process type is incorrect, reject the candidate even if |dest_url|
   // is same-site.  (The URL may have been installed as an app since

content/public/browser/content_browser_client.cc

@@ -81,6 +81,15 @@ GURL ContentBrowserClient::GetEffectiveURL(BrowserContext* browser_context,
   return url;
 }
 
+bool ContentBrowserClient::ShouldCompareEffectiveURLsForSiteInstanceSelection(
+    BrowserContext* browser_context,
+    content::SiteInstance* candidate_site_instance,
+    bool is_main_frame,
+    const GURL& candidate_url,
+    const GURL& destination_url) {
+  return true;
+}
+
 bool ContentBrowserClient::ShouldUseMobileFlingCurve() const {
   return false;
 }

content/public/browser/content_browser_client.h

@@ -257,6 +257,16 @@ class CONTENT_EXPORT ContentBrowserClient {
   virtual GURL GetEffectiveURL(BrowserContext* browser_context,
                                const GURL& url);
 
+  // Returns true if effective URLs should be compared when choosing a
+  // SiteInstance for a navigation to |destination_url|.
+  // |is_main_frame| is true if the navigation will take place in a main frame.
+  virtual bool ShouldCompareEffectiveURLsForSiteInstanceSelection(
+      BrowserContext* browser_context,
+      content::SiteInstance* candidate_site_instance,
+      bool is_main_frame,
+      const GURL& candidate_url,
+      const GURL& destination_url);
+
   // Returns whether gesture fling events should use the mobile-behavior gesture
   // curve for scrolling.
   virtual bool ShouldUseMobileFlingCurve() const;