From e2d46c574c04beefb4265fb71d43caee30c7d265 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Gr=C3=A4=C3=9Flin?= Date: Tue, 14 Jun 2016 14:46:39 +0200 Subject: [PATCH] [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 --- .../autotests/client/test_datadevice.cpp | 59 +++++++++++++++++++ src/wayland/datadevice_interface.cpp | 3 + src/wayland/seat_interface.cpp | 26 ++++---- src/wayland/server/resource.cpp | 1 + src/wayland/server/resource.h | 8 +++ 5 files changed, 84 insertions(+), 13 deletions(-) diff --git a/src/wayland/autotests/client/test_datadevice.cpp b/src/wayland/autotests/client/test_datadevice.cpp index 185849f091..e9971b1f33 100644 --- a/src/wayland/autotests/client/test_datadevice.cpp +++ b/src/wayland/autotests/client/test_datadevice.cpp @@ -26,6 +26,7 @@ License along with this library. If not, see . #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(m_dataDeviceManager->getDataDevice(m_seat)); + QVERIFY(dataDevice->isValid()); + QVERIFY(dataDeviceCreatedSpy.wait()); + auto serverDataDevice = dataDeviceCreatedSpy.first().first().value(); + QVERIFY(serverDataDevice); + QScopedPointer keyboard(m_seat->createKeyboard()); + QVERIFY(keyboard->isValid()); + QSignalSpy surfaceCreatedSpy(m_compositorInterface, &CompositorInterface::surfaceCreated); + QVERIFY(surfaceCreatedSpy.isValid()); + QScopedPointer surface(m_compositor->createSurface()); + QVERIFY(surface->isValid()); + QVERIFY(surfaceCreatedSpy.wait()); + + auto serverSurface = surfaceCreatedSpy.first().first().value(); + QVERIFY(serverSurface); + m_seatInterface->setFocusedKeyboardSurface(serverSurface); + + // now set the selection + QScopedPointer 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; diff --git a/src/wayland/datadevice_interface.cpp b/src/wayland/datadevice_interface.cpp index e06432e416..a42e52c99e 100644 --- a/src/wayland/datadevice_interface.cpp +++ b/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)); diff --git a/src/wayland/seat_interface.cpp b/src/wayland/seat_interface.cpp index 9bfcdcf61f..30cf8841fc 100644 --- a/src/wayland/seat_interface.cpp +++ b/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); diff --git a/src/wayland/server/resource.cpp b/src/wayland/server/resource.cpp index 4c9fbff9f8..eb735e9dd5 100644 --- a/src/wayland/server/resource.cpp +++ b/src/wayland/server/resource.cpp @@ -64,6 +64,7 @@ void Resource::Private::unbind(wl_resource *r) { Private *p = cast(r); p->resource = nullptr; + emit p->q->unbound(); p->q->deleteLater(); } diff --git a/src/wayland/server/resource.h b/src/wayland/server/resource.h index e2edc1070c..0cf51fb44d 100644 --- a/src/wayland/server/resource.h +++ b/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);