New issue
Advanced search Search tips

Issue 837768 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 3
Type: Bug



Sign in to add a comment

Chrome on Linux opens and reads /proc/self/status every time you move the mouse.

Project Member Reported by primiano@chromium.org, Apr 27 2018

Issue description

From internal G+ post: go/xrvft

I followed the code and cannot find any evident path that leads to that on official builds. If this happens on official builds I suspect might be some system library we use (maybe libdbus again?).


 

Comment 1 by ricea@chromium.org, Apr 28 2018

I believe it's ValidateEventTimeClock:

https://cs.chromium.org/chromium/src/ui/events/event_utils.cc?q=ValidateEventTimeClock&l=64

However, I didn't verify this.

I found the issue by running

strace -etrace=open google-chrome-unstable --incognito

Comment 2 by ricea@chromium.org, May 1 2018

Cc: sadrul@chromium.org
Confirmed that it is ValidateEventTimeClock() by adding 

LOG(ERROR) << "About to call BeingDebugged() in ValidateEventTimeClock()";

to that function.

Questions:
1) Do we really need to disable timestamp correction if a debugger is attached?
2) Is it necessary to detect if a debugger is attached after startup?

+sadrul for //ui/events.
Owner: majidvp@chromium.org
Status: Assigned (was: Untriaged)
--> majidvp@

Comment 4 by dancol@google.com, May 1 2018

I think there's also a case to be made for adding to libc a general-purpose and cheap API for detecting debugger attachment. It could be as simple as a variable in BSS, normally zero, written by debuggers (with debugger user approval) upon attach.
Checking for BeingDebugged() on each event IMHO seems overkill.
Is it really worth the benefit?

Can we cache that (i.e. add a "static" to that variable) under the assumptions that most folks attach the debugger since the beginning?

> I think there's also a case to be made for adding to libc a general-purpose and cheap API for detecting debugger attachment. It could be as simple as a variable in BSS, normally zero, written by debuggers (with debugger user approval) upon attach.

Yeah would be great if there was a magic section / symbol populated by debuggers and cleared on detach. Think to __start, __etext, something like __is_debugger_attached.
Good luck fighting that battle upstream though :)

Comment 6 by ricea@chromium.org, May 2 2018

I've created a proposed fix at https://chromium-review.googlesource.com/c/chromium/src/+/1039307. It just doesn't bother checking for a debugger before adjusting the timestamp on X11 events.

Since Linux builds always have USE_X11 defined, it fixes the issue for Chrome.

It may not be worth fixing the issue for DCHECK-enabled builds in general, as if no-one has complained in the 2 or more years it has been like this, it probably doesn't interfere with debugging very much.
I agree that calling BeingDebugged for every event on linux is overkill. It was only needed to avoid triggering DCHECK when debugging so it can be safely skipped for linux. So proposed fix in #6 sounds good to me.

AFAICT, DCHECK enabled build are understood to have additional overhead so checking for
the debugger here seems reasonable. If you disagree I can make the check a static one 
and leave a comment for folk who hit this DCHECK when they have attached the debugger
after startup.

Project Member

Comment 8 by bugdroid1@chromium.org, May 4 2018

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

commit f28d67fcf037d672ece700defceb58801ec34b18
Author: Adam Rice <ricea@chromium.org>
Date: Fri May 04 06:37:53 2018

Avoid checking for debugger on X11 events

Chrome checks that the timestamp on X11 events is reasonable and adjusts
it if it is not. Currently this is only done when a debugger is not
attached. The check for the debugger is expensive, so remove the check
and always adjust the timestamps.

BUG= 837768 

Change-Id: I3252374bccd27feb1bdc6088d17b1123d3c64aee
Reviewed-on: https://chromium-review.googlesource.com/1039307
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Majid Valipour <majidvp@chromium.org>
Commit-Queue: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556002}
[modify] https://crrev.com/f28d67fcf037d672ece700defceb58801ec34b18/ui/events/event_utils.cc

Comment 9 by ricea@chromium.org, May 7 2018

majidvp, feel free to close if there's nothing more you want to do here.
Status: Fixed (was: Assigned)

Sign in to add a comment