New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 763493 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
inactive
Closed: Nov 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocking:
issue 731255



Sign in to add a comment

mus: OzoneCursorLoader/CursorFactoryOzone thread problems

Project Member Reported by kylec...@chromium.org, Sep 8 2017

Issue description

I 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>
 

Comment 1 by sky@chromium.org, Sep 8 2017

Owner: e...@chromium.org
Status: Assigned (was: Available)

Comment 2 by sky@chromium.org, Sep 8 2017

Blocking: 731255

Comment 3 by e...@chromium.org, Oct 12 2017

Owner: mfomitchev@chromium.org
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?
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

Could you get a stack trace for CreateAnimatedCursor on mus? This seems wrong.
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

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.
Ah yes, right, thanks. Looks like this needs to be fixed. Sigh..

Comment 10 by sky@chromium.org, Oct 30 2017

mfomitchev, are you actively working on this?
Not at the moment, working on  issue 717629  now.
Status: Started (was: Assigned)
Project Member

Comment 13 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)

Comment 15 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Comment 16 by dchan@chromium.org, Jan 23 2018

Status: Fixed (was: Archived)

Sign in to add a comment