Disable top-document-isolation if the parent SiteInstance is a hosted app.

chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/chrome_content_browser_client_extensions_part.h"
 
 #include <stddef.h>
 
 #include <set>
 
@@ skipping 667 matching lines @@
       chromeos::VpnServiceFactory::GetForBrowserContext(browser_context);
   if (!vpn_service)
     return nullptr;
   return vpn_service->GetVpnServiceProxy();
 #else
   return nullptr;
 #endif
 }
 
 // static
+bool ChromeContentBrowserClientExtensionsPart::
+    ShouldFrameShareParentSiteInstanceDespiteTopDocumentIsolation(
+        const GURL& subframe_url,
+        content::SiteInstance* parent_site_instance) {
+  const Extension* extension =
+      ExtensionRegistry::Get(parent_site_instance->GetBrowserContext())
+          ->enabled_extensions()
+          .GetExtensionOrAppByURL(parent_site_instance->GetSiteURL());

[Devlin, 2017/05/03 22:54:26]

Why do we do this unequivocally, rather than checking if it's covered by
app.urls?  Do we not want to isolate hosted apps at all?

[Łukasz Anforowicz, 2017/05/03 23:22:21]

parent_site_instance->GetSiteURL() is an *effective* URL.  For example if hosted
app's extent covers docs.google.com and the parent frame is rendering
docs.google.com, then parent_site_instance->GetSiteURL() will be
chrome-extension://apdfllckaahabafndbhieahigkjlhalf/ (rather than
https://docs.google.com).

Do you think changing the code here might make the above more obvious?  Maybe
the code could just check if
1) parent_site_instance->GetSiteURL().scheme() is chrome-extension
2) enabled_extensions() contains (GetByID)
parent_site_instance->GetSiteURL().host()

WDYT?
nick@ should probably double-check, but my current understanding is that: yes -
we do not want to isolate hosted apps at all.  Such isolation is not needed -
hosted apps are just web pages + manifest.  We've reviewed (known to us)
features of hosted apps and only the background *content* (not *page*) is
something that requires special process allocation.  Everything else can be just
put into regular renderer process alongside with other web content.  In
particular, creis@ gathered special features granted to hosted apps and they
weren't particularly sensitive (and AFAIR all of them were obtainable by web
pages).

Also - for completeness:
1. Before and after this CL the behavior doesn't change wrt:
    1.a. We don't isolate hosted apps with --isolate-extensions (i.e. hosted
apps share renderers with web pages)
    1.b. still isolate hosted apps under --site-per-process.
2. Note that the function here applies only to --top-document-isolation.  Before
this CL --top-document-isolation would kick out client5.google.com to a renderer
separate from the docs.google.com process.

Does that make sense?  WDYT?

[Charlie Reis, 2017/05/03 23:37:36]

Yeah, there's two aspects here:

1) For TDI, we're punting on cross-site iframes within hosted apps and leaving
them all in process.
You could imagine putting cross-*site* iframes (i.e., different domain or
scheme) into the TDI subframe process and just leaving non-hosted-app same-site
iframes (e.g., client5.google.com) in the process.  I think we skipped that
mainly due to complexity?

2) In general, I would *love* to stop giving hosted apps any special behavior in
the process model at all, kind of like bookmark apps.  I've been collecting
notes as Lukasz mentions, and we should chat about ideas for that.

[Łukasz Anforowicz, 2017/05/04 16:17:44]

BTW: I should note that GetExtensionOrAppByURL is already used in other methods
of ChromeContentBrowserClientExtensionsPart when the argument is an effective
site URL (i.e. when the argument has already gone through web extent ->
chrome-extension transformation).  Examples:
- ChromeContentBrowserClientExtensionsPart::DoesSiteRequireDedicatedProcess
- ChromeContentBrowserClientExtensionsPart::ShouldLockToOrigin
- ChromeContentBrowserClientExtensionsPart::OverrideNavigationParams
- ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL
- ChromeContentBrowserClientExtensionsPart::SiteInstanceGotProcess
- ChromeContentBrowserClientExtensionsPart::SiteInstanceDeleting

Therefore, I think it is okay to keep GetExtensionOrAppByURL above.
Correct.

Ideally we would:
1) keep web.site.com in the same process as hosted-app.site.com (which is what
this CL fixes for --top-document-isolation)
2) keep hosted-app.site.com in a separate process from
another.different-site.com (not done by this CL)

We've considered implementing (2) by A) adding bool URLPattern::IsSameSite(GURL)
or B) making SiteInstance::IsSameWebSite skip GetEffectiveURL in some cases). 
We've decided that the extra complexity of both A and B is not worth it 1)
before we fully commit to TDI beyond dev/canary finch trials and 2) before we
investigate whether we can address the issue by removing any special process
treatment for hosted apps.

Additionally, as mentioned earlier, (2) [i.e. isolating hosted apps from other
web content] is not that important - such isolation is not done today by
--isolate-extensions, so there is no pressure to have such isolation in
--top-document-isolation.

[Devlin, 2017/05/04 17:20:45]

Sorry that this was unclear.  The call to GetExtensionOrAppByURL() is fine for
looking up the app.  I was just saying why don't we first check if the subframe
url is covered by app.urls (or is the launch url) rather than *just* checking if
it's a hosted app.  That is, this was only one comment: "Do we want to isolate
cross-site iframes of hosted apps that aren't included in the app.urls" (which
is answered below).
I'm not quite sure I understand some of the points here (For instance,
"isolating hosted apps from other web content] is not that important - such
isolation is not done today by --isolate-extensions, so there is no pressure to
have such isolation in --top-document-isolation."  I don't see the correlation
with isolate-extensions.  No web content is isolated either with
isolate-extensions, but we want it with TDI.  My question is why are we saying
hosted apps have *less* isolation than a normal web page - that seems wrong.) so
having a future discussion is probably in order.

That said, I agree that this doesn't make anything any worse than it is now, so
having this as an incremental step is okay (but I would like to revisit it in
the future).

[Łukasz Anforowicz, 2017/05/04 18:58:19]

Agreed - with this CL, hosted apps have *less* isolation than a normal web page.
 I guess we are still not entirely clear on what the --top-document-isolation
mode should isolate, but we believe that unblocking further experiments requires
that docs.google.com and client5.google.com should be in the same process (this
is what this CL does).  If results of the experiments reinforce the desire to
invest more into --top-document-isolation's finer details, then we'll have to
revisit the decision what to isolate in the mode (and in particular whether
truly cross-site content in hosted apps should be subject to
--top-document-isolation).
Agreed.

--site-per-process will likely remain a test-only mode for foreseeable future
and
--top-document-isolation boundary has only performance implications
but
we need to decide how to treat hosted apps for other isolation modes slated for
launch later this year (e.g. treatment of hosted apps will have implications for
the security boundaries/model enforced by "Isolate Subset of Sites" that targets
Q4 of 2017).

+
+  return extension && extension->is_hosted_app();
+}
+
+// static
 void ChromeContentBrowserClientExtensionsPart::RecordShouldAllowOpenURLFailure(
     ShouldAllowOpenURLFailureReason reason,
     const GURL& site_url) {
   UMA_HISTOGRAM_ENUMERATION("Extensions.ShouldAllowOpenURL.Failure", reason,
                             FAILURE_LAST);
 
   // Must be kept in sync with the ShouldAllowOpenURLFailureScheme enum.
   static const char* const kSchemeNames[] = {
       "unknown",
       "",
@@ skipping 173 matching lines @@
     command_line->AppendSwitch(switches::kExtensionProcess);
   }
 }
 
 void ChromeContentBrowserClientExtensionsPart::ResourceDispatcherHostCreated() {
   content::ResourceDispatcherHost::Get()->RegisterInterceptor(
       "Origin", kExtensionScheme, base::Bind(&OnHttpHeaderReceived));
 }
 
 }  // namespace extensions