vitalif
/
kwin
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Drop Toplevel::windowId() 

A window id generated by WaylandServer may reference an X11 window
with the same id, which can result in undefined behavior.

The main reason why we needed windowId() was because of the task
switcher. However, since tabbox uses internal ids now, the window id
property can be dropped.
icc-effect-5.26.4
Vlad Zahorodnii 2020-11-04 17:49:10 +02:00
parent 6d7e8fc8a5
commit e398a7cd1a
17 changed files with 34 additions and 144 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

32
autotests/integration/stacking_order_test.cpp
View File

@ -360,7 +360,7 @@ void StackingOrderTest::testGroupTransientIsAboveWindowGroup()
X11Client *leader = windowCreatedSpy.first().first().value<X11Client *>();
QVERIFY(leader);
QVERIFY(leader->isActive());
QCOMPARE(leader->windowId(), leaderWid);
QCOMPARE(leader->window(), leaderWid);
QVERIFY(!leader->isTransient());
QCOMPARE(workspace()->stackingOrder(), (QList<Toplevel *>{leader}));
@ -375,7 +375,7 @@ void StackingOrderTest::testGroupTransientIsAboveWindowGroup()
X11Client *member1 = windowCreatedSpy.first().first().value<X11Client *>();
QVERIFY(member1);
QVERIFY(member1->isActive());
QCOMPARE(member1->windowId(), member1Wid);
QCOMPARE(member1->window(), member1Wid);
QCOMPARE(member1->group(), leader->group());
QVERIFY(!member1->isTransient());
@ -391,7 +391,7 @@ void StackingOrderTest::testGroupTransientIsAboveWindowGroup()
X11Client *member2 = windowCreatedSpy.first().first().value<X11Client *>();
QVERIFY(member2);
QVERIFY(member2->isActive());
QCOMPARE(member2->windowId(), member2Wid);
QCOMPARE(member2->window(), member2Wid);
QCOMPARE(member2->group(), leader->group());
QVERIFY(!member2->isTransient());
@ -429,7 +429,7 @@ void StackingOrderTest::testGroupTransientIsAboveWindowGroup()
X11Client *transient = windowCreatedSpy.first().first().value<X11Client *>();
QVERIFY(transient);
QVERIFY(transient->isActive());
QCOMPARE(transient->windowId(), transientWid);
QCOMPARE(transient->window(), transientWid);
QCOMPARE(transient->group(), leader->group());
QVERIFY(transient->isTransient());
QVERIFY(transient->groupTransient());
@ -474,7 +474,7 @@ void StackingOrderTest::testRaiseGroupTransient()
X11Client *leader = windowCreatedSpy.first().first().value<X11Client *>();
QVERIFY(leader);
QVERIFY(leader->isActive());
QCOMPARE(leader->windowId(), leaderWid);
QCOMPARE(leader->window(), leaderWid);
QVERIFY(!leader->isTransient());
QCOMPARE(workspace()->stackingOrder(), (QList<Toplevel *>{leader}));
@ -489,7 +489,7 @@ void StackingOrderTest::testRaiseGroupTransient()
X11Client *member1 = windowCreatedSpy.first().first().value<X11Client *>();
QVERIFY(member1);
QVERIFY(member1->isActive());
QCOMPARE(member1->windowId(), member1Wid);
QCOMPARE(member1->window(), member1Wid);
QCOMPARE(member1->group(), leader->group());
QVERIFY(!member1->isTransient());
@ -505,7 +505,7 @@ void StackingOrderTest::testRaiseGroupTransient()
X11Client *member2 = windowCreatedSpy.first().first().value<X11Client *>();
QVERIFY(member2);
QVERIFY(member2->isActive());
QCOMPARE(member2->windowId(), member2Wid);
QCOMPARE(member2->window(), member2Wid);
QCOMPARE(member2->group(), leader->group());
QVERIFY(!member2->isTransient());
@ -543,7 +543,7 @@ void StackingOrderTest::testRaiseGroupTransient()
X11Client *transient = windowCreatedSpy.first().first().value<X11Client *>();
QVERIFY(transient);
QVERIFY(transient->isActive());
QCOMPARE(transient->windowId(), transientWid);
QCOMPARE(transient->window(), transientWid);
QCOMPARE(transient->group(), leader->group());
QVERIFY(transient->isTransient());
QVERIFY(transient->groupTransient());
@ -608,7 +608,7 @@ void StackingOrderTest::testDeletedGroupTransient()
X11Client *leader = windowCreatedSpy.first().first().value<X11Client *>();
QVERIFY(leader);
QVERIFY(leader->isActive());
QCOMPARE(leader->windowId(), leaderWid);
QCOMPARE(leader->window(), leaderWid);
QVERIFY(!leader->isTransient());
QCOMPARE(workspace()->stackingOrder(), (QList<Toplevel *>{leader}));
@ -623,7 +623,7 @@ void StackingOrderTest::testDeletedGroupTransient()
X11Client *member1 = windowCreatedSpy.first().first().value<X11Client *>();
QVERIFY(member1);
QVERIFY(member1->isActive());
QCOMPARE(member1->windowId(), member1Wid);
QCOMPARE(member1->window(), member1Wid);
QCOMPARE(member1->group(), leader->group());
QVERIFY(!member1->isTransient());
@ -639,7 +639,7 @@ void StackingOrderTest::testDeletedGroupTransient()
X11Client *member2 = windowCreatedSpy.first().first().value<X11Client *>();
QVERIFY(member2);
QVERIFY(member2->isActive());
QCOMPARE(member2->windowId(), member2Wid);
QCOMPARE(member2->window(), member2Wid);
QCOMPARE(member2->group(), leader->group());
QVERIFY(!member2->isTransient());
@ -677,7 +677,7 @@ void StackingOrderTest::testDeletedGroupTransient()
X11Client *transient = windowCreatedSpy.first().first().value<X11Client *>();
QVERIFY(transient);
QVERIFY(transient->isActive());
QCOMPARE(transient->windowId(), transientWid);
QCOMPARE(transient->window(), transientWid);
QCOMPARE(transient->group(), leader->group());
QVERIFY(transient->isTransient());
QVERIFY(transient->groupTransient());
@ -728,7 +728,7 @@ void StackingOrderTest::testDontKeepAboveNonModalDialogGroupTransients()
X11Client *leader = windowCreatedSpy.first().first().value<X11Client *>();
QVERIFY(leader);
QVERIFY(leader->isActive());
QCOMPARE(leader->windowId(), leaderWid);
QCOMPARE(leader->window(), leaderWid);
QVERIFY(!leader->isTransient());
QCOMPARE(workspace()->stackingOrder(), (QList<Toplevel *>{leader}));
@ -743,7 +743,7 @@ void StackingOrderTest::testDontKeepAboveNonModalDialogGroupTransients()
X11Client *member1 = windowCreatedSpy.first().first().value<X11Client *>();
QVERIFY(member1);
QVERIFY(member1->isActive());
QCOMPARE(member1->windowId(), member1Wid);
QCOMPARE(member1->window(), member1Wid);
QCOMPARE(member1->group(), leader->group());
QVERIFY(!member1->isTransient());
@ -759,7 +759,7 @@ void StackingOrderTest::testDontKeepAboveNonModalDialogGroupTransients()
X11Client *member2 = windowCreatedSpy.first().first().value<X11Client *>();
QVERIFY(member2);
QVERIFY(member2->isActive());
QCOMPARE(member2->windowId(), member2Wid);
QCOMPARE(member2->window(), member2Wid);
QCOMPARE(member2->group(), leader->group());
QVERIFY(!member2->isTransient());
@ -776,7 +776,7 @@ void StackingOrderTest::testDontKeepAboveNonModalDialogGroupTransients()
X11Client *transient = windowCreatedSpy.first().first().value<X11Client *>();
QVERIFY(transient);
QVERIFY(transient->isActive());
QCOMPARE(transient->windowId(), transientWid);
QCOMPARE(transient->window(), transientWid);
QCOMPARE(transient->group(), leader->group());
QVERIFY(transient->isTransient());
QVERIFY(transient->groupTransient());

16
autotests/integration/x11_client_test.cpp
View File

@ -726,7 +726,7 @@ void X11ClientTest::testX11WindowId()
QVERIFY(windowCreatedSpy.wait());
X11Client *client = windowCreatedSpy.first().first().value<X11Client *>();
QVERIFY(client);
QCOMPARE(client->windowId(), w);
QCOMPARE(client->window(), w);
QVERIFY(client->isActive());
QCOMPARE(client->window(), w);
QCOMPARE(client->internalId().isNull(), false);
@ -801,7 +801,7 @@ void X11ClientTest::testCaptionChanges()
QVERIFY(windowCreatedSpy.wait());
X11Client *client = windowCreatedSpy.first().first().value<X11Client *>();
QVERIFY(client);
QCOMPARE(client->windowId(), w);
QCOMPARE(client->window(), w);
QCOMPARE(client->caption(), QStringLiteral("foo"));
QSignalSpy captionChangedSpy(client, &X11Client::captionChanged);
@ -873,7 +873,7 @@ void X11ClientTest::testCaptionMultipleWindows()
QVERIFY(windowCreatedSpy.wait());
X11Client *client = windowCreatedSpy.first().first().value<X11Client *>();
QVERIFY(client);
QCOMPARE(client->windowId(), w);
QCOMPARE(client->window(), w);
QCOMPARE(client->caption(), QStringLiteral("foo"));
// create second window with same caption
@ -895,7 +895,7 @@ void X11ClientTest::testCaptionMultipleWindows()
QVERIFY(windowCreatedSpy.wait());
X11Client *client2 = windowCreatedSpy.first().first().value<X11Client *>();
QVERIFY(client2);
QCOMPARE(client2->windowId(), w2);
QCOMPARE(client2->window(), w2);
QCOMPARE(client2->caption(), QStringLiteral("foo <2>\u200E"));
NETWinInfo info3(kwinApp()->x11Connection(), w2, kwinApp()->x11RootWindow(), NET::WMVisibleName | NET::WMVisibleIconName, NET::Properties2());
QCOMPARE(QByteArray(info3.visibleName()), QByteArrayLiteral("foo <2>\u200E"));
@ -948,7 +948,7 @@ void X11ClientTest::testFullscreenWindowGroups()
QVERIFY(windowCreatedSpy.wait());
X11Client *client = windowCreatedSpy.first().first().value<X11Client *>();
QVERIFY(client);
QCOMPARE(client->windowId(), w);
QCOMPARE(client->window(), w);
QCOMPARE(client->isActive(), true);
QCOMPARE(client->isFullScreen(), false);
@ -979,7 +979,7 @@ void X11ClientTest::testFullscreenWindowGroups()
X11Client *client2 = windowCreatedSpy.first().first().value<X11Client *>();
QVERIFY(client2);
QVERIFY(client != client2);
QCOMPARE(client2->windowId(), w2);
QCOMPARE(client2->window(), w2);
QCOMPARE(client2->isActive(), true);
QCOMPARE(client2->group(), client->group());
// first client should be moved back to normal layer
@ -1024,7 +1024,7 @@ void X11ClientTest::testActivateFocusedWindow()
QVERIFY(windowCreatedSpy.wait());
X11Client *client1 = windowCreatedSpy.first().first().value<X11Client *>();
QVERIFY(client1);
QCOMPARE(client1->windowId(), window1);
QCOMPARE(client1->window(), window1);
QCOMPARE(client1->isActive(), true);
// Create the second test window.
@ -1041,7 +1041,7 @@ void X11ClientTest::testActivateFocusedWindow()
QVERIFY(windowCreatedSpy.wait());
X11Client *client2 = windowCreatedSpy.last().first().value<X11Client *>();
QVERIFY(client2);
QCOMPARE(client2->windowId(), window2);
QCOMPARE(client2->window(), window2);
QCOMPARE(client2->isActive(), true);
// When the second test window is destroyed, the window manager will attempt to activate the

11
autotests/integration/xdgshellclient_test.cpp
View File

@ -80,7 +80,6 @@ private Q_SLOTS:
void testCaptionMultipleWindows();
void testUnresponsiveWindow_data();
void testUnresponsiveWindow();
void testX11WindowId();
void testAppMenu();
void testNoDecorationModeRequested();
void testSendClientWithTransientToDesktop();
@ -864,16 +863,6 @@ void TestXdgShellClient::testUnresponsiveWindow()
QVERIFY(elapsed2 > 1800); //second ping comes in a second later
}
void TestXdgShellClient::testX11WindowId()
{
QScopedPointer<Surface> surface(Test::createSurface());
QScopedPointer<XdgShellSurface> shellSurface(Test::createXdgShellStableSurface(surface.data()));
auto c = Test::renderAndWaitForShown(surface.data(), QSize(100, 50), Qt::blue);
QVERIFY(c);
QVERIFY(c->windowId() != 0);
QCOMPARE(c->window(), 0u);
}
void TestXdgShellClient::testAppMenu()
{
//register a faux appmenu client

2
autotests/integration/xwaylandserver_restart_test.cpp
View File

@ -102,7 +102,7 @@ void XwaylandServerRestartTest::testRestart()
QVERIFY(windowCreatedSpy.wait());
X11Client *client = windowCreatedSpy.last().first().value<X11Client *>();
QVERIFY(client);
QCOMPARE(client->windowId(), window);
QCOMPARE(client->window(), window);
QVERIFY(client->isDecorated());
// Render a frame to ensure that the compositor doesn't crash.

2
decorations/decoratedclient.cpp
View File

@ -163,7 +163,7 @@ DELEGATE2(QIcon, icon)
DELEGATE(bool, isKeepAbove, keepAbove)
DELEGATE(bool, isKeepBelow, keepBelow)
DELEGATE(bool, isShaded, isShade)
DELEGATE(WId, windowId, windowId)
DELEGATE(WId, windowId, window)
DELEGATE(WId, decorationId, frameId)
#undef DELEGATE

5
effects.cpp
View File

@ -1053,11 +1053,6 @@ EffectWindow* EffectsHandlerImpl::findWindow(WId id) const
return w->effectWindow();
if (Unmanaged* w = Workspace::self()->findUnmanaged(id))
return w->effectWindow();
if (waylandServer()) {
if (AbstractClient *w = waylandServer()->findClient(id)) {
return w->effectWindow();
}
}
return nullptr;
}

2
events.cpp
View File

@ -1173,7 +1173,7 @@ void X11Client::focusInEvent(xcb_focus_in_event_t *e)
if (workspace()->restoreFocus()) {
demandAttention();
} else {
qCWarning(KWIN_CORE, "Failed to restore focus. Activating 0x%x", windowId());
qCWarning(KWIN_CORE, "Failed to restore focus. Activating 0x%x", window());
setActive(true);
}
}

6
internal_client.cpp
View File

@ -27,7 +27,6 @@ namespace KWin
InternalClient::InternalClient(QWindow *window)
: m_internalWindow(window)
, m_windowId(window->winId())
, m_internalWindowFlags(window->flags())
{
connect(m_internalWindow, &QWindow::xChanged, this, &InternalClient::updateInternalWindowGeometry);
@ -252,11 +251,6 @@ bool InternalClient::isOutline() const
return false;
}
quint32 InternalClient::windowId() const
{
return m_windowId;
}
bool InternalClient::isShown(bool shaded_is_shown) const
{
Q_UNUSED(shaded_is_shown)

2
internal_client.h
View File

@ -52,7 +52,6 @@ public:
bool isInternal() const override;
bool isLockScreen() const override;
bool isOutline() const override;
quint32 windowId() const override;
bool isShown(bool shaded_is_shown) const override;
bool isHiddenInternal() const override;
void hideClient(bool hide) override;
@ -89,7 +88,6 @@ private:
QString m_captionSuffix;
double m_opacity = 1.0;
NET::WindowType m_windowType = NET::Normal;
quint32 m_windowId = 0;
Qt::WindowFlags m_internalWindowFlags = Qt::WindowFlags();
bool m_userNoBorder = false;

9
toplevel.cpp
View File

@ -66,7 +66,9 @@ QDebug operator<<(QDebug debug, const Toplevel *toplevel)
debug.nospace();
if (toplevel) {
debug << toplevel->metaObject()->className() << '(' << static_cast<const void *>(toplevel);
debug << ", windowId=0x" << Qt::hex << toplevel->windowId() << Qt::dec;
if (toplevel->window()) {
debug << ", windowId=0x" << Qt::hex << toplevel->window() << Qt::dec;
}
if (const KWaylandServer::SurfaceInterface *surface = toplevel->surface()) {
debug << ", surface=" << surface;
}
@ -809,11 +811,6 @@ QPointF Toplevel::mapToLocal(const QPointF &point) const
return point - bufferGeometry().topLeft();
}
quint32 Toplevel::windowId() const
{
return window();
}
QRect Toplevel::inputGeometry() const
{
return frameGeometry();

6
toplevel.h
View File

@ -112,7 +112,7 @@ class KWIN_EXPORT Toplevel : public QObject
Q_PROPERTY(QRect visibleRect READ visibleRect)
Q_PROPERTY(qreal opacity READ opacity WRITE setOpacity NOTIFY opacityChanged)
Q_PROPERTY(int screen READ screen NOTIFY screenChanged)
Q_PROPERTY(qulonglong windowId READ windowId CONSTANT)
Q_PROPERTY(qulonglong windowId READ window CONSTANT)
Q_PROPERTY(int desktop READ desktop)
/**
@ -295,10 +295,6 @@ public:
explicit Toplevel();
virtual xcb_window_t frameId() const;
xcb_window_t window() const;
/**
* @return a unique identifier for the Toplevel. On X11 same as @ref window
*/
virtual quint32 windowId() const;
/**
* Returns the geometry of the pixmap or buffer attached to this Toplevel.
*

2
useractions.cpp
View File

@ -188,7 +188,7 @@ void UserActionsMenu::helperDialog(const QString& message, AbstractClient* clien
args << QStringLiteral("--dontagain") << QLatin1String("kwin_dialogsrc:") + type;
}
if (client)
args << QStringLiteral("--embed") << QString::number(client->windowId());
args << QStringLiteral("--embed") << QString::number(client->window());
QtConcurrent::run([args]() {
KProcess::startDetached(QStringLiteral("kdialog"), args);
});

66
wayland_server.cpp
View File

@ -748,19 +748,6 @@ void WaylandServer::dispatch()
m_display->dispatchEvents(0);
}
static AbstractClient *findClientInList(const QList<AbstractClient *> &clients, quint32 id)
{
auto it = std::find_if(clients.begin(), clients.end(),
[id] (AbstractClient *c) {
return c->windowId() == id;
}
);
if (it == clients.end()) {
return nullptr;
}
return *it;
}
static AbstractClient *findClientInList(const QList<AbstractClient *> &clients, KWaylandServer::SurfaceInterface *surface)
{
auto it = std::find_if(clients.begin(), clients.end(),
@ -774,17 +761,6 @@ static AbstractClient *findClientInList(const QList<AbstractClient *> &clients,
return *it;
}
AbstractClient *WaylandServer::findClient(quint32 id) const
{
if (id == 0) {
return nullptr;
}
if (AbstractClient *c = findClientInList(m_clients, id)) {
return c;
}
return nullptr;
}
AbstractClient *WaylandServer::findClient(SurfaceInterface *surface) const
{
if (!surface) {
@ -806,48 +782,6 @@ XdgSurfaceClient *WaylandServer::findXdgSurfaceClient(SurfaceInterface *surface)
return qobject_cast<XdgSurfaceClient *>(findClient(surface));
}
quint32 WaylandServer::createWindowId(SurfaceInterface *surface)
{
auto it = m_clientIds.constFind(surface->client());
quint16 clientId = 0;
if (it != m_clientIds.constEnd()) {
clientId = it.value();
} else {
clientId = createClientId(surface->client());
}
Q_ASSERT(clientId != 0);
quint32 id = clientId;
// TODO: this does not prevent that two surfaces of same client get same id
id = (id << 16) | (surface->id() & 0xFFFF);
if (findClient(id)) {
qCWarning(KWIN_CORE) << "Invalid client windowId generated:" << id;
return 0;
}
return id;
}
quint16 WaylandServer::createClientId(ClientConnection *c)
{
const QSet<unsigned short> ids(m_clientIds.constBegin(), m_clientIds.constEnd());
quint16 id = 1;
if (!ids.isEmpty()) {
for (quint16 i = ids.count() + 1; i >= 1 ; i--) {
if (!ids.contains(i)) {
id = i;
break;
}
}
}
Q_ASSERT(!ids.contains(id));
m_clientIds.insert(c, id);
connect(c, &ClientConnection::disconnected, this,
[this] (ClientConnection *c) {
m_clientIds.remove(c);
}
);
return id;
}
bool WaylandServer::isScreenLocked() const
{
if (!hasScreenLockerIntegration()) {

4
wayland_server.h
View File

@ -142,7 +142,6 @@ public:
return m_clients;
}
void removeClient(AbstractClient *c);
AbstractClient *findClient(quint32 id) const;
AbstractClient *findClient(KWaylandServer::SurfaceInterface *surface) const;
XdgToplevelClient *findXdgToplevelClient(KWaylandServer::SurfaceInterface *surface) const;
XdgSurfaceClient *findXdgSurfaceClient(KWaylandServer::SurfaceInterface *surface) const;
@ -207,7 +206,6 @@ public:
return m_internalConnection.registry;
}
void dispatch();
quint32 createWindowId(KWaylandServer::SurfaceInterface *surface);
/**
* Struct containing information for a created Wayland connection through a
@ -253,7 +251,6 @@ Q_SIGNALS:
private:
int createScreenLockerConnection();
void shellClientShown(Toplevel *t);
quint16 createClientId(KWaylandServer::ClientConnection *c);
void destroyInternalConnection();
void initScreenLocker();
void registerXdgGenericClient(AbstractClient *client);
@ -296,7 +293,6 @@ private:
KWaylandServer::XdgForeignV2Interface *m_XdgForeign = nullptr;
KWaylandServer::KeyStateInterface *m_keyState = nullptr;
QList<AbstractClient *> m_clients;
QHash<KWaylandServer::ClientConnection*, quint16> m_clientIds;
InitializationFlags m_initFlags;
QVector<KWaylandServer::PlasmaShellSurfaceInterface*> m_plasmaShellSurfaces;
KWIN_SINGLETON(WaylandServer)

7
waylandclient.cpp
View File

@ -44,8 +44,6 @@ WaylandClient::WaylandClient(SurfaceInterface *surface)
setSurface(surface);
setupCompositing();
m_windowId = waylandServer()->createWindowId(surface);
connect(surface, &SurfaceInterface::shadowChanged,
this, &WaylandClient::updateShadow);
connect(this, &WaylandClient::frameGeometryChanged,
@ -94,11 +92,6 @@ QRect WaylandClient::transparentRect() const
return QRect();
}
quint32 WaylandClient::windowId() const
{
return m_windowId;
}
pid_t WaylandClient::pid() const
{
return surface()->client()->processId();

2
waylandclient.h
View File

@ -31,7 +31,6 @@ public:
void blockActivityUpdates(bool b = true) override;
QPoint clientContentPos() const override;
QRect transparentRect() const override;
quint32 windowId() const override;
pid_t pid() const override;
bool isLockScreen() const override;
bool isLocalhost() const override;
@ -88,7 +87,6 @@ private:
QRect m_requestedClientGeometry;
SyncMode m_positionSyncMode = SyncMode::Sync;
SyncMode m_sizeSyncMode = SyncMode::Sync;
quint32 m_windowId;
bool m_isHidden = false;
};

4
x11client.cpp
View File

@ -2015,10 +2015,10 @@ bool X11Client::takeFocus()
if (rules()->checkAcceptFocus(info->input())) {
xcb_void_cookie_t cookie = xcb_set_input_focus_checked(connection(),
XCB_INPUT_FOCUS_POINTER_ROOT,
windowId(), XCB_TIME_CURRENT_TIME);
window(), XCB_TIME_CURRENT_TIME);
ScopedCPointer<xcb_generic_error_t> error(xcb_request_check(connection(), cookie));
if (error) {
qCWarning(KWIN_CORE, "Failed to focus 0x%x (error %d)", windowId(), error->error_code);
qCWarning(KWIN_CORE, "Failed to focus 0x%x (error %d)", window(), error->error_code);
return false;
}
} else {