From 0bec9ad7337536e319c17c5684d97e1156399fdb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Gr=C3=A4=C3=9Flin?= Date: Wed, 3 May 2017 21:17:25 +0200 Subject: [PATCH] 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. Reviewers: #kwin, #plasma Subscribers: plasma-devel, kwin Tags: #kwin Differential Revision: https://phabricator.kde.org/D5704 --- autotests/CMakeLists.txt | 11 +++ autotests/test_x11_timestamp_update.cpp | 123 ++++++++++++++++++++++++ main.h | 8 +- utils.cpp | 2 +- utils.h | 6 +- 5 files changed, 144 insertions(+), 6 deletions(-) create mode 100644 autotests/test_x11_timestamp_update.cpp diff --git a/autotests/CMakeLists.txt b/autotests/CMakeLists.txt index 2974d96648..d4fdc79ffd 100644 --- a/autotests/CMakeLists.txt +++ b/autotests/CMakeLists.txt @@ -310,3 +310,14 @@ target_link_libraries(testScreenEdges add_test(kwin_testScreenEdges testScreenEdges) ecm_mark_as_test(testScreenEdges) + +######################################################## +# Test X11 TimestampUpdate +######################################################## +add_executable(testX11TimestampUpdate test_x11_timestamp_update.cpp) +target_link_libraries(testX11TimestampUpdate + Qt5::Test + kwin +) +add_test(kwin-testX11TimestampUpdate testX11TimestampUpdate) +ecm_mark_as_test(testX11TimestampUpdate) diff --git a/autotests/test_x11_timestamp_update.cpp b/autotests/test_x11_timestamp_update.cpp new file mode 100644 index 0000000000..50ba151d38 --- /dev/null +++ b/autotests/test_x11_timestamp_update.cpp @@ -0,0 +1,123 @@ +/******************************************************************** +KWin - the KDE window manager +This file is part of the KDE project. + +Copyright (C) 2017 Martin Gräßlin + +This program is free software; you can redistribute it and/or modify +it under the terms of the GNU General Public License as published by +the Free Software Foundation; either version 2 of the License, or +(at your option) any later version. + +This program is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU General Public License for more details. + +You should have received a copy of the GNU General Public License +along with this program. If not, see . +*********************************************************************/ + +#include +#include + +#include "main.h" +#include "utils.h" + +namespace KWin +{ + +class X11TestApplication : public Application +{ + Q_OBJECT +public: + X11TestApplication(int &argc, char **argv); + virtual ~X11TestApplication(); + +protected: + void performStartup() override; + +}; + +X11TestApplication::X11TestApplication(int &argc, char **argv) + : Application(OperationModeX11, argc, argv) +{ + setX11Connection(QX11Info::connection()); + setX11RootWindow(QX11Info::appRootWindow()); +} + +X11TestApplication::~X11TestApplication() +{ +} + +void X11TestApplication::performStartup() +{ +} + +} + +class X11TimestampUpdateTest : public QObject +{ + Q_OBJECT +private Q_SLOTS: + void testGrabAfterServerTime(); + void testBeforeLastGrabTime(); +}; + +void X11TimestampUpdateTest::testGrabAfterServerTime() +{ + // this test tries to grab the X keyboard with a timestamp in future + // that should fail, but after updating the X11 timestamp, it should + // work again + KWin::updateXTime(); + QCOMPARE(KWin::grabXKeyboard(), true); + KWin::ungrabXKeyboard(); + + // now let's change the timestamp + KWin::kwinApp()->setX11Time(KWin::xTime() + 5 * 60 * 1000); + + // now grab keyboard should fail + QCOMPARE(KWin::grabXKeyboard(), false); + + // let's update timestamp, now it should work again + KWin::updateXTime(); + QCOMPARE(KWin::grabXKeyboard(), true); + KWin::ungrabXKeyboard(); +} + +void X11TimestampUpdateTest::testBeforeLastGrabTime() +{ + // this test tries to grab the X keyboard with a timestamp before the + // last grab time on the server. That should fail, but after updating the X11 + // timestamp it should work again + + // first set the grab timestamp + KWin::updateXTime(); + QCOMPARE(KWin::grabXKeyboard(), true); + KWin::ungrabXKeyboard(); + + // now go to past + const auto timestamp = KWin::xTime(); + KWin::kwinApp()->setX11Time(KWin::xTime() - 5 * 60 * 1000, KWin::Application::TimestampUpdate::Always); + QCOMPARE(KWin::xTime(), timestamp - 5 * 60 * 1000); + + // now grab keyboard should fail + QCOMPARE(KWin::grabXKeyboard(), false); + + // let's update timestamp, now it should work again + KWin::updateXTime(); + QVERIFY(KWin::xTime() >= timestamp); + QCOMPARE(KWin::grabXKeyboard(), true); + KWin::ungrabXKeyboard(); +} + +int main(int argc, char *argv[]) +{ + setenv("QT_QPA_PLATFORM", "xcb", true); + KWin::X11TestApplication app(argc, argv); + app.setAttribute(Qt::AA_Use96Dpi, true); + X11TimestampUpdateTest tc; + return QTest::qExec(&tc, argc, argv); +} + +#include "test_x11_timestamp_update.moc" diff --git a/main.h b/main.h index a7dd47dd60..6706c0faf5 100644 --- a/main.h +++ b/main.h @@ -104,8 +104,12 @@ public: xcb_timestamp_t x11Time() const { return m_x11Time; } - void setX11Time(xcb_timestamp_t timestamp) { - if (timestamp > m_x11Time) { + enum class TimestampUpdate { + OnlyIfLarger, + Always + }; + void setX11Time(xcb_timestamp_t timestamp, TimestampUpdate force = TimestampUpdate::OnlyIfLarger) { + if (timestamp > m_x11Time || force == TimestampUpdate::Always) { m_x11Time = timestamp; } } diff --git a/utils.cpp b/utils.cpp index ad45406e56..8d3f2ffbeb 100644 --- a/utils.cpp +++ b/utils.cpp @@ -84,7 +84,7 @@ void updateXTime() // NOTE: QX11Info::getTimestamp does not yet search the event queue as the old // solution did. This means there might be regressions currently. See the // documentation above on how it should be done properly. - kwinApp()->setX11Time(QX11Info::getTimestamp()); + kwinApp()->setX11Time(QX11Info::getTimestamp(), Application::TimestampUpdate::Always); } static int server_grab_count = 0; diff --git a/utils.h b/utils.h index e9fa912328..06980fd643 100644 --- a/utils.h +++ b/utils.h @@ -143,12 +143,12 @@ MaximizeMode operator^(MaximizeMode m1, MaximizeMode m2) template using ScopedCPointer = QScopedPointer; -void updateXTime(); +void KWIN_EXPORT updateXTime(); void grabXServer(); void ungrabXServer(); bool grabbedXServer(); -bool grabXKeyboard(xcb_window_t w = rootWindow()); -void ungrabXKeyboard(); +bool KWIN_EXPORT grabXKeyboard(xcb_window_t w = rootWindow()); +void KWIN_EXPORT ungrabXKeyboard(); /** * Small helper class which performs @link grabXServer in the ctor and