Isolating Chrome Web Store app in the transfer (i.e. non-OpenURL) path.

chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc

Index: chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
diff --git a/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc b/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
index ca0879d192fed96008233ff83eb019cbf965ead5..9f61513cc4d3f95a71c42d71b169184b06dc6ed2 100644
--- a/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
+++ b/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
@@ -268,24 +268,30 @@ bool ChromeContentBrowserClientExtensionsPart::ShouldUseProcessPerSite(
 bool ChromeContentBrowserClientExtensionsPart::DoesSiteRequireDedicatedProcess(
     content::BrowserContext* browser_context,
     const GURL& effective_site_url) {
-  if (effective_site_url.SchemeIs(extensions::kExtensionScheme)) {

[Łukasz Anforowicz, 2016/10/05 22:34:55]

The check for extensions scheme is not necessary here - it is sufficient to see
if GetExtensionOrAppByURL returns an extension.  Furthermore,
GetExtensionOrAppByURL can return an extension for some web URLs (in case of
Chrome Web Store gallery URLs + in case of urls handled by "url handlers"
registered in an app manifest [1,2]).

Because of the above, it seems best to just remove the outer-most "if" statement
here.

[1] https://developer.chrome.com/apps/manifest/url_handlers
[2]
https://chromium.googlesource.com/chromium/src/+/3c19074450571b0fdd16063353e7...

-    // --isolate-extensions should isolate extensions, except for a) hosted
-    // apps, b) platform apps.
-    // a) Isolating hosted apps is a good idea, but ought to be a separate knob.
-    // b) Sandbox pages in platform app can load web content in iframes;
-    //   isolating the app and the iframe leads to StoragePartition mismatch in
-    //   the two processes.
-    //   TODO(lazyboy): We should deprecate this behaviour and not let web
-    //   content load in platform app's process; see http://crbug.com/615585.
-    if (IsIsolateExtensionsEnabled()) {
-      const Extension* extension =
-          ExtensionRegistry::Get(browser_context)
-              ->enabled_extensions()
-              .GetExtensionOrAppByURL(effective_site_url);
-      if (extension && !extension->is_hosted_app() &&
-          !extension->is_platform_app()) {
+  if (IsIsolateExtensionsEnabled()) {
+    const Extension* extension =
+        ExtensionRegistry::Get(browser_context)
+            ->enabled_extensions()
+            .GetExtensionOrAppByURL(effective_site_url);
+    if (extension) {

[Łukasz Anforowicz, 2016/10/05 22:34:56]

Below I am replicating
-      if (extension && !extension->is_hosted_app() &&
-          !extension->is_platform_app()) {
         return true;
-      }
from the old code, but
1) making an exception for Chrome Web Store (to always isolate)
and
2) splitting the single, complex decision into multiple separate, simple
decisions (one for web store, then for the hosted apps exception, then returning
true for all other extensions).

+      // Always isolate Chrome Web Store.
+      if (extension->id() == kWebStoreAppId)
         return true;
-      }
+
+      // --isolate-extensions should isolate extensions, except for a) hosted
+      // apps, b) platform apps.
+      // a) Isolating hosted apps is a good idea, but ought to be a separate
+      //    knob.
+      // b) Sandbox pages in platform app can load web content in iframes;
+      //    isolating the app and the iframe leads to StoragePartition mismatch
+      //    in the two processes.
+      //    TODO(lazyboy): We should deprecate this behaviour and not let web
+      //    content load in platform app's process; see http://crbug.com/615585.
+      if (extension->is_hosted_app() || extension->is_platform_app())

[Łukasz Anforowicz, 2016/10/05 22:34:56]

Probably best to preserve the old behavior above (which is what the CL does
above), but I wondered whether we could simply use |if (extension->is_app())
here?  Definition this would go into is quoted below - as you can see it also
covers "legacy packaged apps" (whatever they are :-).

extensions/common/manifest.h:
  ...
  bool is_app() const {
    return is_legacy_packaged_app() || is_hosted_app() || is_platform_app();
  }

[lazyboy, 2016/10/05 23:20:02]

Given that we have separate plans for dealing with hosted apps and platform apps
(#a, #b above), I think it's better to keep it as is.

+        return false;
+
+      // Isolate all extensions.
+      return true;
     }
   }
   return false;