Browser crash on startup when specifying a long list for --isolate-origins |
||||||||
Issue descriptionWhat steps will reproduce the problem? Start chrome with --isolate-origins=`cat long_origin_list` where long_origin_list is a long comma-separated list of origins. In experimenting with a list of about 600 origins, I hit the following on browser startup for Linux (same list works fine on Mac): [28845:28873:1208/095123.518620:FATAL:zygote_communication_linux.cc(50)] Check failed: data.size() <= kZygoteMaxMessageLength. Trying to send too-large message to zygote (sending 32780 bytes, max is 8192) #0 0x7fadc9c222cc base::debug::StackTrace::StackTrace() #1 0x7fadc9c4c21c logging::LogMessage::~LogMessage() #2 0x7fadc77a4d33 content::ZygoteCommunication::SendMessage() #3 0x7fadc77a5707 content::ZygoteCommunication::ForkRequest() #4 0x7fadc72dd54a content::internal::ChildProcessLauncherHelper::LaunchProcessOnLauncherThread() #5 0x7fadc72dc244 content::internal::ChildProcessLauncherHelper::LaunchOnLauncherThread() #6 0x7fadc6e93ceb _ZN4base8internal7InvokerINS0_9BindStateIMN7content12_GLOBAL__N_121FileTraceDataEndpointEFvvEJ13scoped_refptrIS5_EEEEFvvEE3RunEPNS0_13BindStateBaseE #7 0x7fadc9c22bea base::debug::TaskAnnotator::RunTask() #8 0x7fadc9cbdd0c base::internal::TaskTracker::RunOrSkipTask() #9 0x7fadc9cbe975 base::internal::TaskTrackerPosix::RunOrSkipTask() #10 0x7fadc9cbcd85 base::internal::TaskTracker::RunNextTask() #11 0x7fadc9cb4e69 base::internal::SchedulerWorker::Thread::ThreadMain() #12 0x7fadc9cc8e8f base::(anonymous namespace)::ThreadFunc() #13 0x7fadc9d6d184 start_thread #14 0x7fadbcc7effd clone In practice I was able to only get away with ~100 origins (where the list was ~3K). This might be a problem we need to solve if we want to support longer origin lists for this flag or the corresponding enterprise policy.
,
Jan 9 2018
AFAIK --isolate-origins doesn't need to be propagated to renderer processes - I think we should remove |switches::kIsolateOrigins| from |kSwitchNames| in RenderProcessHostImpl::PropagateBrowserCommandLineToRenderer.
,
Jan 9 2018
#2: that's a good point! I agree we should be able to remove that from PropagateBrowserCommandLineToRenderer, and possibly kSitePerProcess as well. I wonder if moving SiteIsolationPolicy to content/browser made that possible. We can presumably do this and merge to M64, but this will probably remain a problem for setting the --isolate-origins enterprise policy in M63.
,
Jan 9 2018
WIP CL with a fix here: https://crrev.com/c/857236 (and a follow-up CL for later here: https://crrev.com/c/857584)
,
Jan 9 2018
creis@ - the fix I have in #c4 seems fairly simple and safe, so I think we might want to eventually merge it back to M64. Would you agree?
,
Jan 10 2018
Yes, I think merging https://crrev.com/c/857236 to M64 makes sense, to avoid risk of crashes for long lists of origins via enterprise policy. (Did we used to have uses of that flag in the renderer process when we decided to pass it to the renderer? Can you verify that those uses were gone by M64 and not just M65?)
,
Jan 10 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/770c0be163a6153941bd07021ef6b096365ba46c commit 770c0be163a6153941bd07021ef6b096365ba46c Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Wed Jan 10 15:24:48 2018 Avoid propagating switches::kIsolateOrigins to renderer processes. Presence of switches::kIsolateOrigins is only inspected inside the browser process and therefore this cmdline flag doesn't need to be forwarded into cmdline of renderer processes. This saves some room and avoids crashing due to exceeded kZygoteMaxMessageLength. Bug: 793350 Change-Id: Id4b194b2b06810980e9275dfa224e0264e9302e9 Reviewed-on: https://chromium-review.googlesource.com/857236 Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#528319} [modify] https://crrev.com/770c0be163a6153941bd07021ef6b096365ba46c/chrome/browser/chrome_content_browser_client.cc [modify] https://crrev.com/770c0be163a6153941bd07021ef6b096365ba46c/content/browser/isolated_origin_browsertest.cc [modify] https://crrev.com/770c0be163a6153941bd07021ef6b096365ba46c/content/browser/renderer_host/render_process_host_impl.cc
,
Jan 10 2018
,
Jan 10 2018
I've tried |git grep -l '\bkIsolateOrigins\b'| at branch-heads/3282 and I don't see any usages on the renderer side:
chrome/browser/apps/guest_view/web_view_browsertest.cc
chrome/browser/chrome_browser_main.cc
chrome/browser/chrome_content_browser_client.cc
chrome/browser/chrome_content_browser_client_browsertest.cc
chrome/browser/chromeos/login/session/user_session_manager.cc
chrome/browser/extensions/process_management_browsertest.cc
chrome/browser/policy/configuration_policy_handler_list_factory.cc
chrome/browser/policy/site_isolation_policy_browsertest.cc
chrome/browser/prefs/chrome_command_line_pref_store.cc
chrome/browser/ui/extensions/hosted_app_browsertest.cc
chrome/common/pref_names.cc
chrome/common/pref_names.h
content/browser/isolated_origin_browsertest.cc
content/browser/loader/cross_site_document_blocking_browsertest.cc
content/browser/renderer_host/render_process_host_impl.cc
content/common/site_isolation_policy.cc
content/public/common/content_features.cc
content/public/common/content_features.h
content/public/common/content_switches.cc
content/public/common/content_switches.h
In particular:
- content/public/common/content_switches.* and content_features.*
include only declarations/definitions of global variables
- content/common/site_isolation_policy.cc uses kIsolateOrigins
only from the following 2 methods
- SiteIsolationPolicy::AreIsolatedOriginsEnabled
- SiteIsolationPolicy::GetIsolatedOrigins
- These 2 methods are only used inside the browser process:
$ git grep -l '\b\(AreIsolatedOriginsEnabled\|GetIsolatedOrigins\)\b'
content/browser/browser_main_loop.cc
content/browser/loader/resource_dispatcher_host_impl.cc
content/browser/renderer_host/render_process_host_impl.cc
content/common/site_isolation_policy.cc
content/common/site_isolation_policy.h
Based on the above, I think the merge to M64 should be okay.
,
Jan 10 2018
Let's let the fix from #c7 bake for a day on the Canary channel and we can request the merge to M64 tomorrow. I note that verification of the bug might be a bit difficult, because AFAIK there is no Chrome Canary for Linux (which is the only platform where this bug is present). I think the ToT verification done when working on #c7 should be sufficient (and if not, then I guess we could in theory wait for a dev release... :-/).
,
Jan 11 2018
Requesting approval to merge to M64. Commit 770c0be1... initially landed in 65.0.3318.0 and so far there was no negative fallout from the fix. I feel pretty confident that the fix really does fix the issue on Linux, but as pointed out in #c10 we can't verify that on a Canary, because there AFAIK there are no Canaries for Linux. I think this shouldn't block the merge.
,
Jan 11 2018
This bug requires manual review: We are only 11 days from stable. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 11 2018
Oh, and the main reason to merge to M64 would be to avoid making Chrome unusable (crash when starting any renderer) when somebody specifies a *long* list of origins to isolate via Enterprise Policy.
,
Jan 11 2018
Approving merge for M64. Branch:3282
,
Jan 11 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e55d3f2330becb5156ba13276936eb8c0778af96 commit e55d3f2330becb5156ba13276936eb8c0778af96 Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Thu Jan 11 18:41:40 2018 Avoid propagating switches::kIsolateOrigins to renderer processes. Presence of switches::kIsolateOrigins is only inspected inside the browser process and therefore this cmdline flag doesn't need to be forwarded into cmdline of renderer processes. This saves some room and avoids crashing due to exceeded kZygoteMaxMessageLength. TBR=lukasza@chromium.org (cherry picked from commit 770c0be163a6153941bd07021ef6b096365ba46c) Bug: 793350 Change-Id: Id4b194b2b06810980e9275dfa224e0264e9302e9 Reviewed-on: https://chromium-review.googlesource.com/857236 Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#528319} Reviewed-on: https://chromium-review.googlesource.com/862189 Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org> Cr-Commit-Position: refs/branch-heads/3282@{#487} Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840} [modify] https://crrev.com/e55d3f2330becb5156ba13276936eb8c0778af96/chrome/browser/chrome_content_browser_client.cc [modify] https://crrev.com/e55d3f2330becb5156ba13276936eb8c0778af96/content/browser/isolated_origin_browsertest.cc [modify] https://crrev.com/e55d3f2330becb5156ba13276936eb8c0778af96/content/browser/renderer_host/render_process_host_impl.cc
,
Jan 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ffb5de3934abd4e16399b2f79e7100823c6076fd commit ffb5de3934abd4e16399b2f79e7100823c6076fd Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Fri Jan 12 20:31:21 2018 Don't propagate cmdline flags that are not read by renderer processes. This CL identifies cmdline flags that are not read by renderer processes and modifies RenderProcessHostImpl::PropagateBrowserCommandLineToRenderer so that such cmdline flags are not unnecessarily propagated into renderer processes. Candidates for removal have first been identified by: $ # put switches from PropagateBrowserCommandLineToRenderer $ # into ~/scratch/switches.in $ for i in `cat ~/scratch/switches.in`; do echo -n "$i "; git grep -l "\\b$i\\b" | grep -v '/browser/' | grep -v '/common/' | wc -l; done > ~/scratch/switches.out $ cat ~/scratch/switches.out | grep ' 0$' ... and then manually vetted by manual Code Search (to confirm that they really are not used in the renderer code). Bug: 793350 Change-Id: I3e730f0bd0522ebbb61b60906001d914ca033682 Reviewed-on: https://chromium-review.googlesource.com/857584 Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#529051} [modify] https://crrev.com/ffb5de3934abd4e16399b2f79e7100823c6076fd/chrome/browser/chrome_content_browser_client.cc [modify] https://crrev.com/ffb5de3934abd4e16399b2f79e7100823c6076fd/content/browser/child_process_launcher_browsertest.cc [modify] https://crrev.com/ffb5de3934abd4e16399b2f79e7100823c6076fd/content/browser/renderer_host/render_process_host_impl.cc [modify] https://crrev.com/ffb5de3934abd4e16399b2f79e7100823c6076fd/content/public/common/content_switches.cc [modify] https://crrev.com/ffb5de3934abd4e16399b2f79e7100823c6076fd/content/public/common/content_switches.h |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by lukasza@chromium.org
, Jan 8 2018