Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(59)

Issue 985163002: ozone: evdev: Fix possibility of stuck keys (Closed)

Created:
5 years, 9 months ago by spang
Modified:
5 years, 9 months ago
Reviewers:
kpschoedel
CC:
chromium-reviews, kalyank, tdresser+watch_chromium.org, jdduke+watch_chromium.org, ozone-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

ozone: evdev: Fix possibility of stuck keys There's a couple possible ways to get keys stuck down: (1) User unplugs the device while holding a key. A key release is queued by the kernel but races the unplug event. The unplug arrives first and we tear everything down without ever dispatching a release event. (2) Key release gets dropped in-kernel due to queue overruns. We never get the release, and continue processing without dispatching a release event. We do get a SYN_DROPPED event to notify us that events were lost. If stuck, a key can be pressed again to unstick it. Fix these cases by keeping track of pressed keys for every keyboard device, and releasing everything during teardown & following queue overruns. Also unify with the path added for key filtering. With the additional state, we can tell that we have an outstanding press and avoid filtering the corresponding release. BUG=463002 TEST= (1) Tested unplug by unplugging devices with shift held. Shift gets properly released rather than being stuck. (NB: Could not reproduce without adding a delay to expose the race). (2) Tested SYN_DROPPED by suspending chrome with key pressed, releasing it, and then pressing more keys to cause an overrun. (suspended via kill -STOP <browser>) (3) event_unittests Committed: https://crrev.com/48b636eefc4e4aa68c474680519e13ef4a337870 Cr-Commit-Position: refs/heads/master@{#319656}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+112 lines, -24 lines) Patch
M ui/events/ozone/evdev/event_converter_evdev.h View 1 chunk +3 lines, -0 lines 0 comments Download
M ui/events/ozone/evdev/event_converter_evdev.cc View 1 chunk +5 lines, -2 lines 0 comments Download
M ui/events/ozone/evdev/event_converter_evdev_impl.h View 2 chunks +9 lines, -2 lines 0 comments Download
M ui/events/ozone/evdev/event_converter_evdev_impl.cc View 5 chunks +48 lines, -9 lines 0 comments Download
M ui/events/ozone/evdev/event_converter_evdev_impl_unittest.cc View 2 chunks +47 lines, -0 lines 0 comments Download
M ui/events/ozone/evdev/input_controller_evdev.cc View 1 chunk +0 lines, -3 lines 0 comments Download
M ui/events/ozone/evdev/keyboard_evdev.h View 1 chunk +0 lines, -3 lines 0 comments Download
M ui/events/ozone/evdev/keyboard_evdev.cc View 1 chunk +0 lines, -5 lines 0 comments Download

Messages

Total messages: 11 (4 generated)
spang
5 years, 9 months ago (2015-03-06 21:05:57 UTC) #2
kpschoedel
lgtm
5 years, 9 months ago (2015-03-06 21:27:10 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/985163002/1
5 years, 9 months ago (2015-03-07 00:37:30 UTC) #5
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) mac_chromium_rel_ng on ...
5 years, 9 months ago (2015-03-07 02:39:10 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/985163002/1
5 years, 9 months ago (2015-03-09 16:41:18 UTC) #9
commit-bot: I haz the power
Committed patchset #1 (id:1)
5 years, 9 months ago (2015-03-09 17:04:56 UTC) #10
commit-bot: I haz the power
5 years, 9 months ago (2015-03-09 17:10:07 UTC) #11
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/48b636eefc4e4aa68c474680519e13ef4a337870
Cr-Commit-Position: refs/heads/master@{#319656}

Powered by Google App Engine
This is Rietveld 408576698