mash: content_browser crash on shutdown |
|||||||
Issue description
Release, branded (is_google_chrome = true) build for Pixel 1 ("link") using go/simplechrome
bind mount /etc/chrome_dev.conf and edit to add --mash --ash-debug-shortcuts and --enable-crash-reporter-for-testing (the last one should not matter)
start ui
Hit Ctrl-Alt-Shift-K at login screen
Ash process crashes intentionally
content_browser also crashes during shutdown
Crash reporter sees crash for content_browser, but not ash (not sure why)
If you attach gdb to content_browser before you do the above you get this:
#0 0x00007f2b0b52549c in chromeos::LoginDisplayHostImpl::ResetLoginWindowAndView() () at ../../chrome/browser/chromeos/login/ui/login_display_host_impl.cc:1259
#1 0x00007f2b0b52562c in chromeos::LoginDisplayHostImpl::~LoginDisplayHostImpl() () at ../../chrome/browser/chromeos/login/ui/login_display_host_impl.cc:479
#2 0x00007f2b0b525991 in chromeos::LoginDisplayHostImpl::~LoginDisplayHostImpl() () at ../../chrome/browser/chromeos/login/ui/login_display_host_impl.cc:490
#3 0x00007f2b0be34af3 in base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*) () at ../../base/callback.h:68
#4 0x00007f2b0bdb84ef in base::MessageLoop::RunTask(base::PendingTask*) () at ../../base/message_loop/message_loop.cc:423
#5 0x00007f2b0bdb899e in base::MessageLoop::DeferOrRunPendingTask(base::PendingTask) () at ../../base/message_loop/message_loop.cc:434
#6 0x00007f2b0bdbaca0 in base::MessageLoop::DoWork() [clone .part.164] () at ../../base/message_loop/message_loop.cc:527
#7 0x00007f2b0bdbb219 in base::MessagePumpLibevent::Run(base::MessagePump::Delegate*) () at ../../base/message_loop/message_pump_libevent.cc:218
#8 0x00007f2b0bdb825a in base::MessageLoop::RunHandler() () at ../../base/message_loop/message_loop.cc:387
#9 0x00007f2b0bddeffd in base::RunLoop::Run() () at ../../base/run_loop.cc:37
#10 0x00007f2b0b9b3863 in ChromeBrowserMainParts::MainMessageLoopRun(int*) () at ../../chrome/browser/chrome_browser_main.cc:2012
warning: (Internal error: pc 0x7f2b0b9b3862 in read in psymtab, but not in symtab.)
#11 0x00007f2b0a2bf6d0 in content::BrowserMainLoop::RunMainMessageLoopParts() () at ../../content/browser/browser_main_loop.cc:1180
#12 0x00007f2b0a2c3d05 in content::BrowserMainRunnerImpl::Run() () at ../../content/browser/browser_main_runner.cc:141
#13 0x00007f2b0a2bb8d9 in content::BrowserMain(content::MainFunctionParams const&) () at ../../content/browser/browser_main.cc:46
#14 0x00007f2b0b945246 in content::ContentMainRunnerImpl::Run() () at ../../content/app/content_main_runner.cc:836
#15 0x00007f2b0b943b69 in content::ContentMain(content::ContentMainParams const&) () at ../../content/app/content_main.cc:20
#16 0x00007f2b09d2e55b in ChromeMain () at ../../chrome/app/chrome_main.cc:113
#17 0x00007f2b077e4796 in __libc_start_main (main=0x7f2b09d2b6e0 <main>, argc=36, argv=0x7ffe178bad78, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>,
stack_end=0x7ffe178bad68) at ../csu/libc-start.c:289
#18 0x00007f2b09d2e389 in _start ()
warning: (Internal error: pc 0x7f2b0b52549c in read in psymtab, but not in symtab.)
I was suspicious of jdufault's CL https://codereview.chromium.org/2654003008/ but it happens even with that reverted.
It crashes calling login_window_->Close();
I wonder if internal widget state is corrupted somehow. Or maybe |this| has been deleted already and |login_window_| is non-null but bogus.
There's some suspicious code in ShutdownDisplayHost that posts a task to delete |this| then drains the message loop:
base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this);
if (post_quit_task)
base::MessageLoop::current()->QuitWhenIdle();
if (!completion_callback_.is_null())
base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
completion_callback_);
pmarko@ also touched it in https://codereview.chromium.org/2652793003 recently but reverting his patch locally also has the same problem.
Maybe the login_window_ views::Widget is in a bad state somehow?
,
Feb 13 2017
It seems that login_window_ would be deleted when the corresponding NativeWidget is closed/destroyed, because widget.h says that ownership = NATIVE_WIDGET_OWNS_WIDGET by default. There seems to be no notification back to LoginDisplayHostImpl that this has happened, so if this happens first, it could explain the crash.
,
Feb 13 2017
I agree. The ownership needs to change, or LoginWidgetDelegate needs to callback to LoginDisplayHostImpl when destroyed.
,
Feb 13 2017
Having said that, the obvious question to ask is what is different about mash vs ash that we're seeing this crash in mash now. I suspect that previously chrome was shutdown, then ash. We likely don't have that invariant now (especially if ash crashes).
,
Feb 13 2017
Yes, it seems likely that the window server is tearing down a window before Chrome shuts down. I'm crashing ash at the login screen, so that explanation fits the repro. I'll take a look.
,
Feb 13 2017
This may tie in with 678683. We could take the signal that a window is deleted unexpectedly to mean ash crashed and that we should kill chrome. When ash crashes mus destroys all the windows ash created, which includes any windows created at the request of chrome. Chrome will receive WindowTreeClient::OnWindowDeleted(). For top-level windows (I believe the login window is a top-level) this goes to MusClient::OnEmbedRootDestroyed(), which calls CloseNow() on the widget. We could instead make MusClient::OnEmbedRootDestroyed() shutdown chrome, at least for the login window.
,
Feb 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b54401a5a1107d8440ddad54effecca920553fa6 commit b54401a5a1107d8440ddad54effecca920553fa6 Author: jamescook <jamescook@chromium.org> Date: Tue Feb 14 00:57:44 2017 mash: Fix content_browser shutdown crash if ash exits at the login screen An ash crash at the login screen tears down all open windows. This can results in the login screen's widget being destroyed before chrome shuts down. Clean up the cached Widget/View pointers when this happens. The chrome --mash root process will subsequently trigger a clean shutdown and restart of content_browser. BUG= 691134 TEST=for classic ash, existing chromeos login tests. For mash, trigger ash crash at login screen and observe browser shut down cleanly. Review-Url: https://codereview.chromium.org/2690283002 Cr-Commit-Position: refs/heads/master@{#450179} [modify] https://crrev.com/b54401a5a1107d8440ddad54effecca920553fa6/chrome/browser/chromeos/login/ui/login_display_host_impl.cc [modify] https://crrev.com/b54401a5a1107d8440ddad54effecca920553fa6/chrome/browser/chromeos/login/ui/login_display_host_impl.h
,
Feb 14 2017
,
Apr 17 2017
,
May 30 2017
,
Aug 1 2017
,
Oct 14 2017
,
Aug 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/adc0e8057d3389c54d498aa5ee38c8305e05ee69 commit adc0e8057d3389c54d498aa5ee38c8305e05ee69 Author: James Cook <jamescook@chromium.org> Date: Mon Aug 27 23:28:11 2018 chromeos: Fix some login animation code for SingleProcessMash Most of the animations are supported. This fixes a large number of kiosk browser_tests under SingleProcessMash. Remove an unnecessary workaround for ash process crashes at the login screen. Now that we don't have a separate window server process an ash crash will result in a clean exit(1) of the browser. There is still one potential issue with animated avatar icons. I'm following up with the graphics guys on that one. Bug: 756071, 613746 , 691134 Test: OOBE/login with SingleProcessMash, browser_tests *Kiosk* Change-Id: Ib72482a3011eabfc6453e7be9e83341063aa4704 Reviewed-on: https://chromium-review.googlesource.com/1191083 Reviewed-by: Xiyuan Xia <xiyuan@chromium.org> Commit-Queue: James Cook <jamescook@chromium.org> Cr-Commit-Position: refs/heads/master@{#586484} [modify] https://crrev.com/adc0e8057d3389c54d498aa5ee38c8305e05ee69/chrome/browser/chromeos/login/ui/login_display_host_webui.cc [modify] https://crrev.com/adc0e8057d3389c54d498aa5ee38c8305e05ee69/chrome/browser/chromeos/login/ui/login_display_host_webui.h [modify] https://crrev.com/adc0e8057d3389c54d498aa5ee38c8305e05ee69/testing/buildbot/filters/chromeos.single_process_mash.fyi.browser_tests.filter
,
Aug 28
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4242b2ed67e24f903cdae610f27963c93daa37d3 commit 4242b2ed67e24f903cdae610f27963c93daa37d3 Author: James Cook <jamescook@chromium.org> Date: Tue Aug 28 21:19:33 2018 Revert "chromeos: Fix some login animation code for SingleProcessMash" This reverts commit adc0e8057d3389c54d498aa5ee38c8305e05ee69. Reason for revert: Broke single_process_mash_fyi_browser_tests https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mojo%20ChromiumOS/34094 UserAddingScreenTest.AddingSeveralUsers PreferencesTestForceWebUiLogin.MultiProfiles ChromeSessionManagerTest.LoginExistingUsers NSSContextChromeOSBrowserTest.TwoUsers SystemTrayClientClockTest.TestMultiProfile24HourClock Original change's description: > chromeos: Fix some login animation code for SingleProcessMash > > Most of the animations are supported. This fixes a large number of > kiosk browser_tests under SingleProcessMash. > > Remove an unnecessary workaround for ash process crashes at the login > screen. Now that we don't have a separate window server process an > ash crash will result in a clean exit(1) of the browser. > > There is still one potential issue with animated avatar icons. I'm > following up with the graphics guys on that one. > > Bug: 756071, 613746 , 691134 > Test: OOBE/login with SingleProcessMash, browser_tests *Kiosk* > Change-Id: Ib72482a3011eabfc6453e7be9e83341063aa4704 > Reviewed-on: https://chromium-review.googlesource.com/1191083 > Reviewed-by: Xiyuan Xia <xiyuan@chromium.org> > Commit-Queue: James Cook <jamescook@chromium.org> > Cr-Commit-Position: refs/heads/master@{#586484} TBR=jamescook@chromium.org,xiyuan@chromium.org Change-Id: Ic2143414a902fee9a44a95db94d682eb79ce493a No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 756071, 613746 , 691134 Reviewed-on: https://chromium-review.googlesource.com/1194867 Reviewed-by: James Cook <jamescook@chromium.org> Commit-Queue: James Cook <jamescook@chromium.org> Cr-Commit-Position: refs/heads/master@{#586854} [modify] https://crrev.com/4242b2ed67e24f903cdae610f27963c93daa37d3/chrome/browser/chromeos/login/ui/login_display_host_webui.cc [modify] https://crrev.com/4242b2ed67e24f903cdae610f27963c93daa37d3/chrome/browser/chromeos/login/ui/login_display_host_webui.h [modify] https://crrev.com/4242b2ed67e24f903cdae610f27963c93daa37d3/testing/buildbot/filters/chromeos.single_process_mash.fyi.browser_tests.filter
,
Aug 29
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9e2d55204d52f26d1779c118468702e8aa3395a5 commit 9e2d55204d52f26d1779c118468702e8aa3395a5 Author: James Cook <jamescook@chromium.org> Date: Wed Aug 29 00:27:08 2018 Reland "chromeos: Fix some login animation code for SingleProcessMash" This reverts commit 4242b2ed67e24f903cdae610f27963c93daa37d3. Fixed by skipping login animation when adding a secondary user in a multiprofile session. Neither SingleProcessMash nor MultiProcessMash support MultiUserWindowManagerChromeOS, but the login code depends on it to complete animations for adding new users. Original change's description: > Revert "chromeos: Fix some login animation code for SingleProcessMash" > > This reverts commit adc0e8057d3389c54d498aa5ee38c8305e05ee69. > > Reason for revert: Broke single_process_mash_fyi_browser_tests > > https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mojo%20ChromiumOS/34094 > > UserAddingScreenTest.AddingSeveralUsers > PreferencesTestForceWebUiLogin.MultiProfiles > ChromeSessionManagerTest.LoginExistingUsers > NSSContextChromeOSBrowserTest.TwoUsers > SystemTrayClientClockTest.TestMultiProfile24HourClock > > Original change's description: > > chromeos: Fix some login animation code for SingleProcessMash > > > > Most of the animations are supported. This fixes a large number of > > kiosk browser_tests under SingleProcessMash. > > > > Remove an unnecessary workaround for ash process crashes at the login > > screen. Now that we don't have a separate window server process an > > ash crash will result in a clean exit(1) of the browser. > > > > There is still one potential issue with animated avatar icons. I'm > > following up with the graphics guys on that one. > > > > Bug: 756071, 613746 , 691134 > > Test: OOBE/login with SingleProcessMash, browser_tests *Kiosk* > > Change-Id: Ib72482a3011eabfc6453e7be9e83341063aa4704 > > Reviewed-on: https://chromium-review.googlesource.com/1191083 > > Reviewed-by: Xiyuan Xia <xiyuan@chromium.org> > > Commit-Queue: James Cook <jamescook@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#586484} > > TBR=jamescook@chromium.org,xiyuan@chromium.org > > Change-Id: Ic2143414a902fee9a44a95db94d682eb79ce493a > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: 756071, 613746 , 691134 > Reviewed-on: https://chromium-review.googlesource.com/1194867 > Reviewed-by: James Cook <jamescook@chromium.org> > Commit-Queue: James Cook <jamescook@chromium.org> > Cr-Commit-Position: refs/heads/master@{#586854} Bug: 756071, 613746 , 691134 Change-Id: Ie571456b9e242c70f38cf4771c1afaa3f66cb020 Reviewed-on: https://chromium-review.googlesource.com/1194833 Reviewed-by: Xiyuan Xia <xiyuan@chromium.org> Reviewed-by: James Cook <jamescook@chromium.org> Commit-Queue: James Cook <jamescook@chromium.org> Cr-Commit-Position: refs/heads/master@{#586962} [modify] https://crrev.com/9e2d55204d52f26d1779c118468702e8aa3395a5/chrome/browser/chromeos/login/ui/login_display_host_webui.cc [modify] https://crrev.com/9e2d55204d52f26d1779c118468702e8aa3395a5/chrome/browser/chromeos/login/ui/login_display_host_webui.h [modify] https://crrev.com/9e2d55204d52f26d1779c118468702e8aa3395a5/chrome/browser/ui/ash/multi_user/multi_user_window_manager.cc [modify] https://crrev.com/9e2d55204d52f26d1779c118468702e8aa3395a5/testing/buildbot/filters/chromeos.single_process_mash.fyi.browser_tests.filter |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by jamescook@chromium.org
, Feb 10 2017