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

Issue 794581 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

View keyboard shortcuts crashes

Project Member Reported by chiniforooshan@chromium.org, Dec 13 2017

Issue description

Chrome Version: 65.0.3293.0 (64-bit)
OS: CrOS on Linux (did not test on a Chromebook)

What steps will reproduce the problem?
(1) Go to Settings > Keyboard > View keyboard shortcuts

What is the expected result?
See keyboard shortcuts

What happens instead?
Crashes with:

[226486:226503:1213/110420.573237:ERROR:logging_chrome.cc(210)] Unable to create symlink /usr/local/google/home/chiniforooshan/.config/chromium/test-user/chrome_debug.log pointing at /usr/local/google/home/chiniforooshan/.config/chromium/test-user/chrome_debug_20171213-110420: File exists (17)
[226486:226486:1213/110423.847387:ERROR:tab_manager_delegate_chromeos.cc(82)] Set OOM score error: 
[226486:226486:1213/110427.015028:ERROR:tab_manager_delegate_chromeos.cc(82)] Set OOM score error: 
[226486:226486:1213/110540.750865:ERROR:tab_manager_delegate_chromeos.cc(82)] Set OOM score error: 
[226486:226486:1213/110540.750942:ERROR:tab_manager_delegate_chromeos.cc(82)] Set OOM score error: 
[226486:226486:1213/110648.281950:FATAL:binding_state.cc(90)] Check failed: !router_. 
#0 0x7f2dd824915c base::debug::StackTrace::StackTrace()
#1 0x7f2dd826fbcc logging::LogMessage::~LogMessage()
#2 0x7f2dd755eede mojo::internal::BindingStateBase::BindInternal()
#3 0x7f2dd2605596 mojo::internal::BindingState<>::Bind()
#4 0x7f2dd26051cb ash::NewWindowController::BindRequest()
#5 0x7f2dd2602289 ash::mojo_interface_factory::(anonymous namespace)::BindNewWindowControllerRequestOnMainThread()
#6 0x7f2dd26034fd _ZN4base8internal7InvokerINS0_9BindStateIPFvN4mojo16InterfaceRequestIN3ash5mojom10CastConfigEEEEJEEES9_E3RunEPNS0_13BindStateBaseEOS8_
#7 0x7f2dd2603bfe _ZN15service_manager14CallbackBinderIN3ash5mojom10CastConfigEJEE11RunCallbackERKN4base17RepeatingCallbackIFvN4mojo16InterfaceRequestIS3_EEEEES9_
#8 0x7f2dd2603c58 _ZN4base8internal7InvokerINS0_9BindStateIPFvRKNS_17RepeatingCallbackIFvN4mojo16InterfaceRequestIN3ash5mojom10CastConfigEEEEEES9_EJSB_S9_EEEFvvEE7RunOnceEPNS0_13BindStateBaseE
#9 0x7f2dd8249a35 base::debug::TaskAnnotator::RunTask()
#10 0x7f2dd827a039 base::internal::IncomingTaskQueue::RunTask()
#11 0x7f2dd827db3b base::MessageLoop::RunTask()
#12 0x7f2dd827ded3 base::MessageLoop::DeferOrRunPendingTask()
#13 0x7f2dd827e166 base::MessageLoop::DoWork()
#14 0x7f2dd8280699 base::MessagePumpLibevent::Run()
#15 0x7f2dd827d439 base::MessageLoop::Run()
#16 0x7f2dd82b1ba9 base::RunLoop::Run()
#17 0x561744a8841a ChromeBrowserMainParts::MainMessageLoopRun()
#18 0x7f2dd559b2a7 content::BrowserMainLoop::RunMainMessageLoopParts()
#19 0x7f2dd559e4a6 content::BrowserMainRunnerImpl::Run()
#20 0x7f2dd559732a content::BrowserMain()
#21 0x7f2dd5f622a1 content::ContentMainRunnerImpl::Run()
#22 0x7f2dd8795edb service_manager::Main()
#23 0x7f2dd5f60c84 content::ContentMain()
#24 0x561743ffec3f ChromeMain
#25 0x7f2dcb7eef45 __libc_start_main
#26 0x561743ffe91a _start

Received signal 6
#0 0x7f2dd824915c base::debug::StackTrace::StackTrace()
#1 0x7f2dd8248c51 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7f2dd83d0330 <unknown>
#3 0x7f2dcb803c37 gsignal
#4 0x7f2dcb807028 abort
#5 0x7f2dd8246615 base::debug::BreakDebugger()
#6 0x7f2dd826ff6e logging::LogMessage::~LogMessage()
#7 0x7f2dd755eede mojo::internal::BindingStateBase::BindInternal()
#8 0x7f2dd2605596 mojo::internal::BindingState<>::Bind()
#9 0x7f2dd26051cb ash::NewWindowController::BindRequest()
#10 0x7f2dd2602289 ash::mojo_interface_factory::(anonymous namespace)::BindNewWindowControllerRequestOnMainThread()
#11 0x7f2dd26034fd _ZN4base8internal7InvokerINS0_9BindStateIPFvN4mojo16InterfaceRequestIN3ash5mojom10CastConfigEEEEJEEES9_E3RunEPNS0_13BindStateBaseEOS8_
#12 0x7f2dd2603bfe _ZN15service_manager14CallbackBinderIN3ash5mojom10CastConfigEJEE11RunCallbackERKN4base17RepeatingCallbackIFvN4mojo16InterfaceRequestIS3_EEEEES9_
#13 0x7f2dd2603c58 _ZN4base8internal7InvokerINS0_9BindStateIPFvRKNS_17RepeatingCallbackIFvN4mojo16InterfaceRequestIN3ash5mojom10CastConfigEEEEEES9_EJSB_S9_EEEFvvEE7RunOnceEPNS0_13BindStateBaseE
#14 0x7f2dd8249a35 base::debug::TaskAnnotator::RunTask()
#15 0x7f2dd827a039 base::internal::IncomingTaskQueue::RunTask()
#16 0x7f2dd827db3b base::MessageLoop::RunTask()
#17 0x7f2dd827ded3 base::MessageLoop::DeferOrRunPendingTask()
#18 0x7f2dd827e166 base::MessageLoop::DoWork()
#19 0x7f2dd8280699 base::MessagePumpLibevent::Run()
#20 0x7f2dd827d439 base::MessageLoop::Run()
#21 0x7f2dd82b1ba9 base::RunLoop::Run()
#22 0x561744a8841a ChromeBrowserMainParts::MainMessageLoopRun()
#23 0x7f2dd559b2a7 content::BrowserMainLoop::RunMainMessageLoopParts()
#24 0x7f2dd559e4a6 content::BrowserMainRunnerImpl::Run()
#25 0x7f2dd559732a content::BrowserMain()
#26 0x7f2dd5f622a1 content::ContentMainRunnerImpl::Run()
#27 0x7f2dd8795edb service_manager::Main()
#28 0x7f2dd5f60c84 content::ContentMain()
#29 0x561743ffec3f ChromeMain
#30 0x7f2dcb7eef45 __libc_start_main
#31 0x561743ffe91a _start
  r8: ffffbac14c007b40  r9: ffffbac14c007b30 r10: 0000000000000008 r11: 0000000000000202
 r12: 00007ffe1829b998 r13: 00007ffe1829b988 r14: 00007ffe1829b990 r15: 00007ffe1829b4e0
  di: 00000000000374b6  si: 00000000000374b6  bp: 00007ffe1829b4d0  bx: 00007ffe1829b4e0
  dx: 0000000000000006  ax: 0000000000000000  cx: 00007f2dcb803c37  sp: 00007ffe1829b398
  ip: 00007f2dcb803c37 efl: 0000000000000202 cgf: 0000000000000033 erf: 0000000000000000
 trp: 0000000000000000 msk: 0000000000000000 cr2: 0000000000000000
[end of stack trace]
Calling _exit(1). Core file will not be generated.

My args.gn:

use_goma=true
is_component_build=true
is_debug=false
use_ozone=true
target_os="chromeos"
dcheck_always_on=true
enable_nacl=false
 
Cc: e...@chromium.org
Labels: -Pri-3 M-65 Pri-1
Owner: est...@chromium.org
Status: Assigned (was: Untriaged)
estade, could this be related to your CL https://chromium-review.googlesource.com/c/chromium/src/+/696286 ?

P1 for user-reproducible crash. If this happens in M64 it will need backport.

Repros for me on linux-chromeos at r523466 from yesterday.

My guess is this is triggered from c/b/ui/webui/settings/chromeos/device_keyboard_handler.cc

void KeyboardHandler::HandleShowKeyboardShortcutsOverlay(
    const base::ListValue* args) const {
  ash::mojom::NewWindowControllerPtr new_window_controller;
  content::ServiceManagerConnection::GetForProcess()
      ->GetConnector()
      ->BindInterface(ash::mojom::kServiceName, &new_window_controller);
  new_window_controller->ShowKeyboardOverlay();
}

Maybe this is the first place to bind the interface? But that doesn't make a lot of sense. I'm not sure what's going on here.

Comment 2 by est...@chromium.org, Dec 13 2017

I will take a look this afternoon.
Cc: -jamescook@chromium.org est...@chromium.org
Labels: M-64
Owner: jamescook@chromium.org
Status: Started (was: Assigned)
estade, I can take this. As you discovered, it's my fault, due to my change from BindingSet to Binding in ash. https://chromium-review.googlesource.com/c/chromium/src/+/766649

I'm going to partially revert that CL. I think we should backport that fix to M-64, as I introduced the crash before the branch point. This will require a manual merge, but I can take care of that.

Project Member

Comment 4 by bugdroid1@chromium.org, Dec 14 2017

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

commit 3918d7bb6217430a67b1424b7a8a0960b2048c62
Author: James Cook <jamescook@chromium.org>
Date: Thu Dec 14 01:23:05 2017

cros: Fix crash when opening keyboard shortcut overlay from settings

Crash was introduced in commit 8f1e606d49b0b2cea3a0c0796a912f50171e7173

More than one part of chrome calls mojo methods in NewWindowController,
so there can be more than one binding. I've audited the other changes
in the above CL and they should be OK, so keeping this change small
for easier backport.

Bug:  794581 
Test: ash_unittests, chrome unit_tests and browser_tests, manually open keyboard shortcut overlay from webui settings > keyboard section
Change-Id: I6b274ecd7b8e6c0c366c36a398e9094b4c361dd9
Reviewed-on: https://chromium-review.googlesource.com/826026
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523969}
[modify] https://crrev.com/3918d7bb6217430a67b1424b7a8a0960b2048c62/ash/new_window_controller.cc
[modify] https://crrev.com/3918d7bb6217430a67b1424b7a8a0960b2048c62/ash/new_window_controller.h

Project Member

Comment 5 by bugdroid1@chromium.org, Dec 14 2017

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

commit ede316aa6bbb83e9c6c6b9df012041ab160d1159
Author: James Cook <jamescook@chromium.org>
Date: Thu Dec 14 22:38:43 2017

Revert: cros: Replace some use of mojo::BindingSet in ash with Binding

Partial revert of commit 8f1e606d49b0b2cea3a0c0796a912f50171e7173,
which introduced a crash in ash when opening the keyboard shortcuts
viewer from the keyboard settings webui. The code that caused the crash
was reverted separately to make it easier to backport.

More generally, any mojo interface Foo in ash that has both a persistent
client (a SetClient() method) and other methods (a DoBar() method)
might end up with multiple bindings:
* The persistent binding from FooClient in chrome
* A temporary binding from other code in chrome that happens to
request the Foo interface and call DoBar

I've reverted most of the places I changed, since new code might add
calls to DoBar() methods outside of FooClient, and in general I think
we should support that use case. (Otherwise we have to route all
calls in chrome through FooClient::Get()->DoBar() wrapper methods,
which isn't desirable in general.)

Bug:  794581 
Test: ash_unittests, chrome unit_tests and browser_tests, manually open keyboard shortcut overlay from webui settings > keyboard section
Change-Id: I3d974b459f60f7a720d7d345862af8197ddb37df
Reviewed-on: https://chromium-review.googlesource.com/825959
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524207}
[modify] https://crrev.com/ede316aa6bbb83e9c6c6b9df012041ab160d1159/ash/cast_config_controller.cc
[modify] https://crrev.com/ede316aa6bbb83e9c6c6b9df012041ab160d1159/ash/cast_config_controller.h
[modify] https://crrev.com/ede316aa6bbb83e9c6c6b9df012041ab160d1159/ash/ime/ime_controller.cc
[modify] https://crrev.com/ede316aa6bbb83e9c6c6b9df012041ab160d1159/ash/ime/ime_controller.h
[modify] https://crrev.com/ede316aa6bbb83e9c6c6b9df012041ab160d1159/ash/login/login_screen_controller.cc
[modify] https://crrev.com/ede316aa6bbb83e9c6c6b9df012041ab160d1159/ash/login/login_screen_controller.h
[modify] https://crrev.com/ede316aa6bbb83e9c6c6b9df012041ab160d1159/ash/media_controller.cc
[modify] https://crrev.com/ede316aa6bbb83e9c6c6b9df012041ab160d1159/ash/media_controller.h
[modify] https://crrev.com/ede316aa6bbb83e9c6c6b9df012041ab160d1159/ash/session/session_controller.h
[modify] https://crrev.com/ede316aa6bbb83e9c6c6b9df012041ab160d1159/ash/system/tray/system_tray_controller.cc
[modify] https://crrev.com/ede316aa6bbb83e9c6c6b9df012041ab160d1159/ash/system/tray/system_tray_controller.h
[modify] https://crrev.com/ede316aa6bbb83e9c6c6b9df012041ab160d1159/ash/tray_action/tray_action.cc
[modify] https://crrev.com/ede316aa6bbb83e9c6c6b9df012041ab160d1159/ash/tray_action/tray_action.h
[modify] https://crrev.com/ede316aa6bbb83e9c6c6b9df012041ab160d1159/ash/wm/tablet_mode/tablet_mode_controller.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Dec 15 2017

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

commit 9dc63392255bceee3796c4750fcd560cac786040
Author: James Cook <jamescook@chromium.org>
Date: Fri Dec 15 00:27:33 2017

Add docs to a DCHECK in mojo bindings

Confusion about when to use Binding<> vs. BindingSet<> can cause an
attempt to bind to an interface that is already bound. There is an
existing DCHECK for this -- just add some docs to make that more clear.

Bug:  794581 
Change-Id: I14194770190e90e80403c027a65e7820949331f8
Reviewed-on: https://chromium-review.googlesource.com/827903
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524260}
[modify] https://crrev.com/9dc63392255bceee3796c4750fcd560cac786040/mojo/public/cpp/bindings/lib/binding_state.cc

Labels: ReleaseBlock-Stable
RBS so I don't forget to backport.
Labels: Merge-Request-64
Merge-request for M64 for the small CL in comment 4.

Labels: -Merge-Request-64 Merge-Approved-64
Approving merge to M64 Chrome OS to resolve crash
Project Member

Comment 10 by bugdroid1@chromium.org, Dec 15 2017

Labels: -merge-approved-64 merge-merged-3282
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/52e0b44b22e39240a3d6387444b6840d37dcc238

commit 52e0b44b22e39240a3d6387444b6840d37dcc238
Author: James Cook <jamescook@chromium.org>
Date: Fri Dec 15 22:25:26 2017

cros: Fix crash when opening keyboard shortcut overlay from settings

Crash was introduced in commit 8f1e606d49b0b2cea3a0c0796a912f50171e7173

More than one part of chrome calls mojo methods in NewWindowController,
so there can be more than one binding. I've audited the other changes
in the above CL and they should be OK, so keeping this change small
for easier backport.

TBR=jamescook@chromium.org

(cherry picked from commit 3918d7bb6217430a67b1424b7a8a0960b2048c62)

Bug:  794581 
Test: ash_unittests, chrome unit_tests and browser_tests, manually open keyboard shortcut overlay from webui settings > keyboard section
Change-Id: I6b274ecd7b8e6c0c366c36a398e9094b4c361dd9
Reviewed-on: https://chromium-review.googlesource.com/826026
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#523969}
Reviewed-on: https://chromium-review.googlesource.com/830658
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/branch-heads/3282@{#245}
Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840}
[modify] https://crrev.com/52e0b44b22e39240a3d6387444b6840d37dcc238/ash/new_window_controller.cc
[modify] https://crrev.com/52e0b44b22e39240a3d6387444b6840d37dcc238/ash/new_window_controller.h

Status: Fixed (was: Started)

Sign in to add a comment