[server] Introduce a Resource::unbound signal emitted from unbind handler

Summary:
So far for internal cleanup we mostly listen to QObject::destroyed.
When a Resource gets unbind the wl_resource is set to null and the
Resource gets deleteLater. This creates a short time frame when the
Resource is still there, but the wl_resource is null. For most internal
usages the Resource is completely useless at that point and should no
longer be considered. So far it was still considered and could hit
crashers, if a code path did not nullptr check. Unfortunately
libwayland-server is not nullptr safe: if called with a null value it
tends to crash.

So this check introduces a new signal unbound which can be listend to
in addition to the destroyed signal. It's used in SeatInterface for
DataDeviceInterface, where we experienced a crash related to that.

A test case is added which exposes the crash, but it already needs
the unbound signal to get into the crashy condition. The actual crash
is fixed twice - with the help of the unbound signal, but also by
introducing the nullptr check where it's needed.

Reviewers: #plasma_on_wayland

Subscribers: plasma-devel

Tags: #plasma_on_wayland

Differential Revision: https://phabricator.kde.org/D1868

src/wayland/autotests/client/test_datadevice.cpp

@@ -26,6 +26,7 @@ License along with this library.  If not, see <http://www.gnu.org/licenses/>.
 #include "../../src/client/datadevicemanager.h"
 #include "../../src/client/datasource.h"
 #include "../../src/client/compositor.h"
+#include "../../src/client/keyboard.h"
 #include "../../src/client/pointer.h"
 #include "../../src/client/registry.h"
 #include "../../src/client/seat.h"
@@ -51,6 +52,7 @@ private Q_SLOTS:
     void testDrag();
     void testDragInternally();
     void testSetSelection();
+    void testSendSelectionOnSeat();
     void testDestroy();
 
 private:
@@ -380,6 +382,63 @@ void TestDataDevice::testSetSelection()
     QVERIFY(!deviceInterface->selection());
 }
 
+void TestDataDevice::testSendSelectionOnSeat()
+{
+    // this test verifies that the selection is sent when setting a focused keyboard
+    using namespace KWayland::Client;
+    using namespace KWayland::Server;
+    // first add keyboard support to Seat
+    QSignalSpy keyboardChangedSpy(m_seat, &Seat::hasKeyboardChanged);
+    QVERIFY(keyboardChangedSpy.isValid());
+    m_seatInterface->setHasKeyboard(true);
+    QVERIFY(keyboardChangedSpy.wait());
+    // now create DataDevice, Keyboard and a Surface
+    QSignalSpy dataDeviceCreatedSpy(m_dataDeviceManagerInterface, &DataDeviceManagerInterface::dataDeviceCreated);
+    QVERIFY(dataDeviceCreatedSpy.isValid());
+    QScopedPointer<DataDevice> dataDevice(m_dataDeviceManager->getDataDevice(m_seat));
+    QVERIFY(dataDevice->isValid());
+    QVERIFY(dataDeviceCreatedSpy.wait());
+    auto serverDataDevice = dataDeviceCreatedSpy.first().first().value<DataDeviceInterface*>();
+    QVERIFY(serverDataDevice);
+    QScopedPointer<Keyboard> keyboard(m_seat->createKeyboard());
+    QVERIFY(keyboard->isValid());
+    QSignalSpy surfaceCreatedSpy(m_compositorInterface, &CompositorInterface::surfaceCreated);
+    QVERIFY(surfaceCreatedSpy.isValid());
+    QScopedPointer<Surface> surface(m_compositor->createSurface());
+    QVERIFY(surface->isValid());
+    QVERIFY(surfaceCreatedSpy.wait());
+
+    auto serverSurface = surfaceCreatedSpy.first().first().value<SurfaceInterface*>();
+    QVERIFY(serverSurface);
+    m_seatInterface->setFocusedKeyboardSurface(serverSurface);
+
+    // now set the selection
+    QScopedPointer<DataSource> dataSource(m_dataDeviceManager->createDataSource());
+    QVERIFY(dataSource->isValid());
+    dataSource->offer(QStringLiteral("text/plain"));
+    dataDevice->setSelection(1, dataSource.data());
+    // we should get a selection offered for that on the data device
+    QSignalSpy selectionOfferedSpy(dataDevice.data(), &DataDevice::selectionOffered);
+    QVERIFY(selectionOfferedSpy.isValid());
+    QVERIFY(selectionOfferedSpy.wait());
+    QCOMPARE(selectionOfferedSpy.count(), 1);
+
+    // now unfocus the keyboard
+    m_seatInterface->setFocusedKeyboardSurface(nullptr);
+    // if setting the same surface again, we should get another offer
+    m_seatInterface->setFocusedKeyboardSurface(serverSurface);
+    QVERIFY(selectionOfferedSpy.wait());
+    QCOMPARE(selectionOfferedSpy.count(), 2);
+
+    // now let's try to destroy the data device and set a focused keyboard just while the data device is being destroyedd
+    m_seatInterface->setFocusedKeyboardSurface(nullptr);
+    QSignalSpy unboundSpy(serverDataDevice, &Resource::unbound);
+    QVERIFY(unboundSpy.isValid());
+    dataDevice.reset();
+    QVERIFY(unboundSpy.wait());
+    m_seatInterface->setFocusedKeyboardSurface(serverSurface);
+}
+
 void TestDataDevice::testDestroy()
 {
     using namespace KWayland::Client;

src/wayland/datadevice_interface.cpp

@@ -130,6 +130,9 @@ void DataDeviceInterface::Private::setSelection(DataSourceInterface *dataSource)
 
 DataOfferInterface *DataDeviceInterface::Private::createDataOffer(DataSourceInterface *source)
 {
+    if (!resource) {
+        return nullptr;
+    }
     Q_Q(DataDeviceInterface);
     DataOfferInterface *offer = new DataOfferInterface(source, q, resource);
     auto c = q->global()->display()->getConnection(wl_resource_get_client(resource));

src/wayland/seat_interface.cpp

@@ -214,21 +214,21 @@ void SeatInterface::Private::registerDataDevice(DataDeviceInterface *dataDevice)
 {
     Q_ASSERT(dataDevice->seat() == q);
     dataDevices << dataDevice;
-    QObject::connect(dataDevice, &QObject::destroyed, q,
-        [this, dataDevice] {
-            dataDevices.removeAt(dataDevices.indexOf(dataDevice));
-            if (keys.focus.selection == dataDevice) {
-                keys.focus.selection = nullptr;
-            }
-            if (currentSelection == dataDevice) {
-                // current selection is cleared
-                currentSelection = nullptr;
-                if (keys.focus.selection) {
-                    keys.focus.selection->sendClearSelection();
-                }
+    auto dataDeviceCleanup = [this, dataDevice] {
+        dataDevices.removeOne(dataDevice);
+        if (keys.focus.selection == dataDevice) {
+            keys.focus.selection = nullptr;
+        }
+        if (currentSelection == dataDevice) {
+            // current selection is cleared
+            currentSelection = nullptr;
+            if (keys.focus.selection) {
+                keys.focus.selection->sendClearSelection();
             }
         }
-    );
+    };
+    QObject::connect(dataDevice, &QObject::destroyed, q, dataDeviceCleanup);
+    QObject::connect(dataDevice, &Resource::unbound, q, dataDeviceCleanup);
     QObject::connect(dataDevice, &DataDeviceInterface::selectionChanged, q,
         [this, dataDevice] {
             updateSelection(dataDevice, true);

src/wayland/server/resource.cpp

@@ -64,6 +64,7 @@ void Resource::Private::unbind(wl_resource *r)
 {
     Private *p = cast<Private>(r);
     p->resource = nullptr;
+    emit p->q->unbound();
     p->q->deleteLater();
 }
 

src/wayland/server/resource.h

@@ -74,6 +74,14 @@ public:
      **/
     quint32 id() const;
 
+Q_SIGNALS:
+    /**
+     * This signal is emitted when the client unbound this Resource.
+     * The Resource will be deleted in the next event cycle after this event.
+     * @since 5.24
+     **/
+    void unbound();
+
 protected:
     class Private;
     explicit Resource(Private *d, QObject *parent = nullptr);