Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2831683002:
Introduce support for origins that require process isolation. (Closed)
|
Created:
3 years, 8 months ago by alexmos Modified:
3 years, 7 months ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, ajwong+watch_chromium.org, site-isolation-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce support for origins that require process isolation.
This CL changes logic in SiteInstance to support isolating specific
origins based on the full scheme+host+port tuple rather than just
scheme and eTLD+1. The global list of such origins is maintained by
SiteInstanceImpl and currently not exposed outside of content.
This CL also adds a command-line option to add isolated origins.
Sample usage:
--isolate-origins=https://isolated.foo.com,https://bar.com
Design doc: https://goo.gl/99ynqK
BUG=713444
Review-Url: https://codereview.chromium.org/2831683002
Cr-Commit-Position: refs/heads/master@{#475191}
Committed: https://chromium.googlesource.com/chromium/src/+/3b9ad10d35e7cb293f559d262bf417d7aa099f4d
Patch Set 1 #Patch Set 2 : Fix unittest #Patch Set 3 : Add command-line flag, which should fix Android #Patch Set 4 : nit #Patch Set 5 : Update comments #
Total comments: 2
Patch Set 6 : Rebase #
Total comments: 28
Patch Set 7 : Address Charlie's comments #Patch Set 8 : Rebase #
Total comments: 23
Patch Set 9 : Charlie's comments, round 2 #Patch Set 10 : Rebase #Patch Set 11 : Fix compile #
Total comments: 4
Patch Set 12 : Charlie's comments (round 3) #Messages
Total messages: 75 (59 generated)
The CQ bit was checked by alexmos@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== Introduce a whitelist of origins that require process isolation. BUG= ========== to ========== Introduce support for origins that require site isolation. This CL changes logic in SiteInstance to support isolating specific origins based on the full scheme+host+port tuple rather than eTLD+1. The global list of such origins is maintained by SiteInstanceImpl and currently not exposed outside of content. Future CLs will introduce ways of adding origins to this list, such as through a command-line flag. BUG=713444 ==========
The CQ bit was checked by alexmos@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...
Description was changed from ========== Introduce support for origins that require site isolation. This CL changes logic in SiteInstance to support isolating specific origins based on the full scheme+host+port tuple rather than eTLD+1. The global list of such origins is maintained by SiteInstanceImpl and currently not exposed outside of content. Future CLs will introduce ways of adding origins to this list, such as through a command-line flag. BUG=713444 ========== to ========== Introduce support for origins that require site isolation. This CL changes logic in SiteInstance to support isolating specific origins based on the full scheme+host+port tuple rather than eTLD+1. The global list of such origins is maintained by SiteInstanceImpl and currently not exposed outside of content. Future CLs will introduce ways of adding origins to this list, such as through a command-line flag. BUG=713444 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by alexmos@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...
Description was changed from ========== Introduce support for origins that require site isolation. This CL changes logic in SiteInstance to support isolating specific origins based on the full scheme+host+port tuple rather than eTLD+1. The global list of such origins is maintained by SiteInstanceImpl and currently not exposed outside of content. Future CLs will introduce ways of adding origins to this list, such as through a command-line flag. BUG=713444 ========== to ========== Introduce support for origins that require process isolation. This CL changes logic in SiteInstance to support isolating specific origins based on the full scheme+host+port tuple rather than eTLD+1. The global list of such origins is maintained by SiteInstanceImpl and currently not exposed outside of content. Future CLs will introduce ways of adding origins to this list, such as through a command-line flag. BUG=713444 ==========
Description was changed from ========== Introduce support for origins that require process isolation. This CL changes logic in SiteInstance to support isolating specific origins based on the full scheme+host+port tuple rather than eTLD+1. The global list of such origins is maintained by SiteInstanceImpl and currently not exposed outside of content. Future CLs will introduce ways of adding origins to this list, such as through a command-line flag. BUG=713444 ========== to ========== Introduce support for origins that require process isolation. This CL changes logic in SiteInstance to support isolating specific origins based on the full scheme+host+port tuple rather than eTLD+1. The global list of such origins is maintained by SiteInstanceImpl and currently not exposed outside of content. Future CLs will introduce ways of adding origins to this list, such as through a command-line flag. BUG=713444 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexmos@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.
Description was changed from ========== Introduce support for origins that require process isolation. This CL changes logic in SiteInstance to support isolating specific origins based on the full scheme+host+port tuple rather than eTLD+1. The global list of such origins is maintained by SiteInstanceImpl and currently not exposed outside of content. Future CLs will introduce ways of adding origins to this list, such as through a command-line flag. BUG=713444 ========== to ========== Introduce support for origins that require process isolation. This CL changes logic in SiteInstance to support isolating specific origins based on the full scheme+host+port tuple rather than eTLD+1. The global list of such origins is maintained by SiteInstanceImpl and currently not exposed outside of content. This CL also adds a command-line option to add isolated origins. Sample usage: --isolate-origins=https://isolated.foo.com,https://bar.com BUG=713444 ==========
The CQ bit was checked by alexmos@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 alexmos@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...
Description was changed from ========== Introduce support for origins that require process isolation. This CL changes logic in SiteInstance to support isolating specific origins based on the full scheme+host+port tuple rather than eTLD+1. The global list of such origins is maintained by SiteInstanceImpl and currently not exposed outside of content. This CL also adds a command-line option to add isolated origins. Sample usage: --isolate-origins=https://isolated.foo.com,https://bar.com BUG=713444 ========== to ========== Introduce support for origins that require process isolation. This CL changes logic in SiteInstance to support isolating specific origins based on the full scheme+host+port tuple rather than eTLD+1. The global list of such origins is maintained by SiteInstanceImpl and currently not exposed outside of content. This CL also adds a command-line option to add isolated origins. Sample usage: --isolate-origins=https://isolated.foo.com,https://bar.com Design doc: https://goo.gl/99ynqK BUG=713444 ==========
alexmos@chromium.org changed reviewers: + creis@chromium.org
Charlie, can you please take a look? I also organized some of my notes on this into a short design doc, which I linked to from the description. Please feel free to comment on that as well. One thing that's not done here is process reuse (when at process limit, not reusing another process for an isolated origin, as well as not reusing an isolated origin's own process for other things). I put some notes in the design doc on that, but I'm inclined to fix that and add a test in a followup CL - let me know if that's ok. https://codereview.chromium.org/2831683002/diff/80001/content/browser/site_in... File content/browser/site_instance_impl.cc (right): https://codereview.chromium.org/2831683002/diff/80001/content/browser/site_in... content/browser/site_instance_impl.cc:418: // same origin. I discovered this when writing SiteInstanceTest.IsolatedOrigins (I initially had my check after the scheme check). I don't know if it's a problem in practice though, and for the regular cases as well: http://foo.com and blob:http://foo.com/{id} won't be considered same-site here, but I'll need to reason through what this might affect. At least this seems like it should fail closed, e.g., cause an unnecessary process transfer. https://codereview.chromium.org/2831683002/diff/80001/content/common/site_iso... File content/common/site_isolation_policy.cc (right): https://codereview.chromium.org/2831683002/diff/80001/content/common/site_iso... content/common/site_isolation_policy.cc:22: IsTopDocumentIsolationEnabled() || AreIsolatedOriginsEnabled() || I'm a bit sad about this, as I'd love to just enable AreCrossProcessFramesPossible for Android and then remove it everywhere, but it's not clear how soon we can do that. Even with hit testing fixed, we still have issue 690229 (and possibly others?), so it seems we'll need a workaround here in the short term. I ended up just adding the plumbing for the command-line switch to this CL, since it wasn't that much and we'll need to expose it anyway. But we'll need to come back to this if we bake accounts.google.com into a whitelist without a flag, or when the set of isolated origins starts to change dynamically (should we enable AreCrossProcessFramesPossible only when there's 1+ isolated origin, etc). Another problem is that the renderer currently also needs it for a couple of CHECKs, which is the reason I also have to include the new switch in PropagateBrowserCommandLineToRenderer.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I am very excited about this CL! There's still some interesting questions, but I'm glad we're tackling them and adding this per-origin support. One interesting thing I initially thought is that it would give us a way to have a subset of isolated sites as well, but I think I was wrong. Any attempt to list a site with this command line parameter would get treated as an origin, so it couldn't apply site-wide. I suppose we'll need a different way to specify a subset of sites? (Might be worth clarifying that so others don't make the same mistake I did.) https://codereview.chromium.org/2831683002/diff/100001/content/browser/isolat... File content/browser/isolated_origin_browsertest.cc (right): https://codereview.chromium.org/2831683002/diff/100001/content/browser/isolat... content/browser/isolated_origin_browsertest.cc:63: // Navigate to an isolated origin and ensure that this ends up in a new Let's clarify this is a browser-initiated navigation. https://codereview.chromium.org/2831683002/diff/100001/content/browser/isolat... content/browser/isolated_origin_browsertest.cc:77: // Navigate to www.foo.com. This should end up in the |popup|'s process. And that this is a renderer-initiated navigation. (Thanks for testing both!) I feel like we should probably test renderer-initiated from unisolated to isolated as well, just to be safe. https://codereview.chromium.org/2831683002/diff/100001/content/browser/isolat... content/browser/isolated_origin_browsertest.cc:94: EXPECT_EQ(web_contents()->GetSiteInstance(), isolated_instance); nit: Swap order (expected, actual) https://codereview.chromium.org/2831683002/diff/100001/content/browser/site_i... File content/browser/site_instance_impl.cc (right): https://codereview.chromium.org/2831683002/diff/100001/content/browser/site_i... content/browser/site_instance_impl.cc:410: url::Origin src_origin(src_url); Fun. src_url is an effective URL, so it may be a hosted app's chrome-extension:// equivalent. That means isolating an origin which is within a hosted app has no effect, which is probably not what we want. Then again, is it safe for isolated origins to always take precedence over GetEffectiveURL (which is also used for NTP)? I think so? Also worth checking if anyone converts to effective URL *before* passing into this method, just in case. https://codereview.chromium.org/2831683002/diff/100001/content/browser/site_i... content/browser/site_instance_impl.cc:500: void SiteInstanceImpl::AddIsolatedOrigin(const url::Origin& origin) { Might be worth putting a UI thread check in each of the static methods? https://codereview.chromium.org/2831683002/diff/100001/content/browser/site_i... File content/browser/site_instance_impl.h (right): https://codereview.chromium.org/2831683002/diff/100001/content/browser/site_i... content/browser/site_instance_impl.h:16: #include "content/public/browser/site_instance.h" I wonder if we should update the comments at the top of site_instance.h to talk about the fact that we can now track origins as well as sites. I think it's worth mentioning, but I suppose it's not part of the public API yet. I imagine we'll need to make AddIsolatedOrigin public at some point for the features that plan to use it, so maybe we can do it then. https://codereview.chromium.org/2831683002/diff/100001/content/browser/site_i... content/browser/site_instance_impl.h:129: // scheme+host+port tuple rather than eTLD+1 will be used. SiteInstances for Maybe clarify that the default is scheme + eTLD+1? https://codereview.chromium.org/2831683002/diff/100001/content/browser/site_i... content/browser/site_instance_impl.h:130: // these origins will also use the full origin as site URL. This is the first use of url::Origin in this class. Should we clarify some of the potential gotchas? A few that come to mind: - Can't use unique origins here (e.g., data) - What happens to a sandboxed iframe with this origin (but whose url::Origin is unique)? - Presumably about:blank pages within a url::Origin go into that process? There's probably more... :) https://codereview.chromium.org/2831683002/diff/100001/content/browser/site_i... content/browser/site_instance_impl.h:173: // when making process model decisions, rather than the origin's eTLD+1. Each Same nit about previous definition of site. https://codereview.chromium.org/2831683002/diff/100001/content/browser/site_i... File content/browser/site_instance_impl_unittest.cc (right): https://codereview.chromium.org/2831683002/diff/100001/content/browser/site_i... content/browser/site_instance_impl_unittest.cc:847: GURL isolated_bar_url("http://isolated.bar.com"); Let's add tests for bar.isolated.foo.com, too. https://codereview.chromium.org/2831683002/diff/100001/content/browser/site_i... content/browser/site_instance_impl_unittest.cc:850: SiteInstanceImpl::IsIsolatedOrigin(url::Origin(isolated_foo_url))); Maybe check that IsSameWebsite for foo_url and isolated_foo_url returns true before we add it as an isolated origin? https://codereview.chromium.org/2831683002/diff/100001/content/common/site_is... File content/common/site_isolation_policy.cc (right): https://codereview.chromium.org/2831683002/diff/100001/content/common/site_is... content/common/site_isolation_policy.cc:49: // via the command-line switch, which may not be true in the future. Remove Ooh, that's a problem for adding a public API (e.g., for sign-in). Do you think we can get AreCrossProcessFramesPossible enabled on Android in time? Or maybe we don't need the API on Android? https://codereview.chromium.org/2831683002/diff/100001/content/public/common/... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/2831683002/diff/100001/content/public/common/... content/public/common/content_switches.cc:600: // Enable process isolation for a set of origins, specified as a s/Enable process isolation/Require dedicated processes/, perhaps?
The CQ bit was checked by alexmos@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...
Thanks for reviewing, Charlie! I think both this and the design doc are ready for a second look. Main changes are: - isolated origins moved into ChildProcessSecurityPolicyImpl, to allow access from the IO thread (for cookie checks). There are some gotchas to discuss with locking though (see below). - added a cookie access test from isolated origins, to verify that this doesn't lead to renderer kills - fixed hosted apps to not mess with process isolation for isolated origins, and added a new test to hosted_app_browsertest.cc - I added support for subdomains of isolated origins, but spun it off into a separate followup CL. > One interesting thing I initially thought is that it would give us a way to have > a subset of isolated sites as well, but I think I was wrong. Any attempt to > list a site with this command line parameter would get treated as an origin, so > it couldn't apply site-wide. I suppose we'll need a different way to specify a > subset of sites? (Might be worth clarifying that so others don't make the same > mistake I did.) If we keep subdomains of isolated origins in the same SiteInstance as the actual isolated origin, I *think* we can actually reuse this to get site-wide isolation -- if we use --isolate-origins=http://foo.com, that'll just do the right thing for bar.foo.com, keep doc.domain working within the site boundary, etc. I put some more discussion on this in the doc. One quirk we'd need to fix would be to ignore port for an isolated origin's site URL, which might be ok? https://codereview.chromium.org/2831683002/diff/100001/content/browser/isolat... File content/browser/isolated_origin_browsertest.cc (right): https://codereview.chromium.org/2831683002/diff/100001/content/browser/isolat... content/browser/isolated_origin_browsertest.cc:63: // Navigate to an isolated origin and ensure that this ends up in a new On 2017/05/05 23:18:51, Charlie Reis wrote: > Let's clarify this is a browser-initiated navigation. Done. https://codereview.chromium.org/2831683002/diff/100001/content/browser/isolat... content/browser/isolated_origin_browsertest.cc:77: // Navigate to www.foo.com. This should end up in the |popup|'s process. On 2017/05/05 23:18:51, Charlie Reis wrote: > And that this is a renderer-initiated navigation. (Thanks for testing both!) Done. > I feel like we should probably test renderer-initiated from unisolated to > isolated as well, just to be safe. Done. https://codereview.chromium.org/2831683002/diff/100001/content/browser/isolat... content/browser/isolated_origin_browsertest.cc:94: EXPECT_EQ(web_contents()->GetSiteInstance(), isolated_instance); On 2017/05/05 23:18:51, Charlie Reis wrote: > nit: Swap order (expected, actual) Done. https://codereview.chromium.org/2831683002/diff/100001/content/browser/site_i... File content/browser/site_instance_impl.cc (right): https://codereview.chromium.org/2831683002/diff/100001/content/browser/site_i... content/browser/site_instance_impl.cc:410: url::Origin src_origin(src_url); On 2017/05/05 23:18:51, Charlie Reis wrote: > Fun. src_url is an effective URL, so it may be a hosted app's > chrome-extension:// equivalent. That means isolating an origin which is within > a hosted app has no effect, which is probably not what we want. > > Then again, is it safe for isolated origins to always take precedence over > GetEffectiveURL (which is also used for NTP)? I think so? > > Also worth checking if anyone converts to effective URL *before* passing into > this method, just in case. Thanks for pointing this out. I agree that isolated origins should take precedence over hosted apps. I fixed the order so that isolated origins are checked before GetEffectiveURL, both here and in GetSiteForURL. Also added a section about this to the design doc and a test to hosted_app_browsertest.cc. Does seem like anyone converts to effective URLs before calling this. The only other call to GetEffectiveURL besides IsSameWebSite, GetSiteForURL, and tests, is in GetSiteInstanceForNavigation, where it converts two URLs fed into ShouldSwapBrowsingInstancesForNavigation. But that doesn't call into here or to GetSiteForURL. This should play well with NTP as long as we don't try to specify something like chrome-search://foo (or whatever else would conflict with NTP) as an isolated origin. An alternate fix would be to make SiteInstanceImpl::GetEffectiveURL always return |url| whenever it corresponds to an isolated origin (before forwarding to ChromeContentBrowserClient). That might be a more robust fix, as it's centralized and might be less vulnerable to incorrect ordering in future refactors. WDYT? https://codereview.chromium.org/2831683002/diff/100001/content/browser/site_i... content/browser/site_instance_impl.cc:500: void SiteInstanceImpl::AddIsolatedOrigin(const url::Origin& origin) { On 2017/05/05 23:18:51, Charlie Reis wrote: > Might be worth putting a UI thread check in each of the static methods? That was a really good idea, and it made me realize that this isn't really a valid assumption. :/ ChildProcessSecurityPolicy needs this data for cookie access checks on the IO thread via CanAccessDataForOrigin -> GetSiteForURL. Per out offline discussions, I moved this to ChildProcessSecurityPolicyImpl. https://codereview.chromium.org/2831683002/diff/100001/content/browser/site_i... File content/browser/site_instance_impl.h (right): https://codereview.chromium.org/2831683002/diff/100001/content/browser/site_i... content/browser/site_instance_impl.h:16: #include "content/public/browser/site_instance.h" On 2017/05/05 23:18:51, Charlie Reis wrote: > I wonder if we should update the comments at the top of site_instance.h to talk > about the fact that we can now track origins as well as sites. > > I think it's worth mentioning, but I suppose it's not part of the public API > yet. I imagine we'll need to make AddIsolatedOrigin public at some point for > the features that plan to use it, so maybe we can do it then. Yes, I've considered updating those and came to the same conclusion. I agree we can update the comments once we expose isolated origins from the public API. https://codereview.chromium.org/2831683002/diff/100001/content/browser/site_i... content/browser/site_instance_impl.h:129: // scheme+host+port tuple rather than eTLD+1 will be used. SiteInstances for On 2017/05/05 23:18:51, Charlie Reis wrote: > Maybe clarify that the default is scheme + eTLD+1? Yes, thanks, I was sloppy there. Updated here and in a couple of other places. https://codereview.chromium.org/2831683002/diff/100001/content/browser/site_i... content/browser/site_instance_impl.h:130: // these origins will also use the full origin as site URL. On 2017/05/05 23:18:51, Charlie Reis wrote: > This is the first use of url::Origin in this class. Should we clarify some of > the potential gotchas? A few that come to mind: > - Can't use unique origins here (e.g., data) > - What happens to a sandboxed iframe with this origin (but whose url::Origin is > unique)? > - Presumably about:blank pages within a url::Origin go into that process? > > There's probably more... :) Yes, good idea. This moved to ChildProcessSecurityPolicy, but I added some comments along those lines there. https://codereview.chromium.org/2831683002/diff/100001/content/browser/site_i... content/browser/site_instance_impl.h:173: // when making process model decisions, rather than the origin's eTLD+1. Each On 2017/05/05 23:18:51, Charlie Reis wrote: > Same nit about previous definition of site. Done. https://codereview.chromium.org/2831683002/diff/100001/content/browser/site_i... File content/browser/site_instance_impl_unittest.cc (right): https://codereview.chromium.org/2831683002/diff/100001/content/browser/site_i... content/browser/site_instance_impl_unittest.cc:847: GURL isolated_bar_url("http://isolated.bar.com"); On 2017/05/05 23:18:51, Charlie Reis wrote: > Let's add tests for http://bar.isolated.foo.com, too. I've built support for keeping those in isolated.foo.com, but that has quite a bit of plumbing/testing/open questions, so I decided to spin it off to a separate CL (https://codereview.chromium.org/2891443002, still needs some polishing). Does that sound ok? As of this CL, bar.isolated.foo.com will be kept in foo.com's SiteInstance. https://codereview.chromium.org/2831683002/diff/100001/content/browser/site_i... content/browser/site_instance_impl_unittest.cc:850: SiteInstanceImpl::IsIsolatedOrigin(url::Origin(isolated_foo_url))); On 2017/05/05 23:18:51, Charlie Reis wrote: > Maybe check that IsSameWebsite for foo_url and isolated_foo_url returns true > before we add it as an isolated origin? Done. https://codereview.chromium.org/2831683002/diff/100001/content/common/site_is... File content/common/site_isolation_policy.cc (right): https://codereview.chromium.org/2831683002/diff/100001/content/common/site_is... content/common/site_isolation_policy.cc:49: // via the command-line switch, which may not be true in the future. Remove On 2017/05/05 23:18:51, Charlie Reis wrote: > Ooh, that's a problem for adding a public API (e.g., for sign-in). > > Do you think we can get AreCrossProcessFramesPossible enabled on Android in > time? Or maybe we don't need the API on Android? Per offline email thread, the sign-in isolation will be initially needed for desktop only. So we don't need to support Android for now. That said, Ken said he isn't aware of any other outstanding issues other than improving Android test coverage and that perf regression, and we should keep pushing on those to enable this soon. https://codereview.chromium.org/2831683002/diff/100001/content/public/common/... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/2831683002/diff/100001/content/public/common/... content/public/common/content_switches.cc:600: // Enable process isolation for a set of origins, specified as a On 2017/05/05 23:18:52, Charlie Reis wrote: > s/Enable process isolation/Require dedicated processes/, perhaps? Done. https://codereview.chromium.org/2831683002/diff/140001/content/browser/child_... File content/browser/child_process_security_policy_impl.h (right): https://codereview.chromium.org/2831683002/diff/140001/content/browser/child_... content/browser/child_process_security_policy_impl.h:296: // IsIsolatedOrigin(). This cycle is nasty, and I'm not sure this is a good way to deal with it. :( I considered handling isolated origins separately before calling GetSiteForURL, but that didn't seem clean (it'd need to be done in CPSPI::CanAccessDataForOrigin, where isolated_origins_ are, and not on SecurityState, but it'd still need origin_lock_ to compare against which is on SecurityState). Or maybe we could move the isolated origin tracking into their own class after all, to avoid tying their state synchronization to the rest of CPSP. Let me know what you think.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Thanks! This is looking pretty good. That's a hard question about the extra lock, though-- let's see what we can come up with. https://codereview.chromium.org/2831683002/diff/100001/content/browser/site_i... File content/browser/site_instance_impl.cc (right): https://codereview.chromium.org/2831683002/diff/100001/content/browser/site_i... content/browser/site_instance_impl.cc:410: url::Origin src_origin(src_url); On 2017/05/16 17:26:37, alexmos (mostly OOO on 5-19) wrote: > On 2017/05/05 23:18:51, Charlie Reis wrote: > > Fun. src_url is an effective URL, so it may be a hosted app's > > chrome-extension:// equivalent. That means isolating an origin which is > within > > a hosted app has no effect, which is probably not what we want. > > > > Then again, is it safe for isolated origins to always take precedence over > > GetEffectiveURL (which is also used for NTP)? I think so? > > > > Also worth checking if anyone converts to effective URL *before* passing into > > this method, just in case. > > Thanks for pointing this out. I agree that isolated origins should take > precedence over hosted apps. I fixed the order so that isolated origins are > checked before GetEffectiveURL, both here and in GetSiteForURL. Also added a > section about this to the design doc and a test to hosted_app_browsertest.cc. > > Does seem like anyone converts to effective URLs before calling this. The only > other call to GetEffectiveURL besides IsSameWebSite, GetSiteForURL, and tests, > is in GetSiteInstanceForNavigation, where it converts two URLs fed into > ShouldSwapBrowsingInstancesForNavigation. But that doesn't call into here or to > GetSiteForURL. > > This should play well with NTP as long as we don't try to specify something like > chrome-search://foo (or whatever else would conflict with NTP) as an isolated > origin. > > An alternate fix would be to make SiteInstanceImpl::GetEffectiveURL always > return |url| whenever it corresponds to an isolated origin (before forwarding to > ChromeContentBrowserClient). That might be a more robust fix, as it's > centralized and might be less vulnerable to incorrect ordering in future > refactors. WDYT? I like that idea-- seems like it will help us be more consistent overall. https://codereview.chromium.org/2831683002/diff/100001/content/browser/site_i... File content/browser/site_instance_impl_unittest.cc (right): https://codereview.chromium.org/2831683002/diff/100001/content/browser/site_i... content/browser/site_instance_impl_unittest.cc:847: GURL isolated_bar_url("http://isolated.bar.com"); On 2017/05/16 17:26:38, alexmos (mostly OOO on 5-19) wrote: > On 2017/05/05 23:18:51, Charlie Reis wrote: > > Let's add tests for http://bar.isolated.foo.com, too. > > I've built support for keeping those in http://isolated.foo.com, but that has quite a > bit of plumbing/testing/open questions, so I decided to spin it off to a > separate CL (https://codereview.chromium.org/2891443002, still needs some > polishing). Does that sound ok? As of this CL, http://bar.isolated.foo.com will be > kept in foo.com's SiteInstance. Sounds good! https://codereview.chromium.org/2831683002/diff/140001/chrome/browser/ui/exte... File chrome/browser/ui/extensions/hosted_app_browsertest.cc (right): https://codereview.chromium.org/2831683002/diff/140001/chrome/browser/ui/exte... chrome/browser/ui/extensions/hosted_app_browsertest.cc:393: "urls": ["*://isolated.foo.com/"] Do we need to include app.foo.com in here as well? https://codereview.chromium.org/2831683002/diff/140001/chrome/browser/ui/exte... chrome/browser/ui/extensions/hosted_app_browsertest.cc:423: browser()->tab_strip_model()->GetActiveWebContents(); Oh, weird. I didn't realize at first that app_browser_ was different from browser(), which made this look a bit odd. But I guess that's consistent with this whole file, so I'm ok with it. https://codereview.chromium.org/2831683002/diff/140001/content/browser/child_... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/2831683002/diff/140001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:259: nit: No need for churn here. https://codereview.chromium.org/2831683002/diff/140001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:934: Or here. https://codereview.chromium.org/2831683002/diff/140001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:1001: DCHECK(!origin.unique()); Maybe this should be a CHECK with an error message. https://codereview.chromium.org/2831683002/diff/140001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:1002: DCHECK(!IsIsolatedOrigin(origin)); Same here, in case someone said --isolated-origins=http://foo.com,http://foo.com https://codereview.chromium.org/2831683002/diff/140001/content/browser/child_... File content/browser/child_process_security_policy_impl.h (right): https://codereview.chromium.org/2831683002/diff/140001/content/browser/child_... content/browser/child_process_security_policy_impl.h:180: // unique origins, such as data: URLs, are not supported. Suborigins and Might clarify what you mean by suborigins, since it sounds like bar.foo.com at first. Maybe there's a link we can use, or a more specific term? https://codereview.chromium.org/2831683002/diff/140001/content/browser/child_... content/browser/child_process_security_policy_impl.h:296: // IsIsolatedOrigin(). On 2017/05/16 17:26:38, alexmos (mostly OOO on 5-19) wrote: > This cycle is nasty, and I'm not sure this is a good way to deal with it. :( I > considered handling isolated origins separately before calling GetSiteForURL, > but that didn't seem clean (it'd need to be done in > CPSPI::CanAccessDataForOrigin, where isolated_origins_ are, and not on > SecurityState, but it'd still need origin_lock_ to compare against which is on > SecurityState). Or maybe we could move the isolated origin tracking into their > own class after all, to avoid tying their state synchronization to the rest of > CPSP. Let me know what you think. Ooh, this is tough. Let's find a chance to chat about this, since it's unfortunate to need another lock... https://codereview.chromium.org/2831683002/diff/140001/content/browser/site_i... File content/browser/site_instance_impl.cc (right): https://codereview.chromium.org/2831683002/diff/140001/content/browser/site_i... content/browser/site_instance_impl.cc:313: // This must be done before the GetEffectiveURL resolution, as isolated nit: We're not doing it before the GetEffectiveURL calls, and there's a few additional checks before this as well. I think that might be ok, and we should just rephrase the comment (s/before/on the full URLs without/)? https://codereview.chromium.org/2831683002/diff/140001/content/browser/site_i... content/browser/site_instance_impl.cc:392: // TODO(alexmos): revisit this for Isolate-Me. I may just be forgetting, but why would an Isolate-Me origin not require a dedicated process?
One possible solution for the lock issue below. https://codereview.chromium.org/2831683002/diff/140001/content/browser/child_... File content/browser/child_process_security_policy_impl.h (right): https://codereview.chromium.org/2831683002/diff/140001/content/browser/child_... content/browser/child_process_security_policy_impl.h:296: // IsIsolatedOrigin(). On 2017/05/19 00:10:19, Charlie Reis wrote: > On 2017/05/16 17:26:38, alexmos (mostly OOO on 5-19) wrote: > > This cycle is nasty, and I'm not sure this is a good way to deal with it. :( > I > > considered handling isolated origins separately before calling GetSiteForURL, > > but that didn't seem clean (it'd need to be done in > > CPSPI::CanAccessDataForOrigin, where isolated_origins_ are, and not on > > SecurityState, but it'd still need origin_lock_ to compare against which is on > > SecurityState). Or maybe we could move the isolated origin tracking into > their > > own class after all, to avoid tying their state synchronization to the rest of > > CPSP. Let me know what you think. > > Ooh, this is tough. Let's find a chance to chat about this, since it's > unfortunate to need another lock... What if we moved the GetSiteForURL call from SecurityState::CanAccessDataForOrigin out to ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin, before acquiring lock_? Then we could document that both CanAccessDataForOrigin and LockToOrigin on SecurityState expect site_urls, not arbitrary GURLs, and hopefully avoid the need for an extra lock? It looks like the only other use of SiteInstance in ChildProcessSecurityPolicyImpl is another GetSiteForURL call within LockToOrigin, but that's also done before lock_ is acquired, so it shouldn't be a problem.
The CQ bit was checked by alexmos@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by alexmos@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: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexmos@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: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks for reviewing, Charlie! Can you please take another look? The current trybot failures are all due to https://bugs.chromium.org/p/chromium/issues/detail?id=725716 affecting the MainFrameNavigation test (not yet sure why it came up now and not in previous patchsets -- perhaps something in latest rebase?). I'm working on fixing that, or I can also tweak the test to avoid that scenario. https://codereview.chromium.org/2831683002/diff/140001/chrome/browser/ui/exte... File chrome/browser/ui/extensions/hosted_app_browsertest.cc (right): https://codereview.chromium.org/2831683002/diff/140001/chrome/browser/ui/exte... chrome/browser/ui/extensions/hosted_app_browsertest.cc:393: "urls": ["*://isolated.foo.com/"] On 2017/05/19 00:10:18, Charlie Reis wrote: > Do we need to include http://app.foo.com in here as well? Done. Reading the docs (https://developer.chrome.com/webstore/hosted_apps), this might not be technically needed: "urls: The URLs for the pages in the hosted app, not necessarily including the launch page. Once the app is installed, these pages and the launch page have the permissions requested in the manifest." I.e., the launch URL will be appended to this list. But I agree it makes more sense to just include it. https://codereview.chromium.org/2831683002/diff/140001/content/browser/child_... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/2831683002/diff/140001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:259: On 2017/05/19 00:10:18, Charlie Reis wrote: > nit: No need for churn here. Done. https://codereview.chromium.org/2831683002/diff/140001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:934: On 2017/05/19 00:10:18, Charlie Reis wrote: > Or here. Done. https://codereview.chromium.org/2831683002/diff/140001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:1001: DCHECK(!origin.unique()); On 2017/05/19 00:10:18, Charlie Reis wrote: > Maybe this should be a CHECK with an error message. Done. Good idea. https://codereview.chromium.org/2831683002/diff/140001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:1002: DCHECK(!IsIsolatedOrigin(origin)); On 2017/05/19 00:10:18, Charlie Reis wrote: > Same here, in case someone said --isolated-origins=http://foo.com,http://foo.com Done. This particular one might go away with SafeBrowsing though, since a same site could throw up multiple SB warnings, even within the same BrowsingInstance. https://codereview.chromium.org/2831683002/diff/140001/content/browser/child_... File content/browser/child_process_security_policy_impl.h (right): https://codereview.chromium.org/2831683002/diff/140001/content/browser/child_... content/browser/child_process_security_policy_impl.h:180: // unique origins, such as data: URLs, are not supported. Suborigins and On 2017/05/19 00:10:18, Charlie Reis wrote: > Might clarify what you mean by suborigins, since it sounds like http://bar.foo.com at > first. Maybe there's a link we can use, or a more specific term? Ah, good call - I didn't realize there might be such a confusion. I added a link to the spec and an explicit comment that these are not subdomains. https://codereview.chromium.org/2831683002/diff/140001/content/browser/child_... content/browser/child_process_security_policy_impl.h:296: // IsIsolatedOrigin(). On 2017/05/19 16:54:07, Charlie Reis wrote: > On 2017/05/19 00:10:19, Charlie Reis wrote: > > On 2017/05/16 17:26:38, alexmos (mostly OOO on 5-19) wrote: > > > This cycle is nasty, and I'm not sure this is a good way to deal with it. :( > > > I > > > considered handling isolated origins separately before calling > GetSiteForURL, > > > but that didn't seem clean (it'd need to be done in > > > CPSPI::CanAccessDataForOrigin, where isolated_origins_ are, and not on > > > SecurityState, but it'd still need origin_lock_ to compare against which is > on > > > SecurityState). Or maybe we could move the isolated origin tracking into > > their > > > own class after all, to avoid tying their state synchronization to the rest > of > > > CPSP. Let me know what you think. > > > > Ooh, this is tough. Let's find a chance to chat about this, since it's > > unfortunate to need another lock... > > What if we moved the GetSiteForURL call from > SecurityState::CanAccessDataForOrigin out to > ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin, before acquiring lock_? > Then we could document that both CanAccessDataForOrigin and LockToOrigin on > SecurityState expect site_urls, not arbitrary GURLs, and hopefully avoid the > need for an extra lock? > > It looks like the only other use of SiteInstance in > ChildProcessSecurityPolicyImpl is another GetSiteForURL call within > LockToOrigin, but that's also done before lock_ is acquired, so it shouldn't be > a problem. Thanks for the suggestion! I went ahead and followed it. I initially thought something like this would be problematic because we'll end up doing CanAccessDataForOrigin over two transactions instead of one, opening the door to races in between the lock getting acquired, but thinking about this more carefully, this might actually be ok. The scenario I was worried about was: 1. [IO] www.foo.com accesses a cookie and retrieves GetSiteForURL. That queries isolated origins and then returns "foo.com". 2. [UI] www.foo.com is added to the isolated origin list (e.g., as part of SB clickthrough), and we load www.foo.com in a new process locked to a site URL of "www.foo.com". 3. [IO] continuing step 1, we check whether origin_lock_ matches "foo.com" (grabbed from step 1), and it might not if we end up using the origin_lock_ of "www.foo.com" from step 2. But I think steps 1-3 can't happen for the same process as step 2: we won't change the origin lock on any existing processes, we'll only use the new origin lock for new processes dedicated to the new isolated origin. Even if we decide to do something like reloading tabs containing the newly-isolated origin within the same BI, they will reload in a new dedicated process. So I think this is fine. :)
Sorry, a couple more replies that didn't get sent. https://codereview.chromium.org/2831683002/diff/140001/content/browser/site_i... File content/browser/site_instance_impl.cc (right): https://codereview.chromium.org/2831683002/diff/140001/content/browser/site_i... content/browser/site_instance_impl.cc:313: // This must be done before the GetEffectiveURL resolution, as isolated On 2017/05/19 00:10:19, Charlie Reis wrote: > nit: We're not doing it before the GetEffectiveURL calls, and there's a few > additional checks before this as well. I think that might be ok, and we should > just rephrase the comment (s/before/on the full URLs without/)? This no longer applies now that I've modified GetEffectiveURL itself to be aware of isolated origins and removed the comment. https://codereview.chromium.org/2831683002/diff/140001/content/browser/site_i... content/browser/site_instance_impl.cc:392: // TODO(alexmos): revisit this for Isolate-Me. On 2017/05/19 00:10:19, Charlie Reis wrote: > I may just be forgetting, but why would an Isolate-Me origin not require a > dedicated process? I was just thinking about the discussion that isolate-me is more of a hint for the process model rather than a requirement, and that if too many sites use isolate-me, we might not be able to guarantee a dedicated process for each one, unless we totally remove the process limit. Or would that be done through a separate process reuse policy, and we'll still treat isolate-me origins as requiring dedicated processes?
Thanks! LGTM with one question about scheme checks below. https://codereview.chromium.org/2831683002/diff/140001/content/browser/site_i... File content/browser/site_instance_impl.cc (right): https://codereview.chromium.org/2831683002/diff/140001/content/browser/site_i... content/browser/site_instance_impl.cc:392: // TODO(alexmos): revisit this for Isolate-Me. On 2017/05/24 00:28:33, alexmos wrote: > On 2017/05/19 00:10:19, Charlie Reis wrote: > > I may just be forgetting, but why would an Isolate-Me origin not require a > > dedicated process? > > I was just thinking about the discussion that isolate-me is more of a hint for > the process model rather than a requirement, and that if too many sites use > isolate-me, we might not be able to guarantee a dedicated process for each one, > unless we totally remove the process limit. Or would that be done through a > separate process reuse policy, and we'll still treat isolate-me origins as > requiring dedicated processes? I see. Yeah, I think there's some flexibility in how we implement it. I was kind of imagining that we wouldn't put the origin on the list of isolated origins if we decide we can't honor the request, but I like the idea of using a process reuse policy instead to just let them share with each other if necessary. https://codereview.chromium.org/2831683002/diff/200001/content/browser/site_i... File content/browser/site_instance_impl.cc (right): https://codereview.chromium.org/2831683002/diff/200001/content/browser/site_i... content/browser/site_instance_impl.cc:7: #include "base/macros.h" Is this still needed? https://codereview.chromium.org/2831683002/diff/200001/content/browser/site_i... content/browser/site_instance_impl.cc:324: // same origin. Now that you mention this, is this scheme check ever correct for blob or filesystem URLs? If you navigate from foo.com to a blob or filesystem URL on foo.com, that should presumably be considered same-site even in the common case, not just for isolated origins, right? No need to fix that here (at the risk of larger regressions), but maybe this is a bug we should fix?
The CQ bit was checked by alexmos@chromium.org to run a CQ dry run
Thanks, Charlie! https://codereview.chromium.org/2831683002/diff/140001/content/browser/site_i... File content/browser/site_instance_impl.cc (right): https://codereview.chromium.org/2831683002/diff/140001/content/browser/site_i... content/browser/site_instance_impl.cc:392: // TODO(alexmos): revisit this for Isolate-Me. On 2017/05/25 01:54:37, Charlie Reis (overloaded) wrote: > On 2017/05/24 00:28:33, alexmos wrote: > > On 2017/05/19 00:10:19, Charlie Reis wrote: > > > I may just be forgetting, but why would an Isolate-Me origin not require a > > > dedicated process? > > > > I was just thinking about the discussion that isolate-me is more of a hint for > > the process model rather than a requirement, and that if too many sites use > > isolate-me, we might not be able to guarantee a dedicated process for each > one, > > unless we totally remove the process limit. Or would that be done through a > > separate process reuse policy, and we'll still treat isolate-me origins as > > requiring dedicated processes? > > I see. Yeah, I think there's some flexibility in how we implement it. I was > kind of imagining that we wouldn't put the origin on the list of isolated > origins if we decide we can't honor the request, but I like the idea of using a > process reuse policy instead to just let them share with each other if > necessary. Yes - I removed the comment given that this place may not be affected at all for isolate-me if we do things through process reuse policy. It's probably better to figure things out once we start implementing isolate-me than to add this comment now and have it become stale later on. https://codereview.chromium.org/2831683002/diff/200001/content/browser/site_i... File content/browser/site_instance_impl.cc (right): https://codereview.chromium.org/2831683002/diff/200001/content/browser/site_i... content/browser/site_instance_impl.cc:7: #include "base/macros.h" On 2017/05/25 01:54:37, Charlie Reis (overloaded) wrote: > Is this still needed? No - removed. https://codereview.chromium.org/2831683002/diff/200001/content/browser/site_i... content/browser/site_instance_impl.cc:324: // same origin. On 2017/05/25 01:54:37, Charlie Reis (overloaded) wrote: > Now that you mention this, is this scheme check ever correct for blob or > filesystem URLs? If you navigate from http://foo.com to a blob or filesystem URL on > http://foo.com, that should presumably be considered same-site even in the common case, > not just for isolated origins, right? > > No need to fix that here (at the risk of larger regressions), but maybe this is > a bug we should fix? Yes, indeed, I think it's a bug and I need to investigate exactly what consequences it might have. Nasko and I discussed this some time back - I think this might cause us to swap processes when we shouldn't, so unlikely to be a security problem. I filed https://crbug.com/726370 to follow up on this and referenced it here.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
alexmos@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Devlin, can you please review the test in chrome/browser/ui/extensions/hosted_app_browsertest.cc for OWNERS?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Introduce support for origins that require process isolation. This CL changes logic in SiteInstance to support isolating specific origins based on the full scheme+host+port tuple rather than eTLD+1. The global list of such origins is maintained by SiteInstanceImpl and currently not exposed outside of content. This CL also adds a command-line option to add isolated origins. Sample usage: --isolate-origins=https://isolated.foo.com,https://bar.com Design doc: https://goo.gl/99ynqK BUG=713444 ========== to ========== Introduce support for origins that require process isolation. This CL changes logic in SiteInstance to support isolating specific origins based on the full scheme+host+port tuple rather than just scheme and eTLD+1. The global list of such origins is maintained by SiteInstanceImpl and currently not exposed outside of content. This CL also adds a command-line option to add isolated origins. Sample usage: --isolate-origins=https://isolated.foo.com,https://bar.com Design doc: https://goo.gl/99ynqK BUG=713444 ==========
lgtm for this as a test behind a flag (its current state). Do we have a plan for how this fits in with the hosted app model long-term? Do we need to deprecate portions of the process model before we can launch isolate origins?
On 2017/05/26 02:00:24, Devlin (catching up) wrote: > lgtm for this as a test behind a flag (its current state). Do we have a plan > for how this fits in with the hosted app model long-term? Do we need to > deprecate portions of the process model before we can launch isolate origins? Thanks! There's a bit of discussion about this in the design doc (https://goo.gl/99ynqK). In the short-term, the first use case will be accounts.google.com, and for that I think the current behavior should be ok. When we start using this for more (random) sites, I agree that we'll need to think about implications to the hosted app behavior more carefully. Feel free to comment in the doc if you have thoughts about this.
The CQ bit was checked by alexmos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/2831683002/#ps220001 (title: "Charlie's comments (round 3)")
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
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 alexmos@chromium.org
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": 220001, "attempt_start_ts": 1495840142089720,
"parent_rev": "09f367096ac4724bd0a14d86a346317e4cc68500", "commit_rev":
"3b9ad10d35e7cb293f559d262bf417d7aa099f4d"}
Message was sent while issue was closed.
Description was changed from ========== Introduce support for origins that require process isolation. This CL changes logic in SiteInstance to support isolating specific origins based on the full scheme+host+port tuple rather than just scheme and eTLD+1. The global list of such origins is maintained by SiteInstanceImpl and currently not exposed outside of content. This CL also adds a command-line option to add isolated origins. Sample usage: --isolate-origins=https://isolated.foo.com,https://bar.com Design doc: https://goo.gl/99ynqK BUG=713444 ========== to ========== Introduce support for origins that require process isolation. This CL changes logic in SiteInstance to support isolating specific origins based on the full scheme+host+port tuple rather than just scheme and eTLD+1. The global list of such origins is maintained by SiteInstanceImpl and currently not exposed outside of content. This CL also adds a command-line option to add isolated origins. Sample usage: --isolate-origins=https://isolated.foo.com,https://bar.com Design doc: https://goo.gl/99ynqK BUG=713444 Review-Url: https://codereview.chromium.org/2831683002 Cr-Commit-Position: refs/heads/master@{#475191} Committed: https://chromium.googlesource.com/chromium/src/+/3b9ad10d35e7cb293f559d262bf4... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/3b9ad10d35e7cb293f559d262bf4... |
