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

Issue 899881 link

Starred by 18 users

Issue metadata

Status: Verified
Owner:
inactive
Closed: Dec 18
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug-Regression



Sign in to add a comment

Keyboard shortcuts don't work when a textbox is focused

Project Member Reported by aelias@chromium.org, Oct 29

Issue description

Chrome Version: 71.0.3578.20
OS: Linux64 (GLinux Rodete Desktop), with i3 window manager.

What steps will reproduce the problem?
(1) Focus any textbox (either the URL-bar, or a textbox on the page).
(2) Try a Chrome shortcut such as Ctrl-T, Ctrl-L or Ctrl-N.
(3) Observe nothing happens.

Notes: The shortcuts for copy-paste using Ctrl-C/Ctrl-X/Ctrl-V still work.  Also, all shortcuts work when no textbox is focused.  And this issue only affects desktop Linux.

I bisected the bug exactly to http://crrev.com/595454 "[Reland 1] Use accurate X11 event timestamp computation." using the bisect python script.
 
Cc: thomasanderson@chromium.org tdres...@chromium.org
+thomasanderson -- This is the second Linux user who's had this same issue. I can't repro locally. 

The original reason was landing the CL is no longer needed, so we could revert the CL. I'm concerned that there's an underlying bug though, since this CL theoretically only provides *more* accurate timestamps than we were using previously.
Also unable to repro on 71.0.3578.20 beta.  Not sure how your CL could be causing this issue.
Cc: sadrul@chromium.org
+sadrul, in case he has any ideas.
Does this perhaps happen after hibernating the chrome/X11 session?
M71 Stable promotion is coming VERY soon. Your bug is labelled as Stable  ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. Thank you.
M71 Stable promotion is coming VERY soon. Your bug is labelled as Stable  ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. Thank you.
Since this only happens with textboxes, this suggests that the problem lies with sending events to the renderer. This CL only modifies the timestamp of the event [to make it more accurate].

I wonder if we have some logic that throws out keyboard events based on the timestamp?

Not sure whether to leave this CL in or to revert...sadrul?
Reminder M71 Stable is approaching VERY soon. Please review this bug and assess if this is indeed a RBS. If not, please remove the RBS label. If so, please make sure any planned work will be tested in Beta and verified before the Stable date. Thank you.

Requesting to take a look at M71 blockers ASAP due to upcoming Thanksgiving holidays next week.

Status: WontFix (was: Assigned)
Given lack of response from original poster, and our inability to repro, let's leave the CL merged. 
Labels: -ReleaseBlock-Stable -M-71 M-72
Owner: aelias@chromium.org
Status: Assigned (was: WontFix)
Sorry I didn't realize you expected me to reply, since you didn't ask me any question.  I can still repro consistently (no weird scenarios needed, happens after restarting X and Chrome), there are two independent report, I have an exact bisect and the original CL doesn't do anything useful, so all indications are that we should revert.

I guess I'll redownload a Chrome instance and upload a revert to make sure this moves forward.  With the low population of Linux users with unclear % of them impacted, it may not be worth emergency cherry-pick at this point so removing RBS, but I would like to fix it moving forward so I don't have to live with this annoying bug indefinitely.
Cc: erikc...@chromium.org
Hmm, perhaps we don't need to revert, I investigated a bit and I have some more clues about what's going on.  This is a broken interaction with the "xim" input method.  This bug should repro for you locally if you set the environment variable GTK_IM_MODULE=xim (non-English IME).

I noticed that when a textbox is focused and any key is pressed, Chrome spams stderr with the following message.  This happens even if I locally revert http://crrev.com/595454, but it doesn't have any visible symptoms if key events have the old incorrect timestamp.  It doesn't happen if a textbox is not focused.

(chrome:141726): Gdk-WARNING **: gdk_window_set_user_time called on non-toplevel

https://unix.stackexchange.com/questions/180067/chrome-gdk-gdk-window-set-user-time-called-on-non-toplevel says that doing "unset GTK_IM_MODULE" fixes the logspam, and indeed I also found that works around the bug.
Thanks for the investigation. This sounds like a longstanding GDK/Chrome issue which has rather surprising side effects. I would guess that this current bug is simply one visible manifestation.

Do we have any idea how many users this effects? That might help us assign a priority & severity. 

This wikipedia article implies that it's rarely used:
https://en.wikipedia.org/wiki/X_Input_Method

Cc: yoichio@chromium.org
I set it up originally to type Japanese, but I don't know if it's popular or I picked something weird.  yoichio@, would you know if anybody in Tokyo office uses XIM anymore to input Japanese on Linux, or does everybody use something else?
I have the same issue, but my GTK_IM_MODULE is set to "ibus" and not "xim". When I unset GTK_IM_MODULE, the keyboard shortcuts work as expected.
Some more details:
- The bug does not happen at all when my keyboard layout is "English (US)", but does happen when my keyboard layout is "French (Canada)". Unsetting GTK_IM_MODULE, which is normally set to "ibus", does solve the problem. I also do not see the Gdk_Warning referred to above in my terminal.
- When the bug does happen, if I keep spamming the keyboard shortcut that is not working, it will eventually work once at some point. The issue thus seems at least partially non-deterministic and may be related to timing or random events.

After looking at the code of the commit that potentially introduced the issue, it seems possible that clamping the delta to the range [-30, 30] (in ClamDeltaFromExternalSource) instead of [0, 60] (in ValidateEventTimeClock https://cs.chromium.org/chromium/src/ui/events/event_utils.cc?type=cs&q=ValidateEventTimeClock&sq=package:chromium&g=0&l=70) may cause the issue.
The differences are:
- [-30, 0]: the old code will fix these values, the new code will not.
- [30, 60]: the old code will not fix these values, the new code will.

Perhaps my timestamps are usually, but not always, in these two ranges. I suspect they are more likely to be slightly negative than off by 30+ seconds.
Cc: -yoichio@chromium.org nicolaso@chromium.org
Cc: -sadrul@chromium.org yoichio@chromium.org
(removed yoichio@ from CC accidentally, re-adding)
Cc: sadrul@chromium.org
FWIW I'm seeing the same issue in 71 and just checked and I had:
GTK_IM_MODULE=xim
QT4_IM_MODULE=xim
QT_IM_MODULE=ibus
XMODIFIERS=@im=ibus

After launching chrome with all of them set to ibus it seems to work fine.

However looking at im-config details, the reason why it is setting GTK_IM_MODULE=xim is since I didn't have the the im-ibus.so plugin installed.

In Debian these are provided by ibus-gtk and ibus-gtk3, so after installing the missing packages I'm now getting:
GTK_IM_MODULE=ibus
QT4_IM_MODULE=ibus
QT_IM_MODULE=ibus
XMODIFIERS=@im=ibus

And everything is fine.
For anyone experiencing this issue, please try getting a trace of the events from the X server with xtrace:

$ xtrace google-chrome-stable > log.txt

And then upload log.txt
> After looking at the code of the commit that potentially introduced the issue, it seems possible that clamping the delta to the range [-30, 30]

This is not the issue.  I specifically tried making that change in my earlier investigation and it doesn't matter.

The problem is caused simply by setting correct timestamps instead of wildly incorrect timestamps, so inspecting the patch itself isn't very revealing. The old incorrect timestamps must've accidentally worked around another bug.  The root cause relates to "(chrome:141726): Gdk-WARNING **: gdk_window_set_user_time called on non-toplevel" (I'm guessing it needs to be called on the toplevel instead?).

> For anyone experiencing this issue, please try getting a trace of the events from the X server with xtrace:

I'm OOO right now, but I'll try when I get back to my Linux machine.
xtrace logs are attached to this comment.
- log-new-session.txt contains the logs of xtrace when xtrace starts a whole new Chrome session. When Chrome is started in this manner, the bug does not happen.
- log-existing-session.txt contains the logs of xtrace when xtrace simply opens a new window in an existing Chrome session. The bug happens in this case.

In both cases, I simply clicked on the omnibar, then pressed CTRL+T three times, followed by CTRL+W three times, then closed the opened window by clicking on the X button.
log-new-session.txt
5.3 MB View Download
log-existing-session.txt
579 KB View Download
Thanks for the traces!  Hm.. it's odd that the bug disappears when running under xtrace.  The only thing I can think of is that xtrace adds some extra latency.  (Also when opening a browser window in an existing session, the new window gets created by the original browser process, so the communication with the X server doesn't go through xtrace).

Does the bug reproduce reliably?  What about when running a stress test in the background?
The bug can be reproduced reliably on my computer. I tried running a CPU stress test but it didn't impact the behavior of Chrome.

https://superuser.com/questions/1382314/google-chrome-blocks-ctrlletter-and-altarrow-shortcuts-when-in-text-fi These users seem to have the same issue and appear to have solved it by re-adding their problematic keyboard layout.
Project Member

Comment 25 by bugdroid1@chromium.org, Dec 18

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

commit faa579def265753007f87c3ea85a907f0ed469b0
Author: Thomas Anderson <thomasanderson@chromium.org>
Date: Tue Dec 18 19:44:53 2018

Revert "[Reland 1] Use accurate X11 event timestamp computation."""

This reverts commit 2c72cd9089bc3278bf3d2852c971325a9c07f097.

Reason for revert: bugs  915277  and  899881 

Original change's description:
> [Reland 1] Use accurate X11 event timestamp computation.""
> 
> The original CL didn't allow negative time skew from the time server. While this
> is likely correct, this caused unit tests to fail as they relied on negative time
> skew. This CL allows negative time skew, and also removes another source of
> non-determinism from the tests.
> 
> Original change's description:
> > Use accurate X11 event timestamp computation.
> >
> > X events have a timestamp which is only well defined relative to the X11 Server
> > time. The previous computation for timestamp for X11 events was making the
> > assumption that Server time and Chrome time were the same. This assumption is
> > not always true -- this is likely the root cause of "bad" timestamps observed in
> > https://bugs.chromium.org/p/chromium/issues/detail?id=650338#c1
> >
> > This CL changes event timestamp computation to make a roundtrip to the X11
> > Server to get an accurate base::TimeTicks. This logic was lifted out of the
> > responsiveness calculator, which was already doing this computation. The latter
> > will subsequently be changed to use the computation in this CL.
> >
> > Change-Id: I963019cd8bfb8ce14e06b3743a159c9c85f2cb82
> > Bug:  859155 
> > Reviewed-on: https://chromium-review.googlesource.com/1249383
> > Reviewed-by: Thomas Anderson <thomasanderson@chromium.org>
> > Reviewed-by: Avi Drissman <avi@chromium.org>
> > Commit-Queue: Erik Chen <erikchen@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#594844}
> 
> Change-Id: I6595f6aa163a18c9724c50546ac8c84bc5c84a03
> Bug:  859155 
> Reviewed-on: https://chromium-review.googlesource.com/1252685
> Reviewed-by: Thomas Anderson <thomasanderson@chromium.org>
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Commit-Queue: Erik Chen <erikchen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#595454}

TBR=avi@chromium.org,erikchen@chromium.org,thomasanderson@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  859155 , 899881 , 915277 
Change-Id: Ib85a5f5d7bf9469076a334d77ef92771199d6a2d
Reviewed-on: https://chromium-review.googlesource.com/c/1382868
Reviewed-by: Thomas Anderson <thomasanderson@chromium.org>
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617593}
[modify] https://crrev.com/faa579def265753007f87c3ea85a907f0ed469b0/ui/events/event_unittest.cc
[modify] https://crrev.com/faa579def265753007f87c3ea85a907f0ed469b0/ui/events/platform/x11/x11_event_source.cc
[modify] https://crrev.com/faa579def265753007f87c3ea85a907f0ed469b0/ui/events/platform/x11/x11_event_source.h
[modify] https://crrev.com/faa579def265753007f87c3ea85a907f0ed469b0/ui/events/x/events_x.cc
[modify] https://crrev.com/faa579def265753007f87c3ea85a907f0ed469b0/ui/events/x/events_x_unittest.cc
[modify] https://crrev.com/faa579def265753007f87c3ea85a907f0ed469b0/ui/events/x/events_x_utils.cc
[modify] https://crrev.com/faa579def265753007f87c3ea85a907f0ed469b0/ui/events/x/events_x_utils.h

Labels: Merge-Request-72
Status: Fixed (was: Assigned)
thomasanderson@ how safe is this change to land into M72 ? This issue seems to be there since M71, how critical is the fix to go out in M72  can it wait until M73?
It should be pretty safe since it's a clean revert.  I'd like to get this into M72 if possible.
thomasanderson@ can you please update the canary results here once available tomorrow , so I can review/approve merge request
BTW, this is really annoying in M71, is there any chance to merge it there? That would be awesome. Thank you very much.
Cc: viswa.karala@chromium.org
 Issue 912611  has been merged into this issue.
Project Member

Comment 32 by sheriffbot@chromium.org, Dec 19

Labels: -Merge-Request-72 Merge-Review-72 Hotlist-Merge-Review
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Status: Verified (was: Fixed)
Verified on 73.0.3645.0.  Set GTK_IM_MODULE=xim and Ctrl-T works from the omnibox
Labels: -Merge-Review-72 Merge-Approved-72
branch:3626
Project Member

Comment 35 by bugdroid1@chromium.org, Dec 20

Labels: -merge-approved-72 merge-merged-3626
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6ad9f6333b2a7d81bbbf926529e560269e05f439

commit 6ad9f6333b2a7d81bbbf926529e560269e05f439
Author: Tom Anderson <thomasanderson@chromium.org>
Date: Thu Dec 20 19:15:13 2018

[Merge to M72] Revert "[Reland 1] Use accurate X11 event timestamp computation."""

> This reverts commit 2c72cd9089bc3278bf3d2852c971325a9c07f097.
>
> Reason for revert: bugs  915277  and  899881 
>
> Original change's description:
> > [Reland 1] Use accurate X11 event timestamp computation.""
> >
> > The original CL didn't allow negative time skew from the time server. While this
> > is likely correct, this caused unit tests to fail as they relied on negative time
> > skew. This CL allows negative time skew, and also removes another source of
> > non-determinism from the tests.
> >
> > Original change's description:
> > > Use accurate X11 event timestamp computation.
> > >
> > > X events have a timestamp which is only well defined relative to the X11 Server
> > > time. The previous computation for timestamp for X11 events was making the
> > > assumption that Server time and Chrome time were the same. This assumption is
> > > not always true -- this is likely the root cause of "bad" timestamps observed in
> > > https://bugs.chromium.org/p/chromium/issues/detail?id=650338#c1
> > >
> > > This CL changes event timestamp computation to make a roundtrip to the X11
> > > Server to get an accurate base::TimeTicks. This logic was lifted out of the
> > > responsiveness calculator, which was already doing this computation. The latter
> > > will subsequently be changed to use the computation in this CL.
> > >
> > > Change-Id: I963019cd8bfb8ce14e06b3743a159c9c85f2cb82
> > > Bug:  859155 
> > > Reviewed-on: https://chromium-review.googlesource.com/1249383
> > > Reviewed-by: Thomas Anderson <thomasanderson@chromium.org>
> > > Reviewed-by: Avi Drissman <avi@chromium.org>
> > > Commit-Queue: Erik Chen <erikchen@chromium.org>
> > > Cr-Commit-Position: refs/heads/master@{#594844}
> >
> > Change-Id: I6595f6aa163a18c9724c50546ac8c84bc5c84a03
> > Bug:  859155 
> > Reviewed-on: https://chromium-review.googlesource.com/1252685
> > Reviewed-by: Thomas Anderson <thomasanderson@chromium.org>
> > Reviewed-by: Avi Drissman <avi@chromium.org>
> > Commit-Queue: Erik Chen <erikchen@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#595454}
>
> TBR=avi@chromium.org,erikchen@chromium.org,thomasanderson@chromium.org
>
> # Not skipping CQ checks because original CL landed > 1 day ago.
>
> Bug:  859155 , 899881 , 915277 
> Change-Id: Ib85a5f5d7bf9469076a334d77ef92771199d6a2d
> Reviewed-on: https://chromium-review.googlesource.com/c/1382868
> Reviewed-by: Thomas Anderson <thomasanderson@chromium.org>
> Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#617593}

TBR=avi@chromium.org,erikchen@chromium.org,thomasanderson@chromium.org
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true

Bug:  859155 , 899881 , 915277 
Change-Id: I325a7b1587a8dace272d2b2ac8a2dc0ea0082218
Reviewed-on: https://chromium-review.googlesource.com/c/1387545
Reviewed-by: Thomas Anderson <thomasanderson@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#487}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/6ad9f6333b2a7d81bbbf926529e560269e05f439/ui/events/event_unittest.cc
[modify] https://crrev.com/6ad9f6333b2a7d81bbbf926529e560269e05f439/ui/events/platform/x11/x11_event_source.cc
[modify] https://crrev.com/6ad9f6333b2a7d81bbbf926529e560269e05f439/ui/events/platform/x11/x11_event_source.h
[modify] https://crrev.com/6ad9f6333b2a7d81bbbf926529e560269e05f439/ui/events/x/events_x.cc
[modify] https://crrev.com/6ad9f6333b2a7d81bbbf926529e560269e05f439/ui/events/x/events_x_unittest.cc
[modify] https://crrev.com/6ad9f6333b2a7d81bbbf926529e560269e05f439/ui/events/x/events_x_utils.cc
[modify] https://crrev.com/6ad9f6333b2a7d81bbbf926529e560269e05f439/ui/events/x/events_x_utils.h

Labels: Merge-Merged-72-3626
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/6ad9f6333b2a7d81bbbf926529e560269e05f439

Commit: 6ad9f6333b2a7d81bbbf926529e560269e05f439
Author: thomasanderson@chromium.org
Commiter: thomasanderson@chromium.org
Date: 2018-12-20 19:15:13 +0000 UTC

[Merge to M72] Revert "[Reland 1] Use accurate X11 event timestamp computation."""

> This reverts commit 2c72cd9089bc3278bf3d2852c971325a9c07f097.
>
> Reason for revert: bugs  915277  and  899881 
>
> Original change's description:
> > [Reland 1] Use accurate X11 event timestamp computation.""
> >
> > The original CL didn't allow negative time skew from the time server. While this
> > is likely correct, this caused unit tests to fail as they relied on negative time
> > skew. This CL allows negative time skew, and also removes another source of
> > non-determinism from the tests.
> >
> > Original change's description:
> > > Use accurate X11 event timestamp computation.
> > >
> > > X events have a timestamp which is only well defined relative to the X11 Server
> > > time. The previous computation for timestamp for X11 events was making the
> > > assumption that Server time and Chrome time were the same. This assumption is
> > > not always true -- this is likely the root cause of "bad" timestamps observed in
> > > https://bugs.chromium.org/p/chromium/issues/detail?id=650338#c1
> > >
> > > This CL changes event timestamp computation to make a roundtrip to the X11
> > > Server to get an accurate base::TimeTicks. This logic was lifted out of the
> > > responsiveness calculator, which was already doing this computation. The latter
> > > will subsequently be changed to use the computation in this CL.
> > >
> > > Change-Id: I963019cd8bfb8ce14e06b3743a159c9c85f2cb82
> > > Bug:  859155 
> > > Reviewed-on: https://chromium-review.googlesource.com/1249383
> > > Reviewed-by: Thomas Anderson <thomasanderson@chromium.org>
> > > Reviewed-by: Avi Drissman <avi@chromium.org>
> > > Commit-Queue: Erik Chen <erikchen@chromium.org>
> > > Cr-Commit-Position: refs/heads/master@{#594844}
> >
> > Change-Id: I6595f6aa163a18c9724c50546ac8c84bc5c84a03
> > Bug:  859155 
> > Reviewed-on: https://chromium-review.googlesource.com/1252685
> > Reviewed-by: Thomas Anderson <thomasanderson@chromium.org>
> > Reviewed-by: Avi Drissman <avi@chromium.org>
> > Commit-Queue: Erik Chen <erikchen@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#595454}
>
> TBR=avi@chromium.org,erikchen@chromium.org,thomasanderson@chromium.org
>
> # Not skipping CQ checks because original CL landed > 1 day ago.
>
> Bug:  859155 , 899881 , 915277 
> Change-Id: Ib85a5f5d7bf9469076a334d77ef92771199d6a2d
> Reviewed-on: https://chromium-review.googlesource.com/c/1382868
> Reviewed-by: Thomas Anderson <thomasanderson@chromium.org>
> Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#617593}

TBR=avi@chromium.org,erikchen@chromium.org,thomasanderson@chromium.org
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true

Bug:  859155 , 899881 , 915277 
Change-Id: I325a7b1587a8dace272d2b2ac8a2dc0ea0082218
Reviewed-on: https://chromium-review.googlesource.com/c/1387545
Reviewed-by: Thomas Anderson <thomasanderson@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#487}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
Cc: vamshi.kommuri@chromium.org
 Issue 870495  has been merged into this issue.
 Issue 913551  has been merged into this issue.
 Issue 918047  has been merged into this issue.
 Issue 918238  has been merged into this issue.
Cc: phanindra.mandapaka@chromium.org
 Issue 921327  has been merged into this issue.

Sign in to add a comment