xcb: fix and optimize QXcbConnection::xi2SetMouseGrabEnabled

What was broken:

- m_xiGrab on successful ungrab (XIUngrabDevice) was never set to 'false'.
Which means that we would unnecessarily call XIAllowTouchEvents, even when
we are not grabbing (this did not have any apparent side effects).

What was non optimal:

- Redundant XIQueryDevice calls. XIQueryDevice with XIAllMasterDevices flag
already returns all required devices. Calling XIQueryDevice for every id again
does not make sense.

- Querying for master pointer info on every grab is unnecessary. Simply cache
ids of master devices whenever hierarchy changes.

What remains to be investigated some time later (or never):

The original and the re-factored code grabs all master pointer devices. Not
sure if that is the expected behavior on MPX (Multi-pointer X) systems. Could
there be two context menus, each dismissed separately? MPX concept was introduced
in XI2.0, but testing shows that this setup is not very well supported even on
modern desktop environments. Tested on Ubuntu 16.04, where multiple pointers is
enough for crashing a terminal. Also AFAIK there isn't any bug reports about
broken MPX support in Qt.

Change-Id: I53f99c6efd44abc43a0985e15cff8aae7ebba8f1
Reviewed-by: Shawn Rutledge <shawn.rutledge@qt.io>

3 files changed, 54 insertions, 54 deletions

diff --git a/src/plugins/platforms/xcb/qxcbconnection.h b/src/plugins/platforms/xcb/qxcbconnection.h
index ded11525c1..92825acb6d 100644
--- a/src/plugins/platforms/xcb/qxcbconnection.h
+++ b/src/plugins/platforms/xcb/qxcbconnection.h
@@ -712,6 +712,7 @@ private:
     QXcbSystemTrayTracker *m_systemTrayTracker = nullptr;
     QXcbGlIntegration *m_glIntegration = nullptr;
     bool m_xiGrab = false;
+    QVector<int> m_xiMasterPointerIds;
 
     xcb_window_t m_qtSelectionOwner = 0;
 
diff --git a/src/plugins/platforms/xcb/qxcbconnection_xi2.cpp b/src/plugins/platforms/xcb/qxcbconnection_xi2.cpp
index 315f295ff3..bfb3534879 100644
--- a/src/plugins/platforms/xcb/qxcbconnection_xi2.cpp
+++ b/src/plugins/platforms/xcb/qxcbconnection_xi2.cpp
@@ -319,12 +319,18 @@ void QXcbConnection::xi2SetupDevices()
     Display *xDisplay = static_cast<Display *>(m_xlib_display);
     int deviceCount = 0;
     XIDeviceInfo *devices = XIQueryDevice(xDisplay, XIAllDevices, &deviceCount);
+    m_xiMasterPointerIds.clear();
     for (int i = 0; i < deviceCount; ++i) {
-        // Only non-master pointing devices are relevant here.
-        if (devices[i].use != XISlavePointer)
+        XIDeviceInfo deviceInfo = devices[i];
+        if (deviceInfo.use == XIMasterPointer) {
+            m_xiMasterPointerIds.append(deviceInfo.deviceid);
             continue;
-        xi2SetupDevice(&devices[i], false);
+        }
+        if (deviceInfo.use == XISlavePointer) // only slave pointer devices are relevant here
+            xi2SetupDevice(&deviceInfo, false);
     }
+    if (m_xiMasterPointerIds.size() > 1)
+        qCDebug(lcQpaXInputDevices) << "multi-pointer X detected";
     XIFreeDeviceInfo(devices);
 }
 
@@ -843,62 +849,54 @@ bool QXcbConnection::startSystemResizeForTouchBegin(xcb_window_t window, const Q
 
 bool QXcbConnection::xi2SetMouseGrabEnabled(xcb_window_t w, bool grab)
 {
-    if (grab && !canGrab())
-        return false;
-
-    int num_devices = 0;
     Display *xDisplay = static_cast<Display *>(xlib_display());
-    XIDeviceInfo *info = XIQueryDevice(xDisplay, XIAllMasterDevices, &num_devices);
-    if (!info)
-        return false;
-
-    XIEventMask evmask;
-    unsigned char mask[XIMaskLen(XI_LASTEVENT)];
-    evmask.mask = mask;
-    evmask.mask_len = sizeof(mask);
-    memset(mask, 0, sizeof(mask));
-    evmask.deviceid = XIAllMasterDevices;
-
-    XISetMask(mask, XI_ButtonPress);
-    XISetMask(mask, XI_ButtonRelease);
-    XISetMask(mask, XI_Motion);
-    XISetMask(mask, XI_Enter);
-    XISetMask(mask, XI_Leave);
-    XISetMask(mask, XI_TouchBegin);
-    XISetMask(mask, XI_TouchUpdate);
-    XISetMask(mask, XI_TouchEnd);
-
-    bool grabbed = true;
-    for (int i = 0; i < num_devices; i++) {
-        int id = info[i].deviceid, n = 0;
-        XIDeviceInfo *deviceInfo = XIQueryDevice(xDisplay, id, &n);
-        if (deviceInfo) {
-            const bool grabbable = deviceInfo->use != XIMasterKeyboard;
-            XIFreeDeviceInfo(deviceInfo);
-            if (!grabbable)
-                continue;
-        }
-        if (!grab) {
-            Status result = XIUngrabDevice(xDisplay, id, CurrentTime);
+    bool ok = false;
+
+    if (grab) { // grab
+        XIEventMask evmask;
+        unsigned char mask[XIMaskLen(XI_LASTEVENT)];
+        evmask.mask = mask;
+        evmask.mask_len = sizeof(mask);
+        memset(mask, 0, sizeof(mask));
+        XISetMask(mask, XI_ButtonPress);
+        XISetMask(mask, XI_ButtonRelease);
+        XISetMask(mask, XI_Motion);
+        XISetMask(mask, XI_Enter);
+        XISetMask(mask, XI_Leave);
+        XISetMask(mask, XI_TouchBegin);
+        XISetMask(mask, XI_TouchUpdate);
+        XISetMask(mask, XI_TouchEnd);
+
+        for (int id : m_xiMasterPointerIds) {
+            evmask.deviceid = id;
+            Status result = XIGrabDevice(xDisplay, id, w, CurrentTime, None,
+                                         XIGrabModeAsync, XIGrabModeAsync, False, &evmask);
             if (result != Success) {
-                grabbed = false;
-                qCDebug(lcQpaXInput, "XInput 2.2: failed to ungrab events for device %d (result %d)", id, result);
-            }
-        } else {
-            Status result = XIGrabDevice(xDisplay, id, w, CurrentTime, None, XIGrabModeAsync,
-                                         XIGrabModeAsync, False, &evmask);
-            if (result != Success) {
-                grabbed = false;
-                qCDebug(lcQpaXInput, "XInput 2.2: failed to grab events for device %d on window %x (result %d)", id, w, result);
+                qCDebug(lcQpaXInput, "failed to grab events for device %d on window %x"
+                                     "(result %d)", id, w, result);
+            } else {
+                // Managed to grab at least one of master pointers, that should be enough
+                // to properly dismiss windows that rely on mouse grabbing.
+                ok = true;
             }
         }
+    } else { // ungrab
+        for (int id : m_xiMasterPointerIds) {
+            Status result = XIUngrabDevice(xDisplay, id, CurrentTime);
+            if (result != Success)
+                qCDebug(lcQpaXInput, "XIUngrabDevice failed - id: %d (result %d)", id, result);
+        }
+        // XIUngrabDevice does not seem to wait for a reply from X server (similar to
+        // xcb_ungrab_pointer). Ungrabbing won't fail, unless NoSuchExtension error
+        // has occurred due to a programming error somewhere else in the stack. That
+        // would mean that things will crash soon anyway.
+        ok = true;
     }
 
-    XIFreeDeviceInfo(info);
-
-    m_xiGrab = grabbed;
+    if (ok)
+        m_xiGrab = grab;
 
-    return grabbed;
+    return ok;
 }
 
 void QXcbConnection::xi2HandleHierarchyEvent(void *event)
diff --git a/src/plugins/platforms/xcb/qxcbwindow.cpp b/src/plugins/platforms/xcb/qxcbwindow.cpp
index e7096fb8f0..20d030b5c9 100644
--- a/src/plugins/platforms/xcb/qxcbwindow.cpp
+++ b/src/plugins/platforms/xcb/qxcbwindow.cpp
@@ -2560,6 +2560,9 @@ bool QXcbWindow::setMouseGrabEnabled(bool grab)
     if (!grab && connection()->mouseGrabber() == this)
         connection()->setMouseGrabber(nullptr);
 
+    if (grab && !connection()->canGrab())
+        return false;
+
 #if QT_CONFIG(xinput2)
     if (connection()->hasXInput2() && !connection()->xi2MouseEventsDisabled()) {
         bool result = connection()->xi2SetMouseGrabEnabled(m_window, grab);
@@ -2568,8 +2571,6 @@ bool QXcbWindow::setMouseGrabEnabled(bool grab)
         return result;
     }
 #endif
-    if (grab && !connection()->canGrab())
-        return false;
 
     if (!grab) {
         xcb_ungrab_pointer(xcb_connection(), XCB_TIME_CURRENT_TIME);