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

Issue 703510 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug



Sign in to add a comment

Crash in ImeController::CanCycleIme

Project Member Reported by ClusterFuzz, Mar 21 2017

Issue description

Cc: afakhry@chromium.org alemate@chromium.org shuchen@chromium.org osh...@chromium.org warx@chromium.org
Labels: Test-Predator-Wrong M-59
Looks like this is similar to  issue 696329 , could you please take a look and duplicate.
Thank you.
Cc: pkasting@chromium.org msw@chromium.org
+ShowMessageBoxImpl owners.

What happens is ExtensionErrorReporter::ReportLoadError() .
Which turns into ShowMessageBox and then crashes in ShowMessageBoxImpl because it will run the same message loop during initialization.

+msw, +pkasting, what do you think? I don't think running event loop is safe there.


	Received signal 11 SEGV_MAPERR 000000000000
#0 0x7fd51ef922d1 __interceptor_backtrace
#1 0x7fd526511f7e base::debug::StackTrace::StackTrace()
#2 0x7fd526510e1d base::debug::(anonymous namespace)::StackDumpSignalHandler()
#3 0x7fd5178ea340 <unknown>
#4 0x7fd52e5ecd80 ImeController::CanCycleIme()
#5 0x7fd52d63f106 ash::AcceleratorController::AcceleratorPressed()
#6 0x7fd5352eb6af ui::AcceleratorManager::Process()
#7 0x7fd5362fae9c wm::AcceleratorFilter::OnKeyEvent()
#8 0x7fd528b46697 ui::EventDispatcher::DispatchEventToEventHandlers()
#9 0x7fd528b45463 ui::EventDispatcher::ProcessEvent()
#10 0x7fd528b44f7b ui::EventDispatcherDelegate::DispatchEvent()
#11 0x7fd5352fc0fb ui::EventProcessor::OnEventFromSource()
#12 0x7fd52d804913 ash::AshWindowTreeHostX11::DispatchKeyEventPostIME()
#13 0x7fd535d6e1a0 ui::InputMethodBase::DispatchKeyEventPostIME()
#14 0x7fd535d73663 ui::InputMethodChromeOS::DispatchKeyEvent()
#15 0x7fd535d744b9 ui::InputMethodChromeOS::DispatchKeyEvent()
#16 0x7fd52d806bf1 ash::InputMethodEventHandler::OnKeyEvent()
#17 0x7fd528b46697 ui::EventDispatcher::DispatchEventToEventHandlers()
#18 0x7fd528b45463 ui::EventDispatcher::ProcessEvent()
#19 0x7fd528b44f7b ui::EventDispatcherDelegate::DispatchEvent()
#20 0x7fd5352fc0fb ui::EventProcessor::OnEventFromSource()
#21 0x7fd528b4912c ui::EventSource::SendEventToProcessor()
#22 0x7fd535f7fee1 aura::WindowTreeHostX11::DispatchEvent()
#23 0x7fd528b1ecdb ui::PlatformEventSource::DispatchEvent()
#24 0x7fd528c4b9c0 ui::X11EventSource::ExtractCookieDataDispatchEvent()
#25 0x7fd528c4b70c ui::X11EventSource::DispatchXEvents()
#26 0x7fd528c4afbd ui::(anonymous namespace)::XSourceDispatch()
#27 0x7fd51a700ce5 g_main_context_dispatch
#28 0x7fd51a701048 <unknown>
#29 0x7fd51a7010ec g_main_context_iteration
#30 0x7fd5265895a4 base::MessagePumpGlib::Run()
#31 0x7fd52657e989 base::MessageLoop::RunHandler()
#32 0x7fd52660cd5e base::RunLoop::Run()
#33 0x7fd52e0a8e62 chrome::(anonymous namespace)::ShowMessageBoxImpl()
#34 0x7fd52e0a8272 chrome::ShowWarningMessageBox()
#35 0x7fd52c4801d7 ExtensionErrorReporter::ReportError()
#36 0x7fd52c47f33b ExtensionErrorReporter::ReportLoadError()
#37 0x7fd52c58e491 extensions::UnpackedInstaller::ReportExtensionLoadError()
#38 0x7fd52c58ddb1 extensions::UnpackedInstaller::LoadFromCommandLine()
#39 0x7fd52c4af9f7 ExtensionService::LoadExtensionsFromCommandLineFlag()
#40 0x7fd52c4aee88 ExtensionService::Init()
#41 0x7fd52c4e70e1 extensions::ExtensionSystemImpl::Shared::Init()
#42 0x7fd52c4e81b5 extensions::ExtensionSystemImpl::InitForRegularProfile()
#43 0x7fd5259d333d ProfileManager::DoFinalInitForServices()
#44 0x7fd5259d2d6b ProfileManager::DoFinalInit()
#45 0x7fd5259d6112 ProfileManager::AddProfile()
#46 0x7fd5259c2556 ProfileManager::CreateAndInitializeProfile()
#47 0x7fd5259c1aff ProfileManager::GetProfile()
#48 0x7fd5259c12bc ProfileManager::GetActiveUserOrOffTheRecordProfileFromPath()
#49 0x7fd5259c16db ProfileManager::GetActiveUserProfile()
#50 0x7fd52578f846 ChromeBrowserMainParts::PreMainMessageLoopRunImpl()
#51 0x7fd52578dcc6 ChromeBrowserMainParts::PreMainMessageLoopRun()
#52 0x7fd5241960a3 chromeos::ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun()
#53 0x7fd52044499f content::BrowserMainLoop::PreMainMessageLoopRun()
#54 0x7fd52110d9ad content::StartupTaskRunner::RunAllTasksNow()
#55 0x7fd52043e6b2 content::BrowserMainLoop::CreateStartupTasks()
#56 0x7fd5204530b3 content::BrowserMainRunnerImpl::Initialize()
#57 0x7fd520435601 content::BrowserMain()
#58 0x7fd5256160af content::ContentMainRunnerImpl::Run()
#59 0x7fd525611612 content::ContentMain()
#60 0x7fd51f01b9f2 ChromeMain

Showing a message box has to spin an event loop, or it can't detect the button press.

Why is this crashing when it's in a nested message loop?
> Why is this crashing when it's in a nested message loop?

I think this is because ProfileManager::CreateAndInitializeProfile() (incl. everything that is called from it) doesn't expect the run loop to spin while it is running.

Could we do PostTask(Bind(&ShowMessageBoxImpl)) inside ShowWarningMessageBox() instead of direct call?
Maybe; we could also do something like using a different surface (e.g. an infobar).  One of the things I'm worried about is other message box users getting into similar trouble, so I wouldn't want to just fix by avoiding the call here if some other caller will trip the same bug.

What part of CreateAndInitializeProfile() is breaking with this?  Basically, what I'm really wanting to know is precisely what kind of unsafe data manipulation (or whatever) is occurring here, in detail.
That's a tricky question as it is difficult to traverse a whole call tree inside CreateAndInitializeProfile(). I think there are numerous cases.

The stack trace above suggests that we cannot handle user input events until profile is fully initialized. One of the reasons is that IME is implemented as extension, so it cannot be used until extension initialization code is completed.

We also cannot handle user input events because they should be targeted to the active user, but no active user exists before its profile is initialized (for example, we need mapping User <-> Profile for many ChromeOS services).

I also see that Profile attributes are initialized after profile has been added to the storage, and thus "ephemeral" flag may not be set for the profile.

I guess I was more hoping to know what the null pointer near CanCycleIme() was about.  Sounds like maybe the issue is that extension init hasn't actually finished yet, so we can't safely access extensions, but the accelerator handling code assumes it can access the IME, which is implemented as an extension.

If that's correct, it's hard to know whether there's something wrong with that (for example, having IME be an extension at all sounds suspicious to me), but it may be reasonable to post a task to show this message box, rather than doing it synchronously.
...That said, I kinda wonder whether all warning message boxes shouldn't be shown off posted tasks in that case, for safety's sake?  Are there any call sites that want to show a warning and then go ahead and do something that we really need to show the warning in advance of doing?
> I guess I was more hoping to know what the null pointer near CanCycleIme() was about.

It is a pointer to current state of Input Methods Engine. To create one we need IME initialized.


Side-note: Actually IME is a very specific extension. It is component extension and thus built-in. But it still requires initialization (and some times it depends on external files). Also, the API is extension-based, so we need extensions initialized before using it. That is why IME::State is bound to the user profile.

Also I remember other issues with running loops inside CreateProfile (sometimes it leads to recursive CreateProfile). So I would prefer to never display messaged inside profile initialization. We can either implement a delayed error messages in extensions initialization, or to always run error popup as separate task via PostTask(THREAD_UI). I think that PostTask in ShowWarningMessageBox() is simpler ;)

But I am not aware of other usage scenarios of ShowWarningMessageBox() that may require existing behavior.
Right, we'd have to audit if we wanted to make this change for all callers.
Owner: pkasting@chromium.org
Status: Assigned (was: Untriaged)
We need this fixed. There are more crashes like this, see  Issue 696329  for example.
Cc: rdevlin....@chromium.org
 Issue 696329  has been merged into this issue.
Owner: alemate@chromium.org
I don't have bandwidth to work on this myself.  My hope above was to ask the right questions to guide you to the right design for a fix.

If you can't implement this, please consult an appropriate PM to get help scheduling the work with someone.
Owner: wzang@chromium.org
Project Member

Comment 15 by bugdroid1@chromium.org, Apr 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f8c865910c21c379df39eb057530e13630d2f00d

commit f8c865910c21c379df39eb057530e13630d2f00d
Author: wzang <wzang@chromium.org>
Date: Fri Apr 28 01:36:26 2017

Check IME state in ImeController

ImeController::CanCycleIme() and ImeController::CanSwitchIme() should first
check if IME state is NULL and if it is, ignore the cycle / switch action
because it does not make sense before IME initialization.

BUG= 703510 

Review-Url: https://codereview.chromium.org/2806613002
Cr-Commit-Position: refs/heads/master@{#467804}

[modify] https://crrev.com/f8c865910c21c379df39eb057530e13630d2f00d/chrome/browser/ui/ash/ime_controller_chromeos.cc

Comment 16 by wzang@chromium.org, Apr 28 2017

Status: Fixed (was: Assigned)
Project Member

Comment 17 by ClusterFuzz, Apr 28 2017

ClusterFuzz has detected this issue as fixed in range 467762:467817.

Detailed report: https://clusterfuzz.com/testcase?key=6071399941931008

Fuzzer: meacer_chromebot_extensions
Job Type: linux_asan_chrome_chromeos
Platform Id: linux

Crash Type: UNKNOWN
Crash Address: 0x000000000000
Crash State:
  ImeController::CanCycleIme
  ash::AcceleratorController::AcceleratorPressed
  ui::AcceleratorManager::Process
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_chromeos&range=458162:458196
Fixed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_chromeos&range=467762:467817

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6071399941931008


Additional requirements: Requires Gestures

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

Sign in to add a comment