Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2840383002:
Disable top-document-isolation if the parent SiteInstance is a hosted app. (Closed)
|
Created:
3 years, 7 months ago by Łukasz Anforowicz Modified:
3 years, 7 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, Charlie Reis, site-isolation-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisable top-document-isolation if the parent SiteInstance is a hosted app.
BUG=679011
Review-Url: https://codereview.chromium.org/2840383002
Cr-Commit-Position: refs/heads/master@{#469659}
Committed: https://chromium.googlesource.com/chromium/src/+/0311d114732b87f41f80d839c0170cd68c364c10
Patch Set 1 #Patch Set 2 : Added a test. #
Total comments: 3
Patch Set 3 : Relax test verifications when running with --site-per-process. #Patch Set 4 : Updated comments and test expectations for --site-per-process behavior. #
Total comments: 14
Patch Set 5 : Addressing CR feedback. #
Messages
Total messages: 41 (26 generated)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
lukasza@chromium.org changed reviewers: + nick@chromium.org
nick@, can you PTAL? https://codereview.chromium.org/2840383002/diff/20001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/hosted_app_browsertest.cc (right): https://codereview.chromium.org/2840383002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/hosted_app_browsertest.cc:279: }; I guess the above might need to be refactored depending on where we go with https://codereview.chromium.org/2849603005.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2840383002/diff/20001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2840383002/diff/20001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1457: url, parent_site_instance); (nit/optional) Might be better if factored like this: #if BUILDFLAG(ENABLE_EXTENSIONS) if (ChromeContentBrowserClientExtensionsPart:: ShouldFrameShareParentSiteInstanceDespiteTopDocumentIsolation( url, parent_site_instance)) return true; #endif return false; https://codereview.chromium.org/2840383002/diff/20001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/hosted_app_browsertest.cc (right): https://codereview.chromium.org/2840383002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/hosted_app_browsertest.cc:269: url.spec().c_str())); You could also use C++11 raw string literals here: test_app_dir.WriteManifestWithSingleQuotes(base::StringPrintf( R"({ 'name': 'Hosted App vs TDI Test', 'version': '1', 'manifest_version': 2, 'app': { 'launch': { 'web_url': '%s' }, 'urls': ['*://app.site.com/frame_tree'] } })", url.spec().c_str())); (technically you could use the non-singlequote version too, but I think it's easier to read with singlequotes, and works better with syntax highlighters that aren't hip to C++11 string literals)
Hmmm... I think I've jumped the gun here - the new test I am adding fails with --site-per-process (see https://crbug.com679011#c5) so the product code change in this CL are at least insufficient (and maybe should be replaced altogether with something affecting broader RFHM functionality).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/04/28 18:50:34, Łukasz A. wrote: > Hmmm... I think I've jumped the gun here - the new test I am adding fails with > --site-per-process (see https://crbug.com679011#c5) so the product code change > in this CL are at least insufficient (and maybe should be replaced altogether > with something affecting broader RFHM functionality). Your test is exploring an interesting area: how does --site-per-process apply to URLs that are same-site with a hosted app, but outside of its web extent, when in the same browsinginstance as the hosted app? It may not be necessary to ever solve that, if hosted app deprecation happens sufficiently soon. For now I would just update the test expectations to document the existing behavior, and leave a comment in the test code explaining what's happening.
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
On 2017/04/28 20:34:37, ncarter wrote: > On 2017/04/28 18:50:34, Łukasz A. wrote: > > Hmmm... I think I've jumped the gun here - the new test I am adding fails with > > --site-per-process (see https://crbug.com679011#c5) so the product code change > > in this CL are at least insufficient (and maybe should be replaced altogether > > with something affecting broader RFHM functionality). > > Your test is exploring an interesting area: how does --site-per-process apply > to URLs that are same-site with a hosted app, but outside of its web extent, > when in the same browsinginstance as the hosted app? > > It may not be necessary to ever solve that, if hosted app deprecation happens > sufficiently soon. For now I would just update the test expectations to > document the existing behavior, and leave a comment in the test code > explaining what's happening. Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lukasza@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
rdevlin.cronin@, can you PTAL?
https://codereview.chromium.org/2840383002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2840383002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:695: .GetExtensionOrAppByURL(parent_site_instance->GetSiteURL()); 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?
https://codereview.chromium.org/2840383002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2840383002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:695: .GetExtensionOrAppByURL(parent_site_instance->GetSiteURL()); On 2017/05/03 22:54:26, Devlin wrote: > Why do we do this unequivocally, rather than checking if it's covered by > app.urls? 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? > Do we not want to isolate hosted apps at all? 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?
https://codereview.chromium.org/2840383002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2840383002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:695: .GetExtensionOrAppByURL(parent_site_instance->GetSiteURL()); On 2017/05/03 23:22:21, Łukasz A. wrote: > On 2017/05/03 22:54:26, Devlin wrote: > > Why do we do this unequivocally, rather than checking if it's covered by > > app.urls? > > parent_site_instance->GetSiteURL() is an *effective* URL. For example if hosted > app's extent covers http://docs.google.com and the parent frame is rendering > http://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? > > > Do we not want to isolate hosted apps at all? > > 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 http://client5.google.com to a renderer > separate from the http://docs.google.com process. > > Does that make sense? WDYT? 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2840383002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2840383002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:695: .GetExtensionOrAppByURL(parent_site_instance->GetSiteURL()); On 2017/05/03 23:37:36, Charlie Reis wrote: > On 2017/05/03 23:22:21, Łukasz A. wrote: > > On 2017/05/03 22:54:26, Devlin wrote: > > > Why do we do this unequivocally, rather than checking if it's covered by > > > app.urls? > > > > parent_site_instance->GetSiteURL() is an *effective* URL. For example if > hosted > > app's extent covers http://docs.google.com and the parent frame is rendering > > http://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? 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. > > > > > Do we not want to isolate hosted apps at all? > > > > 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 http://client5.google.com to a > renderer > > separate from the http://docs.google.com process. > > > > Does that make sense? WDYT? > > 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., http://client5.google.com) in the process. I think we skipped that > mainly due to complexity? 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. > > 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.
https://codereview.chromium.org/2840383002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2840383002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:695: .GetExtensionOrAppByURL(parent_site_instance->GetSiteURL()); On 2017/05/04 16:17:44, Łukasz A. wrote: > On 2017/05/03 23:37:36, Charlie Reis wrote: > > On 2017/05/03 23:22:21, Łukasz A. wrote: > > > On 2017/05/03 22:54:26, Devlin wrote: > > > > Why do we do this unequivocally, rather than checking if it's covered by > > > > app.urls? > > > > > > parent_site_instance->GetSiteURL() is an *effective* URL. For example if > > hosted > > > app's extent covers http://docs.google.com and the parent frame is rendering > > > http://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? > > 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. 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). > > > > > > > Do we not want to isolate hosted apps at all? > > > > > > 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 http://client5.google.com to > a > > renderer > > > separate from the http://docs.google.com process. > > > > > > Does that make sense? WDYT? > > > > 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., http://client5.google.com) in the process. I think we skipped > that > > mainly due to complexity? > > Correct. > > Ideally we would: > 1) keep http://web.site.com in the same process as http://hosted-app.site.com (which is what > this CL fixes for --top-document-isolation) > 2) keep http://hosted-app.site.com in a separate process from > http://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. > > > > > 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. 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). https://codereview.chromium.org/2840383002/diff/60001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/hosted_app_browsertest.cc (right): https://codereview.chromium.org/2840383002/diff/60001/chrome/browser/ui/exten... chrome/browser/ui/extensions/hosted_app_browsertest.cc:251: // style nit: I don't think these warrant // // Three-line-comments // <code> Maybe just // Do foo <code> https://codereview.chromium.org/2840383002/diff/60001/chrome/browser/ui/exten... chrome/browser/ui/extensions/hosted_app_browsertest.cc:259: " 'name': 'Hosted App vs TDI Test',\n" Optional: Now that we can use raw string literals, it's sometimes cleaner to do that (and the SingleQuotes method is a little less useful). e.g., base::StringPrintf( R"( ... )" It avoids the need for the repeated \ns. https://codereview.chromium.org/2840383002/diff/60001/chrome/browser/ui/exten... chrome/browser/ui/extensions/hosted_app_browsertest.cc:353: // https://crbug.com/679011#c2. I think we should add a TODO to address this. https://codereview.chromium.org/2840383002/diff/60001/chrome/browser/ui/exten... chrome/browser/ui/extensions/hosted_app_browsertest.cc:360: // synchronously script |diff_dir| and/or |same_site|). Should we try to fix that?
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2840383002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2840383002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:695: .GetExtensionOrAppByURL(parent_site_instance->GetSiteURL()); On 2017/05/04 17:20:45, Devlin wrote: > On 2017/05/04 16:17:44, Łukasz A. wrote: > > On 2017/05/03 23:37:36, Charlie Reis wrote: > > > On 2017/05/03 23:22:21, Łukasz A. wrote: > > > > On 2017/05/03 22:54:26, Devlin wrote: > > > > > Why do we do this unequivocally, rather than checking if it's covered by > > > > > app.urls? > > > > > > > > parent_site_instance->GetSiteURL() is an *effective* URL. For example if > > > hosted > > > > app's extent covers http://docs.google.com and the parent frame is > rendering > > > > http://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? > > > > 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. > > 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). > > > > > > > > > > Do we not want to isolate hosted apps at all? > > > > > > > > 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 http://client5.google.com > to > > a > > > renderer > > > > separate from the http://docs.google.com process. > > > > > > > > Does that make sense? WDYT? > > > > > > 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., http://client5.google.com) in the process. I think we > skipped > > that > > > mainly due to complexity? > > > > Correct. > > > > Ideally we would: > > 1) keep http://web.site.com in the same process as http://hosted-app.site.com > (which is what > > this CL fixes for --top-document-isolation) > > 2) keep http://hosted-app.site.com in a separate process from > > http://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. > > > > > > > > 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. > > 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. 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). > 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). 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). https://codereview.chromium.org/2840383002/diff/60001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/hosted_app_browsertest.cc (right): https://codereview.chromium.org/2840383002/diff/60001/chrome/browser/ui/exten... chrome/browser/ui/extensions/hosted_app_browsertest.cc:251: // On 2017/05/04 17:20:45, Devlin wrote: > style nit: I don't think these warrant > // > // Three-line-comments > // > > <code> > > Maybe just > // Do foo > <code> Done. I did the header-style comments, because I wasn't sure if it is obvious that the comment applies to all the statements until the next comment (not just to the first blank line). https://codereview.chromium.org/2840383002/diff/60001/chrome/browser/ui/exten... chrome/browser/ui/extensions/hosted_app_browsertest.cc:259: " 'name': 'Hosted App vs TDI Test',\n" On 2017/05/04 17:20:45, Devlin wrote: > Optional: Now that we can use raw string literals, it's sometimes cleaner to do > that (and the SingleQuotes method is a little less useful). e.g., > > base::StringPrintf( > R"( > ... > )" > > It avoids the need for the repeated \ns. Done. Also - what I have in the latest patchset was formatted manually. Raw strings are (understandably I guess) problematic for clang-format - I've opened https://bugs.llvm.org/show_bug.cgi?id=32928 https://codereview.chromium.org/2840383002/diff/60001/chrome/browser/ui/exten... chrome/browser/ui/extensions/hosted_app_browsertest.cc:353: // https://crbug.com/679011#c2. On 2017/05/04 17:20:45, Devlin wrote: > I think we should add a TODO to address this. Done (in the TODO, I am referring to the newly opened https://crbug.com/718516). https://codereview.chromium.org/2840383002/diff/60001/chrome/browser/ui/exten... chrome/browser/ui/extensions/hosted_app_browsertest.cc:360: // synchronously script |diff_dir| and/or |same_site|). On 2017/05/04 17:20:45, Devlin wrote: > Should we try to fix that? Yes, although the fix is not urgent given that --site-per-process is a test-only mode and the issue doesn't affect --isolate-extensions and (after this CL) --top-document-isolation. I've added a reference to https://crbug.com/718516 in the comment.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm. Thanks for filing the extra bugs!
The CQ bit was checked by lukasza@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org Link to the patchset: https://codereview.chromium.org/2840383002/#ps80001 (title: "Addressing CR feedback.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493998469252100,
"parent_rev": "3a69e4860efab03bba98d13bbcf4883fae7a3bc0", "commit_rev":
"0311d114732b87f41f80d839c0170cd68c364c10"}
Message was sent while issue was closed.
Description was changed from ========== Disable top-document-isolation if the parent SiteInstance is a hosted app. BUG=679011 ========== to ========== Disable top-document-isolation if the parent SiteInstance is a hosted app. BUG=679011 Review-Url: https://codereview.chromium.org/2840383002 Cr-Commit-Position: refs/heads/master@{#469659} Committed: https://chromium.googlesource.com/chromium/src/+/0311d114732b87f41f80d839c017... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/0311d114732b87f41f80d839c017... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
