D5704: Improve the x11 timestamp handling
graesslin added a comment. This caused a regression for XWayland windows. On Wayland QX11Info::getTimestamp always returns 0 and thus our timestamp gets set to 0. REPOSITORY R108 KWin REVISION DETAIL https://phabricator.kde.org/D5704 To: graesslin, #kwin, #plasma, davidedmundson Cc: cfeck, luebking, plasma-devel, kwin, spstarr, progwolff, Zren, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, lukas
D5704: Improve the x11 timestamp handling
This revision was automatically updated to reflect the committed changes. Closed by commit R108:0bec9ad73375: Improve the x11 timestamp handling (authored by graesslin). REPOSITORY R108 KWin CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5704?vs=14122=14169 REVISION DETAIL https://phabricator.kde.org/D5704 AFFECTED FILES autotests/CMakeLists.txt autotests/test_x11_timestamp_update.cpp main.h utils.cpp utils.h To: graesslin, #kwin, #plasma, davidedmundson Cc: cfeck, luebking, plasma-devel, kwin, spstarr, progwolff, Zren, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, lukas
D5704: Improve the x11 timestamp handling
luebking added a comment.
In https://phabricator.kde.org/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.
REPOSITORY
R108 KWin
BRANCH
x11-timestamp-handling-5.8
REVISION DETAIL
https://phabricator.kde.org/D5704
To: graesslin, #kwin, #plasma, davidedmundson
Cc: cfeck, luebking, plasma-devel, kwin, spstarr, progwolff, Zren, lesliezhai,
ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, lukas
D5704: Improve the x11 timestamp handling
davidedmundson accepted this revision. This revision is now accepted and ready to land. REPOSITORY R108 KWin BRANCH x11-timestamp-handling-5.8 REVISION DETAIL https://phabricator.kde.org/D5704 To: graesslin, #kwin, #plasma, davidedmundson Cc: cfeck, luebking, plasma-devel, kwin, spstarr, progwolff, Zren, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, lukas
D5704: Improve the x11 timestamp handling
cfeck added a comment. 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) ... REPOSITORY R108 KWin REVISION DETAIL https://phabricator.kde.org/D5704 To: graesslin, #kwin, #plasma Cc: cfeck, luebking, plasma-devel, kwin, spstarr, progwolff, Zren, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, lukas
D5704: Improve the x11 timestamp handling
luebking added a comment. > 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 ;-) REPOSITORY R108 KWin REVISION DETAIL https://phabricator.kde.org/D5704 To: graesslin, #kwin, #plasma Cc: luebking, plasma-devel, kwin, spstarr, progwolff, Zren, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, lukas
D5704: Improve the x11 timestamp handling
luebking added a comment. This should also mitigate the "idiotic client confused X11 time with epoch" condition (iirc some ssh per script?), yesno? REPOSITORY R108 KWin REVISION DETAIL https://phabricator.kde.org/D5704 To: graesslin, #kwin, #plasma Cc: luebking, plasma-devel, kwin, spstarr, progwolff, Zren, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, lukas
D5704: Improve the x11 timestamp handling
graesslin created this revision. Restricted Application added a project: KWin. Restricted Application added subscribers: kwin, plasma-devel. REVISION 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. REPOSITORY R108 KWin BRANCH x11-timestamp-handling-5.8 REVISION DETAIL https://phabricator.kde.org/D5704 AFFECTED FILES autotests/CMakeLists.txt autotests/test_x11_timestamp_update.cpp main.h utils.cpp utils.h To: graesslin, #kwin, #plasma Cc: plasma-devel, kwin, spstarr, progwolff, Zren, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, lukas
