Fix Chrome Web Store loading in an isolated origin.

When a hosted app URL also corresponds to an isolated origin, the
isolated origin takes precedence, and the corresponding SiteInstance
won't use the effective URL for the hosted app.  However, this logic
needs to exclude CWS, which still needs to resolve to its effective
URL, so that the corresponding process ends up in the ProcessMap for
the CWS extension ID.  Otherwise, security checks such as CanCommitURL
won't allow CWS navigations to succeed.

Bug: 788837
Change-Id: I2b8d03d044e72bb9b8f71cb4c3accfba8d907ac4
Reviewed-on: https://chromium-review.googlesource.com/792596Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519701}

chrome/browser/chrome_content_browser_client.cc

@@ -1151,19 +1151,14 @@ GURL ChromeContentBrowserClient::GetEffectiveURL(
 
   // If the input |url| should be assigned to the Instant renderer, make its
   // effective URL distinct from other URLs on the search provider's domain.
+  // This needs to happen even if |url| corresponds to an isolated origin; see
+  // https://crbug.com/755595.
   if (search::ShouldAssignURLToInstantRenderer(url, profile))
     return search::GetEffectiveURLForInstant(url, profile);
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)
-  // If |url| has an isolated origin, don't resolve effective URLs
-  // corresponding to extensions, since isolated origins should take precedence
-  // over hosted apps.  Note that for NTP, we do want to resolve the effective
-  // URL above; see https://crbug.com/755595.
-  if (is_isolated_origin)
-    return url;
-
   return ChromeContentBrowserClientExtensionsPart::GetEffectiveURL(
-      profile, url);
+      profile, url, is_isolated_origin);
 #else
   return url;
 #endif

chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc

@@ -263,7 +263,9 @@ ChromeContentBrowserClientExtensionsPart::
 
 // static
 GURL ChromeContentBrowserClientExtensionsPart::GetEffectiveURL(
-    Profile* profile, const GURL& url) {
+    Profile* profile,
+    const GURL& url,
+    bool is_isolated_origin) {
   // If the input |url| is part of an installed app, the effective URL is an
   // extension URL with the ID of that extension as the host. This has the
   // effect of grouping apps together in a common SiteInstance.
@@ -281,6 +283,13 @@ GURL ChromeContentBrowserClientExtensionsPart::GetEffectiveURL(
   if (extension->from_bookmark())
     return url;
 
+  // If |url| corresponds to an isolated origin, don't resolve effective URLs,
+  // since isolated origins should take precedence over hosted apps.  One
+  // exception is a URL for Chrome Web Store, which should always be resolved
+  // to its effective URL, so that the CWS process gets proper bindings.
+  if (is_isolated_origin && extension->id() != kWebStoreAppId)
+    return url;
+
   // If the URL is part of an extension's web extent, convert it to an
   // extension URL.
   return extension->GetResourceURL(url.path());

chrome/browser/extensions/chrome_content_browser_client_extensions_part.h

@@ -26,7 +26,9 @@ class ChromeContentBrowserClientExtensionsPart
   ~ChromeContentBrowserClientExtensionsPart() override;
 
   // Corresponds to the ChromeContentBrowserClient function of the same name.
-  static GURL GetEffectiveURL(Profile* profile, const GURL& url);
+  static GURL GetEffectiveURL(Profile* profile,
+                              const GURL& url,
+                              bool is_isolated_origin);
   static bool ShouldUseProcessPerSite(Profile* profile,
                                       const GURL& effective_url);
   static bool DoesSiteRequireDedicatedProcess(

chrome/browser/extensions/process_management_browsertest.cc

@@ -24,6 +24,7 @@
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/site_instance.h"
 #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 "extensions/browser/extension_host.h"
@@ -60,7 +61,6 @@ class ChromeWebStoreProcessTest : public ExtensionBrowserTest {
  public:
   const GURL& gallery_url() { return gallery_url_; }
 
- private:
   // Overrides location of Chrome Web Store gallery to a test controlled URL.
   void SetUpCommandLine(base::CommandLine* command_line) override {
     ExtensionBrowserTest::SetUpCommandLine(command_line);
@@ -72,6 +72,7 @@ class ChromeWebStoreProcessTest : public ExtensionBrowserTest {
                                     gallery_url_.spec());
   }
 
+ private:
   void SetUpOnMainThread() override {
     host_resolver()->AddRule("*", "127.0.0.1");
   }
@@ -79,6 +80,22 @@ class ChromeWebStoreProcessTest : public ExtensionBrowserTest {
   GURL gallery_url_;
 };
 
+class ChromeWebStoreInIsolatedOriginTest : public ChromeWebStoreProcessTest {
+ public:
+  ChromeWebStoreInIsolatedOriginTest() {}
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    ChromeWebStoreProcessTest::SetUpCommandLine(command_line);
+
+    // Mark the Chrome Web Store URL as an isolated origin.
+    command_line->AppendSwitchASCII(::switches::kIsolateOrigins,
+                                    gallery_url().spec());
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ChromeWebStoreInIsolatedOriginTest);
+};
+
 }  // namespace
 
 
@@ -415,6 +432,31 @@ IN_PROC_BROWSER_TEST_F(ChromeWebStoreProcessTest,
   EXPECT_NE(old_process_host, new_process_host);
 }
 
+// Check that navigations to the Chrome Web Store succeed when the Chrome Web
+// Store URL's origin is set as an isolated origin via the
+// --isolate-origins flag.  See https://crbug.com/788837.
+IN_PROC_BROWSER_TEST_F(ChromeWebStoreInIsolatedOriginTest,
+                       NavigationLoadsChromeWebStore) {
+  // Sanity check that a SiteInstance for a Chrome Web Store URL requires a
+  // dedicated process.
+  content::BrowserContext* context = browser()->profile();
+  scoped_refptr<content::SiteInstance> cws_site_instance =
+      content::SiteInstance::CreateForURL(context, gallery_url());
+  EXPECT_TRUE(cws_site_instance->RequiresDedicatedProcess());
+
+  // Navigate to Chrome Web Store and check that it's loaded successfully.
+  ui_test_utils::NavigateToURL(browser(), gallery_url());
+  WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  EXPECT_EQ(gallery_url(), web_contents->GetLastCommittedURL());
+
+  // Verify that the Chrome Web Store hosted app is really loaded.
+  content::RenderProcessHost* render_process_host =
+      web_contents->GetMainFrame()->GetProcess();
+  EXPECT_TRUE(extensions::ProcessMap::Get(profile())->Contains(
+      extensions::kWebStoreAppId, render_process_host->GetID()));
+}
+
 // This test verifies that blocked navigations to extensions pages do not
 // overwrite process-per-site map inside content/.
 IN_PROC_BROWSER_TEST_F(ProcessManagementTest,