mus: OzoneCursorLoader/CursorFactoryOzone thread problems |
|||||||
Issue descriptionI was looking into a different cursor bug and noticed that when running ./chrome --mus CursorFactoryOzone is accessed from multiple threads. The easiest way to verify this is to put a ThreadChecker in X11CursorFactoryOzone. I think this is related to OzoneCursorFactory having some thread problems? There are two instances of it. One is created/destroyed by ash on the same thread. That's fine. The second is created on the WS thread and destroyed on the chrome thread? I have verified the constructor and destructor are running on separate threads for the stack trace below. Constructor in WS: #0 0x7f8e6fca8f6c base::debug::StackTrace::StackTrace() #1 0x7f8e700db818 ui::CursorLoaderOzone::CursorLoaderOzone() #2 0x7f8e700dbecb ui::CursorLoader::Create() #3 0x7f8e700d944b ui::ImageCursors::Initialize() #4 0x558b433ff695 ui::ws::ThreadedImageCursors::ThreadedImageCursors() #5 0x558b41a2063a ui::(anonymous namespace)::ThreadedImageCursorsFactoryImpl::CreateCursors() #6 0x558b4342a1c4 ui::ws::PlatformDisplay::Create() #7 0x558b434219ce ui::ws::Display::Init() #8 0x558b433fc105 ui::ws::DisplayManager::OnDisplayAdded() #9 0x558b433fb5e7 ui::ws::DisplayManager::AddDisplayForWindowManager() #10 0x558b4340f9ef ui::ws::WindowTree::ProcessSetDisplayRoot() #11 0x558b4341ba9e ui::ws::WindowTree::SetDisplayRoot() #12 0x558b41465ea6 ui::mojom::WindowManagerClientStubDispatch::AcceptWithResponder() #13 0x558b4341f026 ui::mojom::WindowManagerClientStub<>::AcceptWithResponder() #14 0x7f8e6ef0a11c mojo::InterfaceEndpointClient::HandleValidatedMessage() #15 0x7f8e6ef09a76 mojo::FilterChain::Accept() #16 0x7f8e6ef0b3b5 mojo::InterfaceEndpointClient::HandleIncomingMessage() #17 0x7f8e6ef14b69 mojo::internal::MultiplexRouter::ProcessIncomingMessage() #18 0x7f8e6ef14087 mojo::internal::MultiplexRouter::Accept() #19 0x7f8e6ef09a76 mojo::FilterChain::Accept() #20 0x7f8e6ef04c85 mojo::Connector::ReadSingleMessage() #21 0x7f8e6ef05761 mojo::Connector::ReadAllAvailableMessages() #22 0x7f8e6ef05619 mojo::Connector::OnHandleReadyInternal() #23 0x7f8e6ef05db5 mojo::SimpleWatcher::DiscardReadyState() #24 0x7f8e6eedcf04 mojo::SimpleWatcher::OnHandleReady() #25 0x7f8e6eedd44f _ZN4base8internal7InvokerINS0_9BindStateIMN4mojo13SimpleWatcherEFvijRKNS3_18HandleSignalsStateEEJNS_7WeakPtrIS4_EEijS5_EEEFvvEE7RunImplIRKS9_RKNSt3__15tupleIJSB_ijS5_EEEJLm0ELm1ELm2ELm3EEEEvOT_OT0_NSI_16integer_sequenceImJXspT1_EEEE #26 0x7f8e6fca9818 base::debug::TaskAnnotator::RunTask() #27 0x7f8e6fcd8b25 base::internal::IncomingTaskQueue::RunTask() #28 0x7f8e6fcdacf2 base::MessageLoop::RunTask() #29 0x7f8e6fcdb40c base::MessageLoop::DoWork() #30 0x7f8e6fcddc99 base::MessagePumpLibevent::Run() #31 0x7f8e6fcda802 base::MessageLoop::Run() #32 0x7f8e6fd0e3ca base::RunLoop::Run() #33 0x7f8e6fd4b2e7 base::Thread::Run() #34 0x7f8e6fd4b89b base::Thread::ThreadMain() #35 0x7f8e6fd42cff base::(anonymous namespace)::ThreadFunc() #36 0x7f8e6fe22184 start_thread #37 0x7f8e63fbeffd clone Destructor in content: #0 0x7f8e6fca8f6c base::debug::StackTrace::StackTrace() #1 0x7f8e700db91b ui::CursorLoaderOzone::~CursorLoaderOzone() #2 0x7f8e700dba0e ui::CursorLoaderOzone::~CursorLoaderOzone() #3 0x558b418edf62 std::__1::__tree<>::destroy() #4 0x558b41a4581f BrowserProcessPlatformPart::DestroyImageCursorsSet() #5 0x558b42efa6d1 AshInit::~AshInit() #6 0x558b42d814d1 ChromeBrowserMainExtraPartsAsh::PostMainMessageLoopRun() #7 0x558b41a4c9ea ChromeBrowserMainParts::PostMainMessageLoopRun() #8 0x558b416393f3 chromeos::ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun() #9 0x7f8e6d15e9a2 content::BrowserMainLoop::ShutdownThreadsAndCleanUp() #10 0x7f8e6d1614b3 content::BrowserMainRunnerImpl::Shutdown() #11 0x7f8e6d159e0b content::BrowserMain() #12 0x7f8e6d9b80e8 content::ContentMainRunnerImpl::Run() #13 0x7f8e701f3071 service_manager::Main() #14 0x7f8e6d9b6af4 content::ContentMain() #15 0x558b4106a5a8 ChromeMain #16 0x7f8e63ee2f45 __libc_start_main #17 0x558b4106a3e4 <unknown>
,
Sep 8 2017
,
Oct 12 2017
,
Oct 12 2017
kylechar@: We need to create CursorLoader and the associated factory on the WS thread. The ownership of the loader then needs to be transferred to the browser thread. This is because we cannot access the ResourceBundle on the WS thread, and so we need to do it on the browser thread. Now, we could move CursorLoader back to the WS thread before destroying it, but that would add more complexity. Why is it this is causing a problem? OzoneCursorFactory itself isn't (shouldn't be?) destroyed on the brwoser thread, only the CursorLoaders are. CursorLoader are never accessed on the WS thread, they are only constructed there. That seems ok, no?
,
Oct 12 2017
The conceptual ownership sounds fine. It's the access of X11CursorFactoryOzone from multiple threads that worries me. I put a couple LOG(ERROR) statements that printed |this| and base::MessageLoop::GetThreadName() in X11CursorFactoryOzone. Both mus and CrBrowserMain thread access the same object by the looks of things? [15429:15475:1012/143649.365060:ERROR:x11_cursor_factory_ozone.cc(81)] X11CursorFactoryOzone[0x214228eec810] CreateAnimatedCursor on mus [15429:15429:1012/143649.501682:ERROR:x11_cursor_factory_ozone.cc(51)] X11CursorFactoryOzone[0x214228eec810] GetDefaultCursor on CrBrowserMain [15429:15429:1012/143649.501820:ERROR:x11_cursor_factory_ozone.cc(51)] X11CursorFactoryOzone[0x214228eec810] GetDefaultCursor on CrBrowserMain [15429:15475:1012/143649.505839:ERROR:x11_cursor_factory_ozone.cc(81)] X11CursorFactoryOzone[0x214228eec810] CreateAnimatedCursor on mus
,
Oct 12 2017
Could you get a stack trace for CreateAnimatedCursor on mus? This seems wrong.
,
Oct 12 2017
Ok, I see what's going on there. I've pasted the stack trace below. In order to fix this, I guess we'd need to create a copy of the CursorData and pass it over to the browser thread...
Is the way it's now going to lead to problems though? I might be missing something - the code in CreateAnimatedCursor seems like it should be thread safe, no?
#2 0x7f0014e4bd0e in ui::X11CursorFactoryOzone::CreateAnimatedCursor(std::__1::vector<SkBitmap, std::__1::allocator<SkBitmap> > const&, gfx::Point const&, int, float) ui/ozone/platform/x11/x11_cursor_factory_ozone.cc:69:3
#3 0x564658fd9298 in ui::ws::PlatformDisplayDefault::SetCursor(ui::CursorData const&) services/ui/ws/platform_display_default.cc:129:53
#4 0x564658fb0836 in operator() services/ui/ws/cursor_state.cc:155:16
#5 0x564658fb0836 in ui::ws::CursorState::SetPlatformCursor() services/ui/ws/cursor_state.cc:160:0
#6 0x564658f9cd14 in WmSetGlobalOverrideCursor services/ui/ws/window_tree.cc:2524:41
#7 0x564658f9cd14 in non-virtual thunk to ui::ws::WindowTree::WmSetGlobalOverrideCursor(base::Optional<ui::CursorData>) services/ui/ws/window_tree.cc:0:0
,
Oct 12 2017
I don't think RefCountedBase<T> is thread safe? Beyond that I'm not really sure, I just noticed the weird thread access in the cursor code under mushrome.
,
Oct 12 2017
Ah yes, right, thanks. Looks like this needs to be fixed. Sigh..
,
Oct 30 2017
mfomitchev, are you actively working on this?
,
Oct 30 2017
Not at the moment, working on issue 717629 now.
,
Nov 15 2017
,
Nov 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/09c6e5e2459ca38de402a3cf6b5b367b0145f84c commit 09c6e5e2459ca38de402a3cf6b5b367b0145f84c Author: Mikhail Fomitchev <mfomitchev@chromium.org> Date: Wed Nov 15 23:22:13 2017 Ensure CursorFactoryOzone is only accessed on the browser thread. CursorFactoryOzone implementations use ref-counting and not thread-safe. Therefore, we should only access CursorFactoryOzone on a single thread. Since we may need to load resources as part of cursor loading, this thread should be the browser thread. This CL moves the call to CursorFactoryOzone::CreateAnimatedCursor from the Mus thread to the browser thread. Bug: 763493 Change-Id: Id046ea98b2ef26fbb46993b77607555dc9def2d1 Reviewed-on: https://chromium-review.googlesource.com/768852 Reviewed-by: Scott Violet <sky@chromium.org> Commit-Queue: Mikhail Fomitchev <mfomitchev@chromium.org> Cr-Commit-Position: refs/heads/master@{#516883} [modify] https://crrev.com/09c6e5e2459ca38de402a3cf6b5b367b0145f84c/services/ui/ws/platform_display_default.cc [modify] https://crrev.com/09c6e5e2459ca38de402a3cf6b5b367b0145f84c/services/ui/ws/threaded_image_cursors.cc [modify] https://crrev.com/09c6e5e2459ca38de402a3cf6b5b367b0145f84c/services/ui/ws/threaded_image_cursors.h
,
Nov 15 2017
,
Jan 22 2018
,
Jan 23 2018
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by sky@chromium.org
, Sep 8 2017Status: Assigned (was: Available)