Improve the x11 timestamp handling
ClosedPublic
Actions

Authored by graesslin on May 3 2017, 7:36 PM.

Details

Reviewers

Group Reviewers

KWin
Plasma

Commits

R108:0bec9ad73375: Improve the x11 timestamp handling

Summary

So far KWin only updated the x11 timestamp if the new timestamp is larger
than the existing one. While this is a useful thing it creates problems
when the 32 bit msec based time stamp wraps around which happens after
running an X server for 49 days. After the timestamp wrapped around KWin
would not update the timestamp any more and thus some calls might fail.
Most prominent victims are keyboard and pointer grab which fails as the
timestamp is either larger than the server timestamp or smaller than the
last grab timestamp.

Another problem related to timestamp handling is KWin getting broken by
wrong timestamps sent by applications. A prominent example is clusterssh
which used to send a timestamp as unix time which is larger than the
x timestamp and thus our timestamp gets too large.

This change addresses these problems by allowing to reset the timestamp.
This is only used from updateXTime (which is normally invoked before we
do things like grabKeyboard). Thus we make QX11Info::getTimestamp the
ultimate trusted source for timestamps.

BUG: 377901
BUG: 348569
FIXED-IN: 5.8.7

Test Plan

As I cannot wait 50 days: unit tests for the two conditions added.

Diff Detail

Repository

R108 KWin

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

graesslin created this revision.May 3 2017, 7:36 PM

Restricted Application added a project: KWin. · View Herald TranscriptMay 3 2017, 7:36 PM

Restricted Application added subscribers: kwin, plasma-devel. · View Herald Transcript

This should also mitigate the "idiotic client confused X11 time with epoch" condition (iirc some ssh per script?), yesno?

Another problem related to timestamp handling is KWin getting broken by wrong timestamps sent by applications. A prominent example is clusterssh

Should have scrolled down the mail ;-)

I may be totally unaware about what the changes do, but from reading the comments on the bug report, I had expected to see something like

if (timestamp > m_X11Time || int(timestamp + INT_MAX / 2) < m_X11Time) ...

davidedmundson accepted this revision.May 4 2017, 8:57 PM

This revision is now accepted and ready to land.May 4 2017, 8:57 PM

In D5704#106951, @cfeck wrote:

I may be totally unaware about what the changes do, but from reading the comments on the bug report, I had expected to see something like

if (timestamp > m_X11Time || int(timestamp + INT_MAX / 2) < m_X11Time) ...

There is no reasonable heuristic here.

There is a reliable way to update the X11 time: ask the server (expensinve on the Qt5 design)
And there's an unreliable way: track the timestamps on incoming events (for free)
Times from the latter will typically be "a bit dated" and arrive in random order.

If you get time far after the known one, the system might have hibernated or there might have been a network problem (if those are a remote clients) or the new value is just bullshit ("time()")
If you get a time before the known one, the client might just have taken a longer time to send the event, you accepted a bullshit value before, this value is bullshit or the server time has wrapped around. Testing for a huge™ absolute difference *suggests* a wrap, but could also just mean some bullshit value is involved (and you don't know which one is true) and on the other hand, a small™ difference could stem from a stalled client/server connection across a wrap.

One could probably keep timestamp stats and exclude outliers to protect agains broken clients and take "ok, everyone says it's ~120, so it's probably not 2147483648" (ie. m_x11Time being the new outlier) as factual wrap, but we're asking the server on a regular base anyway. And that time is always right.

Closed by commit R108:0bec9ad73375: Improve the x11 timestamp handling (authored by graesslin). · Explain WhyMay 5 2017, 5:11 PM

This revision was automatically updated to reflect the committed changes.

This caused a regression for XWayland windows. On Wayland QX11Info::getTimestamp always returns 0 and thus our timestamp gets set to 0.