WS: system accelerators break after events are ack'ed out-of-order |
||||||||||
Issue descriptionKSV app breaks system accelerators sometimes On linux-chromeos ToT @ #590830 (1) Open the KSV (Ctrl-Alt-/) (2) Repeatedly click between that window and a browser window (?) (3) Press Ctrl-Shift-Q Expected: The system accelerator is always handled (shows a warning) Actual: Sometimes the accelerator stops working while KSV is focused It's tough to work out exactly what steps trigger this. When it does break, Ctrl-N also stops working.
,
Sep 17
Somehow the ordering of client event acks breaks, causing events to be dropped by the window tree. At some point in local experimentation like above, the client acks an event out of order: [shortcut:58228:58228:0917/101606.618768:ERROR:input_method_mus.cc(224)] MSW InputMethodMus ProcessKeyEventCallback key=KeyQ handled=0 [shortcut:58228:58228:0917/101607.170239:ERROR:input_method_mus.cc(224)] MSW InputMethodMus ProcessKeyEventCallback key=KeyQ handled=0 [58086:58086:0917/101607.170516:ERROR:exit_warning_handler.cc(100)] MSW ExitWarningHandler::HandleAccelerator [shortcut:58228:58228:0917/101607.250286:ERROR:input_method_mus.cc(224)] MSW InputMethodMus ProcessKeyEventCallback key=KeyQ handled=0 [58086:58086:0917/101607.818165:ERROR:window_tree.cc(1740)] MSW client acked unknown event id=18126535, 2 in-flight front_id=25939441 [shortcut:58228:58228:0917/101607.818188:ERROR:input_method_mus.cc(224)] MSW InputMethodMus ProcessKeyEventCallback key=ControlLeft handled=0 [58086:58086:0917/101607.818241:ERROR:window_tree.cc(1745)] MSW in-flight id=25939441 type=ET_KEY_RELEASED keyControlLeft [58086:58086:0917/101607.818267:ERROR:window_tree.cc(1747)] MSW in-flight id=18126535 null That causes all subsequent unhandled events to be discarded, so no system accelerators are handled. The in-flight queue eventually fills to 99 events. I *think* the out-of-order event ack (id=18126535) might be a mouse event... more investigation is needed.
,
Sep 17
Yeah, it's a mouse move event: window_tree.cc(172)] MSW QUEUE id=26016650 type=ET_KEY_RELEASED window_tree.cc(172)] MSW QUEUE id=20907042 type=ET_MOUSE_MOVED window_tree.cc(1740)] MSW ACK id=20907042 window_tree.cc(1742)] MSW client acked unknown event id=20907042, 2 in-flight front_id=26016650 window_tree.cc(1747)] MSW in-flight id=26016650 type=ET_KEY_RELEASED keyControlLeft window_tree.cc(1749)] MSW in-flight id=20907042 null window_tree.cc(1740)] MSW ACK id=26016650 TBD why the mouse move is acked before the control key release.
,
Sep 17
EASY REPRO: Move the mouse *while* pressing Ctrl-Shift-Q or another accelerator. This comes down to the async behavior of InputMethodMus::SendKeyEventToInputMethod/ProcessKeyEventCallback. These call through the InputMethod::ProcessKeyEvent mojo api to chrome's InputMethodBridge::ProcessKeyEvent. https://cs.chromium.org/chromium/src/services/ws/public/mojom/ime/ime.mojom?rcl=55de0d943e5c1d69acd07f8e58054ef60a44954e&l=155 During that async op, a mouse move event may come in and be automatically acked, breaking ws/client event ack ordering. [shortcut:window_tree_client.cc(1317)] MSW INPUT id=29789879 type=ET_KEY_RELEASED key=ControlLeft [shortcut:window_tree_client.cc(1347)] MSW INPUT input_method [shortcut:input_method_mus.cc(147)] MSW InputMethodMus::SendKeyEventToInputMethod key=ControlLeft [shortcut:window_tree_client.cc(1317)] MSW INPUT id=32761908 type=ET_POINTER_MOVED [shortcut:window_tree_client.cc(640)] MSW ACK id=32761908 [window_tree.cc(1742)] MSW client acked unknown event id=32761908, 2 in-flight front_id=29789879 [window_tree.cc(1747)] MSW in-flight id=29789879 type=ET_KEY_RELEASED key=ControlLeft [window_tree.cc(1749)] MSW in-flight id=32761908 null [shortcut:input_method_mus.cc(225)] MSW InputMethodMus::ProcessKeyEventCallback key=ControlLeft handled=0 [shortcut:window_tree_client.cc(640)] MSW ACK id=29789879 Here are some possible fixes (I think I would be a proponent of #2): 1) Have clients order acks to match the arrival of events (queue acks while waiting on IME) 2) Make the WS allow acks to be out-of-order (check the set if in-flight events) 3) Some larger change to IME architecture (eg. send events WS->IME->client?)
,
Sep 17
How about we start with simplest option for now, which is (2).
,
Sep 17
Maybe a slight variation; enforce ordering by the event type. So, the client must ack key events in order, and pointer events in order, but the two can be intermixed. For example, a stream of K1 M1 M2 M3 K2 (K=key event M=mouse event), then the ack must be K1 or M1.
,
Sep 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d867f894e43126caaa58790138f456536a9a5334 commit d867f894e43126caaa58790138f456536a9a5334 Author: Mike Wasserman <msw@chromium.org> Date: Tue Sep 18 18:01:29 2018 ws: Fix ime/mouse event ack ordering issue Track in-flight key events separately from others in WindowTree. (client may ack key events out-of-order from others, for async IME) Add a simple test of OnWindowInputEventAck behavior. Bug: 884368 Test: Moving a mouse while pressing an accelerator with KSV window focused. Change-Id: I7d76ce1aec2ef311f5e721903d40e1b3819e2953 Reviewed-on: https://chromium-review.googlesource.com/1229074 Commit-Queue: Michael Wasserman <msw@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#592107} [modify] https://crrev.com/d867f894e43126caaa58790138f456536a9a5334/services/ws/window_tree.cc [modify] https://crrev.com/d867f894e43126caaa58790138f456536a9a5334/services/ws/window_tree.h [modify] https://crrev.com/d867f894e43126caaa58790138f456536a9a5334/services/ws/window_tree_test_helper.h [modify] https://crrev.com/d867f894e43126caaa58790138f456536a9a5334/services/ws/window_tree_unittest.cc
,
Sep 18
Requesting merge to M-70 to avoid breaking accelerators when users open the new KSV app and move the mouse *while* typing or pressing accelerator keys.
,
Sep 19
This bug requires manual review: M70 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 24
geohsu@, please consider my merge request, thanks. It's a fairly low-risk change to fix a mild-severity defect in the Chrome OS keyboard shortcut viewer.
,
Sep 24
,
Sep 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c0a3c47f44e6d771bed5d4ad49445f0fcc702041 commit c0a3c47f44e6d771bed5d4ad49445f0fcc702041 Author: Mike Wasserman <msw@chromium.org> Date: Mon Sep 24 21:45:22 2018 ws: Fix ime/mouse event ack ordering issue (MERGE TO M-70 BRANCH 3538) Track in-flight key events separately from others in WindowTree. (client may ack key events out-of-order from others, for async IME) Add a simple test of OnWindowInputEventAck behavior. TBR=msw@chromium.org (cherry picked from commit d867f894e43126caaa58790138f456536a9a5334) Bug: 884368 Test: Moving a mouse while pressing an accelerator with KSV window focused. Change-Id: I7d76ce1aec2ef311f5e721903d40e1b3819e2953 Reviewed-on: https://chromium-review.googlesource.com/1229074 Commit-Queue: Michael Wasserman <msw@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#592107} Reviewed-on: https://chromium-review.googlesource.com/1241757 Reviewed-by: Michael Wasserman <msw@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#612} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/c0a3c47f44e6d771bed5d4ad49445f0fcc702041/services/ws/window_tree.cc [modify] https://crrev.com/c0a3c47f44e6d771bed5d4ad49445f0fcc702041/services/ws/window_tree.h [modify] https://crrev.com/c0a3c47f44e6d771bed5d4ad49445f0fcc702041/services/ws/window_tree_test_helper.h [modify] https://crrev.com/c0a3c47f44e6d771bed5d4ad49445f0fcc702041/services/ws/window_tree_unittest.cc
,
Sep 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c0a3c47f44e6d771bed5d4ad49445f0fcc702041 Commit: c0a3c47f44e6d771bed5d4ad49445f0fcc702041 Author: msw@chromium.org Commiter: msw@chromium.org Date: 2018-09-24 21:45:22 +0000 UTC ws: Fix ime/mouse event ack ordering issue (MERGE TO M-70 BRANCH 3538) Track in-flight key events separately from others in WindowTree. (client may ack key events out-of-order from others, for async IME) Add a simple test of OnWindowInputEventAck behavior. TBR=msw@chromium.org (cherry picked from commit d867f894e43126caaa58790138f456536a9a5334) Bug: 884368 Test: Moving a mouse while pressing an accelerator with KSV window focused. Change-Id: I7d76ce1aec2ef311f5e721903d40e1b3819e2953 Reviewed-on: https://chromium-review.googlesource.com/1229074 Commit-Queue: Michael Wasserman <msw@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#592107} Reviewed-on: https://chromium-review.googlesource.com/1241757 Reviewed-by: Michael Wasserman <msw@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#612} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
,
Sep 24
I think that merge went through okay, marking fixed. |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by jamescook@chromium.org
, Sep 14