|
|
Created:
3 years, 5 months ago by Charlie Reis Modified:
3 years, 5 months ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, site-isolation-reviews_chromium.org, ncarter (slow) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisable cross-process navigations in --single-process.
This flag takes precedence over --site-per-process and other
site isolation modes.
BUG=688617
TEST=Cross-site subframes work with --single-process --site-per-process
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2964033002
Cr-Commit-Position: refs/heads/master@{#485292}
Committed: https://chromium.googlesource.com/chromium/src/+/0b49fa16f0265a0b1274383546fe933c288d2106
Patch Set 1 #Patch Set 2 : Don't lock to origin #Patch Set 3 : Fix condition. #
Total comments: 5
Patch Set 4 : Only update --site-per-process #
Messages
Total messages: 28 (20 generated)
Description was changed from ========== Disable cross-process navigations in --single-process and --process-per-tab. These flags take precedence over --site-per-process. BUG=688617 TEST=Cross-site subframes work with --single-process --site-per-process ========== to ========== Disable cross-process navigations in --single-process and --process-per-tab. These flags take precedence over --site-per-process. BUG=688617 TEST=Cross-site subframes work with --single-process --site-per-process CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by creis@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_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
Description was changed from ========== Disable cross-process navigations in --single-process and --process-per-tab. These flags take precedence over --site-per-process. BUG=688617 TEST=Cross-site subframes work with --single-process --site-per-process CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Disable cross-process navigations in --single-process and --process-per-tab. These flags take precedence over --site-per-process. BUG=688617, 717459 TEST=Cross-site subframes work with --single-process --site-per-process CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by creis@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...
creis@chromium.org changed reviewers: + alexmos@chromium.org
Alex, can you take a look? Hopefully this updated version should pass the try jobs. The goal is that --single-process and --process-per-tab should basically turn off Site Isolation features, since we don't swap processes for cross-site pages (including OOPIFs) and thus can't lock to origin. I'm curious if you see any concerns for how this would interact with --isolate-origins. With this change, these modes seem to take precedence over --isolate-origins, Isolate Extensions, and --site-per-process. I think that's ok, but there are consequences to that (e.g., for actual tests that run in these modes, as well as users who might not know that --process-per-tab is losing out on security improvements).
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_...)
Thanks! I think this makes sense, but a few sanity checks about process-per-tab below. https://codereview.chromium.org/2964033002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2964033002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1083: // within a tab.) Yes, I agree that it makes sense to prioritize --single-process over all other modes, and this should fix the Android webview case if it's indeed the cause of the perf regression there. A few questions about --process-per-tab, which seems trickier: :) 1. Won't we run into trouble with layout tests which still use it? In particular, I'm curious if we will now avoid OOPIFs in the tests that used to have them (e.g., on our FYI bot with --site-per-process). Also, it looks like some devtools extensions layout tests are failing both with and without --site-per-process, so I wonder if pfeldman's new oopif.test logic from https://chromium-review.googlesource.com/c/533897/ would also be broken. We should probably reevaluate whether layout tests can run without --process-per-tab now that we have swarming, but we probably want something in the meantime - perhaps allowing ShouldTransitionCrossSite to return true for layout tests via ShellContentBrowserClient, or just for --site-per-process? 2. Just to verify, are we ok changing precedence so that --process-per-tab is preferred over all our security modes (--isolate-extensions, --site-per-process, etc.)? Assuming people use process-per-tab for performance (to reduce overhead from extra processes and process swaps), it makes sense to disable things like TDI, but it might also make sense to prioritize some security modes over it, kind of like we do in the force_swap cases. I wonder if the Opera process-per-tab use cases around embedded environments actually use extensions -- because this effectively would unship isolate extensions for them. Also, overriding --site-per-process doesn't seem necessary for the Opera uses, as we'll never ship it, and Opera can just not specify the flag. :) 3. Isolated origins aren't really compatible with process-per-tab, and it's probably fine to pick --process-per-tab if that's what the user wants. One way to expose that could be to log a warning and maybe an early return in AddIsolatedOrigin if --process-per-tab is specified. And/or, WDYT about adding "process-per-tab" to kBadFlags, so users will see the warning that "stability and security will suffer"? (Which is true if we prefer it over --isolate-extensions.) I guess we're not too worried about not giving process isolation to sign-in on Opera embedded devices, since we'll probably add the isolated origin from the chrome layer anyway, so Opera won't add it if they build on top of content/ -- is that what they do? Plus Opera won't have any of the sign-in stuff built into Chrome. 4. To help think about 2 and 3, I was curious how many people actually use --process-per-tab in the wild -- do we have any UMAs on that? 5. How does this interact with oopif-webview? Does that whole infrastructure still work with --process-per-tab?
https://codereview.chromium.org/2964033002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2964033002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1078: bool RenderFrameHostManager::ShouldTransitionCrossSite() { Just noticed that the comment for this in the header is a bit out of date (doesn't include --single-process). https://codereview.chromium.org/2964033002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1083: // within a tab.) On 2017/06/30 02:23:29, alexmos wrote: > Yes, I agree that it makes sense to prioritize --single-process over all other > modes, and this should fix the Android webview case if it's indeed the cause of > the perf regression there. > > A few questions about --process-per-tab, which seems trickier: :) > > 1. Won't we run into trouble with layout tests which still use it? In > particular, I'm curious if we will now avoid OOPIFs in the tests that used to > have them (e.g., on our FYI bot with --site-per-process). Also, it looks like > some devtools extensions layout tests are failing both with and without > --site-per-process, so I wonder if pfeldman's new oopif.test logic from > https://chromium-review.googlesource.com/c/533897/ would also be broken. > > We should probably reevaluate whether layout tests can run without > --process-per-tab now that we have swarming, but we probably want something in > the meantime - perhaps allowing ShouldTransitionCrossSite to return true for > layout tests via ShellContentBrowserClient, or just for --site-per-process? Note that Lukasz is removing --process-per-tab from layout tests in https://codereview.chromium.org/2968753003/ :)
creis@chromium.org changed reviewers: + kenrb@chromium.org
https://codereview.chromium.org/2964033002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2964033002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1083: // within a tab.) On 2017/06/30 20:40:38, alexmos wrote: > On 2017/06/30 02:23:29, alexmos wrote: > > Yes, I agree that it makes sense to prioritize --single-process over all other > > modes, and this should fix the Android webview case if it's indeed the cause > of > > the perf regression there. > > > > A few questions about --process-per-tab, which seems trickier: :) > > > > 1. Won't we run into trouble with layout tests which still use it? In > > particular, I'm curious if we will now avoid OOPIFs in the tests that used to > > have them (e.g., on our FYI bot with --site-per-process). Also, it looks like > > some devtools extensions layout tests are failing both with and without > > --site-per-process, so I wonder if pfeldman's new oopif.test logic from > > https://chromium-review.googlesource.com/c/533897/ would also be broken. > > > > We should probably reevaluate whether layout tests can run without > > --process-per-tab now that we have swarming, but we probably want something in > > the meantime - perhaps allowing ShouldTransitionCrossSite to return true for > > layout tests via ShellContentBrowserClient, or just for --site-per-process? > > Note that Lukasz is removing --process-per-tab from layout tests in > https://codereview.chromium.org/2968753003/ :) These are excellent questions. :) I'm getting the sense we should try to address --single-process and --process-per-tab separately. For --single-process, I think we may want to land something like this to unblock Ken's CL, and then follow up on Nick's idea to see if we can make this unnecessary for --single-process. In other words, see if we can avoid the memory overhead by ordering things such that the old document/RenderFrame is unloaded before committing the new one, since they're all in the same process and thread. That might move us closer to not needing this function at all. For --process-per-tab, we can get by a little longer now that layout tests don't need it. For Opera's use case, we should discuss these questions you've raised on https://crbug.com/717459 and figure out the right API/behavior to expose. It may be that we eliminate --process-per-tab entirely and give embedders a way to configure the behavior they want. I'll see if I can update this CL to reflect that next week. (Ken, feel free to try out this CL locally in the meantime if you find a way to repro the regression, to see if this will fix it.)
The CQ bit was checked by creis@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== Disable cross-process navigations in --single-process and --process-per-tab. These flags take precedence over --site-per-process. BUG=688617, 717459 TEST=Cross-site subframes work with --single-process --site-per-process CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Disable cross-process navigations in --single-process. This flag takes precedence over --site-per-process and other site isolation modes. BUG=688617 TEST=Cross-site subframes work with --single-process --site-per-process CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Thanks for the feedback! I've limited this CL to only affect --site-per-process mode for now, leaving --process-per-tab as a no-op until we can make it defer to the higher security modes. https://codereview.chromium.org/2964033002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2964033002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1083: // within a tab.) In more detail... On 2017/06/30 23:45:15, Charlie Reis (slow) wrote: > On 2017/06/30 20:40:38, alexmos wrote: > > On 2017/06/30 02:23:29, alexmos wrote: > > > Yes, I agree that it makes sense to prioritize --single-process over all > other > > > modes, and this should fix the Android webview case if it's indeed the cause > > of > > > the perf regression there. > > > > > > A few questions about --process-per-tab, which seems trickier: :) > > > > > > 1. Won't we run into trouble with layout tests which still use it? In > > > particular, I'm curious if we will now avoid OOPIFs in the tests that used > to > > > have them (e.g., on our FYI bot with --site-per-process). Also, it looks > like > > > some devtools extensions layout tests are failing both with and without > > > --site-per-process, so I wonder if pfeldman's new oopif.test logic from > > > https://chromium-review.googlesource.com/c/533897/ would also be broken. Yes, those tests must somehow assume that we have OOPIFs and/or --isolate-extensions? When my CL was disabling that in --process-per-tab, it caused those tests to fail. Moot now that we don't use --process-per-tab for layout tests. > > > > > > We should probably reevaluate whether layout tests can run without > > > --process-per-tab now that we have swarming, but we probably want something > in > > > the meantime - perhaps allowing ShouldTransitionCrossSite to return true for > > > layout tests via ShellContentBrowserClient, or just for --site-per-process? > > > > Note that Lukasz is removing --process-per-tab from layout tests in > > https://codereview.chromium.org/2968753003/ :) Hooray! > These are excellent questions. :) > > I'm getting the sense we should try to address --single-process and > --process-per-tab separately. > > For --single-process, I think we may want to land something like this to unblock > Ken's CL, and then follow up on Nick's idea to see if we can make this > unnecessary for --single-process. In other words, see if we can avoid the > memory overhead by ordering things such that the old document/RenderFrame is > unloaded before committing the new one, since they're all in the same process > and thread. That might move us closer to not needing this function at all. > > For --process-per-tab, we can get by a little longer now that layout tests don't > need it. For Opera's use case, we should discuss these questions you've raised > on https://crbug.com/717459 and figure out the right API/behavior to expose. It > may be that we eliminate --process-per-tab entirely and give embedders a way to > configure the behavior they want. > > I'll see if I can update this CL to reflect that next week. (Ken, feel free to > try out this CL locally in the meantime if you find a way to repro the > regression, to see if this will fix it.) > 2. Just to verify, are we ok changing precedence so that --process-per-tab is > preferred over all our security modes (--isolate-extensions, --site-per-process, > etc.)? Assuming people use process-per-tab for performance (to reduce overhead > from extra processes and process swaps), it makes sense to disable things like > TDI, but it might also make sense to prioritize some security modes over it, > kind of like we do in the force_swap cases. I wonder if the Opera > process-per-tab use cases around embedded environments actually use extensions > -- because this effectively would unship isolate extensions for them. Also, > overriding --site-per-process doesn't seem necessary for the Opera uses, as > we'll never ship it, and Opera can just not specify the flag. :) It's probably not a good thing that my update to the --process-per-tab flag turns off --isolate-extensions and isolated origins in Chrome. I think that mode should probably turn off "unnecessary" process swaps for general cross-site navigations, but not ones that have been deemed necessary for security. We'll need to handle that a bit differently, so let's leave --process-per-tab as a no-op for now (as it has been on all platforms but Android for a while now) and revisit to make it work properly after this CL lands. > > 3. Isolated origins aren't really compatible with process-per-tab, and it's > probably fine to pick --process-per-tab if that's what the user wants. One way > to expose that could be to log a warning and maybe an early return in > AddIsolatedOrigin if --process-per-tab is specified. And/or, WDYT about adding > "process-per-tab" to kBadFlags, so users will see the warning that "stability > and security will suffer"? (Which is true if we prefer it over > --isolate-extensions.) > > I guess we're not too worried about not giving process isolation to sign-in on > Opera embedded devices, since we'll probably add the isolated origin from the > chrome layer anyway, so Opera won't add it if they build on top of content/ -- > is that what they do? Plus Opera won't have any of the sign-in stuff built into > Chrome. > > 4. To help think about 2 and 3, I was curious how many people actually use > --process-per-tab in the wild -- do we have any UMAs on that? I'm not aware of any UMAs for tracking uses of this flag. We could probably query crash reports to get a sense for how prevalent it is, though? > 5. How does this interact with oopif-webview? Does that whole infrastructure > still work with --process-per-tab? > Good question. With the new plan to make OOPIFs work within an updated --process-per-tab mode, it shouldn't be a problem.
Thanks, LGTM!
The CQ bit was checked by creis@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": 1499703966599400, "parent_rev": "0fd5121f8fe912464b4f4d46dbb90d6209f59bf0", "commit_rev": "0b49fa16f0265a0b1274383546fe933c288d2106"}
Message was sent while issue was closed.
Description was changed from ========== Disable cross-process navigations in --single-process. This flag takes precedence over --site-per-process and other site isolation modes. BUG=688617 TEST=Cross-site subframes work with --single-process --site-per-process CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Disable cross-process navigations in --single-process. This flag takes precedence over --site-per-process and other site isolation modes. BUG=688617 TEST=Cross-site subframes work with --single-process --site-per-process CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2964033002 Cr-Commit-Position: refs/heads/master@{#485292} Committed: https://chromium.googlesource.com/chromium/src/+/0b49fa16f0265a0b1274383546fe... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/0b49fa16f0265a0b1274383546fe... |