|
|
Created:
3 years, 8 months ago by alexmos Modified:
3 years, 8 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, chromium-apps-reviews_chromium.org, site-isolation-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't create unnecessary OOPIFs for subframe navigations that don't require a dedicated process.
Previously, if we had a.com embedding chrome-extension://foo, and the
subframe was navigated to b.com (with --isolate-extensions being on by
default but without any other OOPIF modes), the subframe stayed in an
OOPIF in a new process for b.com, even though technically the b.com
OOPIF is not necessary per --isolate-extensions security model. This
CL fixes that.
BUG=711006
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2809653008
Cr-Commit-Position: refs/heads/master@{#465055}
Committed: https://chromium.googlesource.com/chromium/src/+/24409702df38cbd9505f1cae180be98d2f7e148d
Patch Set 1 #Patch Set 2 : Fix devtools extensions tests #
Total comments: 6
Patch Set 3 : Rebase #Patch Set 4 : Revise comment #
Total comments: 4
Messages
Total messages: 34 (21 generated)
Description was changed from ========== Don't create unnecessary OOPIFs for subframe navigations that don't require a dedicated process. BUG=711006 ========== to ========== Don't create unnecessary OOPIFs for subframe navigations that don't require a dedicated process. BUG=711006 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
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_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_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 ========== Don't create unnecessary OOPIFs for subframe navigations that don't require a dedicated process. BUG=711006 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Don't create unnecessary OOPIFs for subframe navigations that don't require a dedicated process. Previously, if we had a.com embedding chrome-extension://foo, and the subframe was navigated to b.com (with --isolate-extensions being on by default but without any other OOPIF modes), the subframe stayed in an OOPIF in a new process for b.com, even though technically the b.com OOPIF is not necessary per --isolate-extensions security model. This CL fixes that. BUG=711006 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
alexmos@chromium.org changed reviewers: + creis@chromium.org
Charlie, can you please take a look? https://codereview.chromium.org/2809653008/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2809653008/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1478: !parent_url_requires_dedicated_process) { Sigh, without this workaround, the devtools extensions tests that Andrew added broke: the parent (devtools extension) would have a chrome-devtools:// SiteInstance, and dest_url would be http://foo.com/, neither of which requires a dedicated process, yet we do want the process swap. :( Not sure what the best way to fix this is. This one relies on last successful url still being chrome-extension:// in this case. An alternate fix could be to exclude the devtools SiteInstance altogether. Or, any reason why we couldn't require a dedicated process for the whole chrome-devtools:// scheme? (That one is probably better as a separate CL though.) This workaround would go away once we swap processes for devtools extensions in 706169, so I'm hoping we won't need it for long.
Thanks for fixing this! LGTM with some thoughts below. Also, while I'd love to see this get into M59 as a way to potentially reduce the number of OOPIFs in --isolate-extensions, I think we should probably land this after branch cut. Any change to the process model carries some risk of regression, and this doesn't seem like a probable enough case today to require the fix in M59. https://codereview.chromium.org/2809653008/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/2809653008/diff/20001/chrome/browser/extensio... chrome/browser/extensions/process_manager_browsertest.cc:1226: IN_PROC_BROWSER_TEST_F(ProcessManagerBrowserTest, Yep, makes sense to put this here for now, since I'm not sure how to encounter this situation within content. Once we start adding the ability to isolate individual sites to content, we should probably add an equivalent test there. https://codereview.chromium.org/2809653008/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2809653008/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1460: // enforced after the TopDocumentIsolation checks above. I'm slightly wary about this invariant, since it feels like the "requires dedicated process" logic is just one of many ways we might decide an OOPIF or cross-process navigation is necessary. Even the last sentence hints at that, since TDI doesn't use dedicated processes, so that check has to come first. I would imagine there are (or will be) other cases as well-- GuestViews may not fall into this category, but we're considering some future behavior that more aggressively swaps processes on cross-site main frame navigations, such that iframes may have to become OOPIFs to script their openers, etc. That would be an OOPIF for compatibility rather than because either required a dedicated process. Maybe we can rephrase this as more of a heuristic to avoid OOPIFs, as opposed to sounding like an invariant that we need to satisfy? https://codereview.chromium.org/2809653008/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1478: !parent_url_requires_dedicated_process) { On 2017/04/14 00:15:46, alexmos wrote: > Sigh, without this workaround, the devtools extensions tests that Andrew added > broke: the parent (devtools extension) would have a chrome-devtools:// > SiteInstance, and dest_url would be http://foo.com/, neither of which requires a > dedicated process, yet we do want the process swap. :( > > Not sure what the best way to fix this is. This one relies on last successful > url still being chrome-extension:// in this case. An alternate fix could be to > exclude the devtools SiteInstance altogether. Or, any reason why we couldn't > require a dedicated process for the whole chrome-devtools:// scheme? (That one > is probably better as a separate CL though.) > > This workaround would go away once we swap processes for devtools extensions in > 706169, so I'm hoping we won't need it for long. Fun. Devtools definitely should require a dedicated process, but I imagine that would force DevTools extensions out of its process, right? It sounds like that might be part of how we fix https://crbug.com/706169, at least from a high level. The main risk with your current approach is that it could apply to cases other than DevTools, but I don't see that as likely. I don't think there are other cases where a URL says it requires a dedicated process but we don't give it one, and hopefully there will be no such cases after we fix DevTools. So this seems ok to me for the time being.
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...
https://codereview.chromium.org/2809653008/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/2809653008/diff/20001/chrome/browser/extensio... chrome/browser/extensions/process_manager_browsertest.cc:1226: IN_PROC_BROWSER_TEST_F(ProcessManagerBrowserTest, On 2017/04/14 16:57:05, Charlie Reis wrote: > Yep, makes sense to put this here for now, since I'm not sure how to encounter > this situation within content. Once we start adding the ability to isolate > individual sites to content, we should probably add an equivalent test there. Acknowledged, and in fact I've already got that test written as part of my origin isolation CL that's coming soon. :) https://codereview.chromium.org/2809653008/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2809653008/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1460: // enforced after the TopDocumentIsolation checks above. On 2017/04/14 16:57:05, Charlie Reis wrote: > I'm slightly wary about this invariant, since it feels like the "requires > dedicated process" logic is just one of many ways we might decide an OOPIF or > cross-process navigation is necessary. Even the last sentence hints at that, > since TDI doesn't use dedicated processes, so that check has to come first. > > I would imagine there are (or will be) other cases as well-- GuestViews may not > fall into this category, but we're considering some future behavior that more > aggressively swaps processes on cross-site main frame navigations, such that > iframes may have to become OOPIFs to script their openers, etc. That would be > an OOPIF for compatibility rather than because either required a dedicated > process. > > Maybe we can rephrase this as more of a heuristic to avoid OOPIFs, as opposed to > sounding like an invariant that we need to satisfy? Thanks, I agree. I updated the comment - let me know if the new one is better.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sounds good, thanks! LGTM.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
alexmos@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Devlin, can you please review the new test for OWNERS?
lgtm https://codereview.chromium.org/2809653008/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/2809653008/diff/60001/chrome/browser/extensio... chrome/browser/extensions/process_manager_browsertest.cc:1229: if (content::AreAllSitesIsolatedForTesting()) Would it be worth enforcing this in the test via SetUpCommandLine or similar? Trybots have a funny way of usually running with all the experiments turned on.
Thanks! https://codereview.chromium.org/2809653008/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/2809653008/diff/60001/chrome/browser/extensio... chrome/browser/extensions/process_manager_browsertest.cc:1229: if (content::AreAllSitesIsolatedForTesting()) On 2017/04/17 21:49:32, Devlin wrote: > Would it be worth enforcing this in the test via SetUpCommandLine or similar? > Trybots have a funny way of usually running with all the experiments turned on. Good idea, but it might be tricky. I don't see an easy way to remove a switch when it's already been added (e.g., on bots that run with --site-per-process). It also might be dangerous to do it here inside the test, as we might've already started making some process model assumptions (e.g., BrowserProcessImpl::PreCreateThreads has checks for --isolate-extensions). I think it's pretty safe to assume that we'll have bots running tests without --site-per-process for the time being, and we have other tests that rely on this. If we ever end up in our dream world of --site-per-process being on by default, we'll probably want to do something uniformly for all our tests that do this kind of thing.
https://codereview.chromium.org/2809653008/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/2809653008/diff/60001/chrome/browser/extensio... chrome/browser/extensions/process_manager_browsertest.cc:1229: if (content::AreAllSitesIsolatedForTesting()) On 2017/04/17 22:38:59, alexmos wrote: > On 2017/04/17 21:49:32, Devlin wrote: > > Would it be worth enforcing this in the test via SetUpCommandLine or similar? > > Trybots have a funny way of usually running with all the experiments turned > on. > > Good idea, but it might be tricky. I don't see an easy way to remove a switch > when it's already been added (e.g., on bots that run with --site-per-process). > It also might be dangerous to do it here inside the test, as we might've already > started making some process model assumptions (e.g., > BrowserProcessImpl::PreCreateThreads has checks for --isolate-extensions). > > I think it's pretty safe to assume that we'll have bots running tests without > --site-per-process for the time being, and we have other tests that rely on > this. If we ever end up in our dream world of --site-per-process being on by > default, we'll probably want to do something uniformly for all our tests that do > this kind of thing. Fair enough, but my $0.02 are that it's worth looking into sooner rather than later. I've been bit multiple times because of bot configurations. But, certainly doesn't need to block this CL. :)
https://codereview.chromium.org/2809653008/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/2809653008/diff/60001/chrome/browser/extensio... chrome/browser/extensions/process_manager_browsertest.cc:1229: if (content::AreAllSitesIsolatedForTesting()) On 2017/04/17 23:03:37, Devlin wrote: > On 2017/04/17 22:38:59, alexmos wrote: > > On 2017/04/17 21:49:32, Devlin wrote: > > > Would it be worth enforcing this in the test via SetUpCommandLine or > similar? > > > Trybots have a funny way of usually running with all the experiments turned > > on. > > > > Good idea, but it might be tricky. I don't see an easy way to remove a switch > > when it's already been added (e.g., on bots that run with --site-per-process). > > > It also might be dangerous to do it here inside the test, as we might've > already > > started making some process model assumptions (e.g., > > BrowserProcessImpl::PreCreateThreads has checks for --isolate-extensions). > > > > I think it's pretty safe to assume that we'll have bots running tests without > > --site-per-process for the time being, and we have other tests that rely on > > this. If we ever end up in our dream world of --site-per-process being on by > > default, we'll probably want to do something uniformly for all our tests that > do > > this kind of thing. > > Fair enough, but my $0.02 are that it's worth looking into sooner rather than > later. I've been bit multiple times because of bot configurations. > > But, certainly doesn't need to block this CL. :) Acknowledged, we probably should discuss bot configurations/coverage some time soon given all the different OOPIF modes we're working on, so I'll bring this up.
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": 60001, "attempt_start_ts": 1492470489986130, "parent_rev": "b7cd6a5130a7a261ae997c5e37dc4fec4560cff3", "commit_rev": "24409702df38cbd9505f1cae180be98d2f7e148d"}
Message was sent while issue was closed.
Description was changed from ========== Don't create unnecessary OOPIFs for subframe navigations that don't require a dedicated process. Previously, if we had a.com embedding chrome-extension://foo, and the subframe was navigated to b.com (with --isolate-extensions being on by default but without any other OOPIF modes), the subframe stayed in an OOPIF in a new process for b.com, even though technically the b.com OOPIF is not necessary per --isolate-extensions security model. This CL fixes that. BUG=711006 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Don't create unnecessary OOPIFs for subframe navigations that don't require a dedicated process. Previously, if we had a.com embedding chrome-extension://foo, and the subframe was navigated to b.com (with --isolate-extensions being on by default but without any other OOPIF modes), the subframe stayed in an OOPIF in a new process for b.com, even though technically the b.com OOPIF is not necessary per --isolate-extensions security model. This CL fixes that. BUG=711006 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2809653008 Cr-Commit-Position: refs/heads/master@{#465055} Committed: https://chromium.googlesource.com/chromium/src/+/24409702df38cbd9505f1cae180b... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/24409702df38cbd9505f1cae180b... |