From d75e7a6d603132ce19a3318af68a71a98c77762e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Gr=C3=A4=C3=9Flin?= Date: Sun, 20 May 2012 15:52:24 +0200 Subject: [PATCH] Use smart pointers to protect access to TabBoxClient Client holds a SharedPointer to the TabBoxClient and only provides access to a WeakPointer which is passed to TabBox. ClientModel is adjusted to hold a list of WeakPointers instead of the direct pointers. This fixes the following reproducable crash: 1. Configure both primary and secondary TabBox with different layouts 2. Use primary TabBox 3. Close a window, best the one which used to be active 4. Use secondary TabBox -> Crash The reason is that the ClientModel still contains the pointer to the deleted TabBoxClient in step 3 and while creating the layout access to the TabBoxClient is needed to get the Client's icon. By using the weak pointer it can be ensured that we don't try to dereference the deleted pointer and prevent the crash. CCBUG: 290482 CCBUG: 285747 CCBUG: 237345 REVIEW: 105000 --- client.cpp | 5 +-- client.h | 6 ++-- tabbox/clientmodel.cpp | 68 +++++++++++++++++++++++----------------- tabbox/clientmodel.h | 2 +- tabbox/tabbox.cpp | 40 ++++++++++++++--------- tabbox/tabbox.h | 8 ++--- tabbox/tabboxhandler.cpp | 34 ++++++++++++++++---- tabbox/tabboxhandler.h | 12 +++---- 8 files changed, 109 insertions(+), 66 deletions(-) diff --git a/client.cpp b/client.cpp index 820a232b8b..dd2ad52ffe 100644 --- a/client.cpp +++ b/client.cpp @@ -187,7 +187,7 @@ Client::Client(Workspace* ws) #ifdef KWIN_BUILD_TABBOX // TabBoxClient - m_tabBoxClient = new TabBox::TabBoxClientImpl(this); + m_tabBoxClient = QSharedPointer(new TabBox::TabBoxClientImpl(this)); #endif geom = QRect(0, 0, 100, 100); // So that decorations don't start with size being (0,0) @@ -221,9 +221,6 @@ Client::~Client() assert(block_geometry_updates == 0); assert(!check_active_modal); delete bridge; -#ifdef KWIN_BUILD_TABBOX - delete m_tabBoxClient; -#endif } // Use destroyClient() or releaseWindow(), Client instances cannot be deleted directly diff --git a/client.h b/client.h index 0f0e9b8a93..c92e6eff86 100644 --- a/client.h +++ b/client.h @@ -621,8 +621,8 @@ public: }; void layoutDecorationRects(QRect &left, QRect &top, QRect &right, QRect &bottom, CoordinateMode mode) const; - TabBox::TabBoxClientImpl* tabBoxClient() const { - return m_tabBoxClient; + QWeakPointer tabBoxClient() const { + return m_tabBoxClient.toWeakRef(); } bool isFirstInTabBox() const { return m_firstInTabBox; @@ -945,7 +945,7 @@ private: // we (instead of Qt) initialize the Pixmaps, and have to free them bool m_responsibleForDecoPixmap; PaintRedirector* paintRedirector; - TabBox::TabBoxClientImpl* m_tabBoxClient; + QSharedPointer m_tabBoxClient; bool m_firstInTabBox; bool electricMaximizing; diff --git a/tabbox/clientmodel.cpp b/tabbox/clientmodel.cpp index b3f8954212..06a0c1a869 100644 --- a/tabbox/clientmodel.cpp +++ b/tabbox/clientmodel.cpp @@ -63,22 +63,26 @@ QVariant ClientModel::data(const QModelIndex& index, int role) const int clientIndex = index.row() * columnCount() + index.column(); if (clientIndex >= m_clientList.count()) return QVariant(); + QSharedPointer client = m_clientList[ clientIndex ].toStrongRef(); + if (!client) { + return QVariant(); + } switch(role) { case Qt::DisplayRole: case CaptionRole: - return m_clientList[ clientIndex ]->caption(); + return client->caption(); case ClientRole: - return qVariantFromValue((void*)m_clientList[ clientIndex ]); + return qVariantFromValue((void*)client.data()); case DesktopNameRole: { - return tabBox->desktopName(m_clientList[ clientIndex ]); + return tabBox->desktopName(client.data()); } case WIdRole: - return qulonglong(m_clientList[ clientIndex ]->window()); + return qulonglong(client->window()); case MinimizedRole: - return m_clientList[ clientIndex ]->isMinimized(); + return client->isMinimized(); case CloseableRole: //clients that claim to be first are not closeable - return m_clientList[ clientIndex ]->isCloseable() && !m_clientList[ clientIndex ]->isFirstInTabBox(); + return client->isCloseable() && !client->isFirstInTabBox(); default: return QVariant(); } @@ -87,7 +91,8 @@ QVariant ClientModel::data(const QModelIndex& index, int role) const QString ClientModel::longestCaption() const { QString caption; - foreach (TabBoxClient *client, m_clientList) { + foreach (QWeakPointer clientPointer, m_clientList) { + QSharedPointer client = clientPointer.toStrongRef(); if (client->caption().size() > caption.size()) { caption = client->caption(); } @@ -148,7 +153,7 @@ QModelIndex ClientModel::index(int row, int column, const QModelIndex& parent) c return createIndex(row, column); } -QModelIndex ClientModel::index(TabBoxClient* client) const +QModelIndex ClientModel::index(QWeakPointer client) const { if (!m_clientList.contains(client)) return QModelIndex(); @@ -165,31 +170,35 @@ void ClientModel::createClientList(bool partialReset) void ClientModel::createClientList(int desktop, bool partialReset) { - TabBoxClient* start = tabBox->activeClient(); + TabBoxClient* start = tabBox->activeClient().toStrongRef().data(); // TODO: new clients are not added at correct position - if (partialReset && !m_clientList.isEmpty()) - start = m_clientList.first(); + if (partialReset && !m_clientList.isEmpty()) { + QSharedPointer firstClient = m_clientList.first().toStrongRef(); + if (firstClient) { + start = firstClient.data(); + } + } m_clientList.clear(); - QList stickyClients; + QList< QWeakPointer< TabBoxClient > > stickyClients; switch(tabBox->config().clientSwitchingMode()) { case TabBoxConfig::FocusChainSwitching: { - TabBoxClient* c = tabBox->nextClientFocusChain(start); + TabBoxClient* c = tabBox->nextClientFocusChain(start).data(); TabBoxClient* stop = c; while (c) { - TabBoxClient* add = tabBox->clientToAddToList(c, desktop); - if (add != NULL) { - if (start == add) { + QWeakPointer add = tabBox->clientToAddToList(c, desktop); + if (!add.isNull()) { + if (start == add.data()) { m_clientList.removeAll(add); m_clientList.prepend(add); } else m_clientList += add; - if (add->isFirstInTabBox()) { + if (add.data()->isFirstInTabBox()) { stickyClients << add; } } - c = tabBox->nextClientFocusChain(c); + c = tabBox->nextClientFocusChain(c).data(); if (c == stop) break; @@ -199,25 +208,25 @@ void ClientModel::createClientList(int desktop, bool partialReset) case TabBoxConfig::StackingOrderSwitching: { // TODO: needs improvement TabBoxClientList stacking = tabBox->stackingOrder(); - TabBoxClient* c = stacking.first(); + TabBoxClient* c = stacking.first().data(); TabBoxClient* stop = c; int index = 0; while (c) { - TabBoxClient* add = tabBox->clientToAddToList(c, desktop); - if (add != NULL) { - if (start == add) { + QWeakPointer add = tabBox->clientToAddToList(c, desktop); + if (!add.isNull()) { + if (start == add.data()) { m_clientList.removeAll(add); m_clientList.prepend(add); } else m_clientList += add; - if (add->isFirstInTabBox()) { + if (add.data()->isFirstInTabBox()) { stickyClients << add; } } if (index >= stacking.size() - 1) { c = NULL; } else { - c = stacking[++index]; + c = stacking[++index].data(); } if (c == stop) @@ -226,13 +235,13 @@ void ClientModel::createClientList(int desktop, bool partialReset) break; } } - foreach (TabBoxClient *c, stickyClients) { + foreach (QWeakPointer< TabBoxClient > c, stickyClients) { m_clientList.removeAll(c); m_clientList.prepend(c); } if (tabBox->config().showDesktopMode() == TabBoxConfig::ShowDesktopClient || m_clientList.isEmpty()) { - TabBoxClient* desktopClient = tabBox->desktopClient(); - if (desktopClient) + QWeakPointer desktopClient = tabBox->desktopClient(); + if (!desktopClient.isNull()) m_clientList.append(desktopClient); } reset(); @@ -244,7 +253,10 @@ void ClientModel::close(int i) if (!ind.isValid()) { return; } - m_clientList.at(i)->close(); + QSharedPointer client = m_clientList.at(i).toStrongRef(); + if (client) { + client->close(); + } } void ClientModel::activate(int i) diff --git a/tabbox/clientmodel.h b/tabbox/clientmodel.h index 6d646ab4d2..bc737c9a18 100644 --- a/tabbox/clientmodel.h +++ b/tabbox/clientmodel.h @@ -71,7 +71,7 @@ public: * @return Returns the ModelIndex of given TabBoxClient or an invalid ModelIndex * if the model does not contain the given TabBoxClient. */ - QModelIndex index(TabBoxClient* client) const; + QModelIndex index(QWeakPointer client) const; /** * Generates a new list of TabBoxClients based on the current config. diff --git a/tabbox/tabbox.cpp b/tabbox/tabbox.cpp index 62b9df0efd..098d7598c0 100644 --- a/tabbox/tabbox.cpp +++ b/tabbox/tabbox.cpp @@ -93,14 +93,14 @@ QString TabBoxHandlerImpl::desktopName(int desktop) const return Workspace::self()->desktopName(desktop); } -TabBoxClient* TabBoxHandlerImpl::nextClientFocusChain(TabBoxClient* client) const +QWeakPointer TabBoxHandlerImpl::nextClientFocusChain(TabBoxClient* client) const { if (TabBoxClientImpl* c = static_cast< TabBoxClientImpl* >(client)) { Client* next = Workspace::self()->tabBox()->nextClientFocusChain(c->client()); if (next) return next->tabBoxClient(); } - return NULL; + return QWeakPointer(); } int TabBoxHandlerImpl::nextDesktopFocusChain(int desktop) const @@ -113,12 +113,12 @@ int TabBoxHandlerImpl::numberOfDesktops() const return Workspace::self()->numberOfDesktops(); } -TabBoxClient* TabBoxHandlerImpl::activeClient() const +QWeakPointer TabBoxHandlerImpl::activeClient() const { if (Workspace::self()->activeClient()) return Workspace::self()->activeClient()->tabBoxClient(); else - return NULL; + return QWeakPointer(); } bool TabBoxHandlerImpl::checkDesktop(TabBoxClient* client, int desktop) const @@ -153,26 +153,35 @@ bool TabBoxHandlerImpl::checkApplications(TabBoxClient* client) const { Client* current = (static_cast< TabBoxClientImpl* >(client))->client(); TabBoxClientImpl* c; - QListIterator< TabBoxClient* > i(clientList()); + QListIterator< QWeakPointer > i(clientList()); switch (config().clientApplicationsMode()) { case TabBoxConfig::OneWindowPerApplication: // check if the list already contains an entry of this application while (i.hasNext()) { - if ((c = dynamic_cast< TabBoxClientImpl* >(i.next()))) { + QSharedPointer client = i.next().toStrongRef(); + if (!client) { + continue; + } + if ((c = dynamic_cast< TabBoxClientImpl* >(client.data()))) { if (c->client()->resourceClass() == current->resourceClass()) { return false; } } } return true; - case TabBoxConfig::AllWindowsCurrentApplication: - if ((c = dynamic_cast< TabBoxClientImpl* >(tabBox->activeClient()))) { + case TabBoxConfig::AllWindowsCurrentApplication: { + QSharedPointer pointer = tabBox->activeClient().toStrongRef(); + if (!pointer) { + return false; + } + if ((c = dynamic_cast< TabBoxClientImpl* >(pointer.data()))) { if (c->client()->resourceClass() == current->resourceClass()) { return true; } } return false; + } default: // TabBoxConfig::AllWindowsAllApplications return true; } @@ -205,7 +214,7 @@ bool TabBoxHandlerImpl::checkMultiScreen(TabBoxClient* client) const } } -TabBoxClient* TabBoxHandlerImpl::clientToAddToList(TabBoxClient* client, int desktop) const +QWeakPointer TabBoxHandlerImpl::clientToAddToList(TabBoxClient* client, int desktop) const { Client* ret = NULL; Client* current = (static_cast< TabBoxClientImpl* >(client))->client(); @@ -230,7 +239,7 @@ TabBoxClient* TabBoxHandlerImpl::clientToAddToList(TabBoxClient* client, int des if (ret) return ret->tabBoxClient(); else - return NULL; + return QWeakPointer(); } TabBoxClientList TabBoxHandlerImpl::stackingOrder() const @@ -268,7 +277,7 @@ void TabBoxHandlerImpl::elevateClient(TabBoxClient *c, WId tabbox, bool b) const } -TabBoxClient* TabBoxHandlerImpl::desktopClient() const +QWeakPointer TabBoxHandlerImpl::desktopClient() const { foreach (Toplevel *toplevel, Workspace::self()->stackingOrder()) { Client *client = qobject_cast(toplevel); @@ -276,7 +285,7 @@ TabBoxClient* TabBoxHandlerImpl::desktopClient() const return client->tabBoxClient(); } } - return NULL; + return QWeakPointer(); } void TabBoxHandlerImpl::showOutline(const QRect &outline) @@ -575,8 +584,11 @@ ClientList TabBox::currentClientList() { TabBoxClientList list = m_tabBox->clientList(); ClientList ret; - foreach (const TabBoxClient * client, list) { - if (const TabBoxClientImpl* c = static_cast< const TabBoxClientImpl* >(client)) + foreach (QWeakPointer clientPointer, list) { + QSharedPointer client = clientPointer.toStrongRef(); + if (!client) + continue; + if (const TabBoxClientImpl* c = static_cast< const TabBoxClientImpl* >(client.data())) ret.append(c->client()); } return ret; diff --git a/tabbox/tabbox.h b/tabbox/tabbox.h index 70c30e447b..bf69ad9184 100644 --- a/tabbox/tabbox.h +++ b/tabbox/tabbox.h @@ -46,19 +46,19 @@ public: virtual ~TabBoxHandlerImpl(); virtual int activeScreen() const; - virtual TabBoxClient* activeClient() const; + virtual QWeakPointer< TabBoxClient > activeClient() const; virtual int currentDesktop() const; virtual QString desktopName(TabBoxClient* client) const; virtual QString desktopName(int desktop) const; - virtual TabBoxClient* nextClientFocusChain(TabBoxClient* client) const; + virtual QWeakPointer< TabBoxClient > nextClientFocusChain(TabBoxClient* client) const; virtual int nextDesktopFocusChain(int desktop) const; virtual int numberOfDesktops() const; virtual TabBoxClientList stackingOrder() const; virtual void elevateClient(TabBoxClient* c, WId tabbox, bool elevate) const; virtual void raiseClient(TabBoxClient *client) const; virtual void restack(TabBoxClient *c, TabBoxClient *under); - virtual TabBoxClient* clientToAddToList(TabBoxClient* client, int desktop) const; - virtual TabBoxClient* desktopClient() const; + virtual QWeakPointer< TabBoxClient > clientToAddToList(KWin::TabBox::TabBoxClient* client, int desktop) const; + virtual QWeakPointer< TabBoxClient > desktopClient() const; virtual void hideOutline(); virtual void showOutline(const QRect &outline); virtual QVector< Window > outlineWindowIds() const; diff --git a/tabbox/tabboxhandler.cpp b/tabbox/tabboxhandler.cpp index cd21484f59..242dcb3f15 100644 --- a/tabbox/tabboxhandler.cpp +++ b/tabbox/tabboxhandler.cpp @@ -158,8 +158,14 @@ void TabBoxHandlerPrivate::updateHighlightWindows() // TODO if ( (lastRaisedClientWasMinimized = lastRaisedClient->isMinimized()) ) // lastRaisedClient->setMinimized( false ); TabBoxClientList order = q->stackingOrder(); - int succIdx = order.indexOf(lastRaisedClient) + 1; // this is likely related to the index parameter?! - lastRaisedClientSucc = (succIdx < order.count()) ? order.at(succIdx) : 0; + int succIdx = order.count() + 1; + for (int i=0; iraiseClient(lastRaisedClient); } } @@ -415,7 +421,7 @@ bool TabBoxHandler::containsPos(const QPoint& pos) const return w->geometry().contains(pos); } -QModelIndex TabBoxHandler::index(KWin::TabBox::TabBoxClient* client) const +QModelIndex TabBoxHandler::index(QWeakPointer client) const { return d->clientModel()->index(client); } @@ -440,13 +446,29 @@ TabBoxClient* TabBoxHandler::client(const QModelIndex& index) const void TabBoxHandler::createModel(bool partialReset) { switch(d->config.tabBoxMode()) { - case TabBoxConfig::ClientTabBox: + case TabBoxConfig::ClientTabBox: { d->clientModel()->createClientList(partialReset); - if (d->lastRaisedClient && !stackingOrder().contains(d->lastRaisedClient)) + // TODO: C++11 use lambda function + bool lastRaised = false; + bool lastRaisedSucc = false; + foreach (QWeakPointer clientPointer, stackingOrder()) { + QSharedPointer client = clientPointer.toStrongRef(); + if (!client) { + continue; + } + if (client.data() == d->lastRaisedClient) { + lastRaised = true; + } + if (client.data() == d->lastRaisedClientSucc) { + lastRaisedSucc = true; + } + } + if (d->lastRaisedClient && !lastRaised) d->lastRaisedClient = 0; - if (d->lastRaisedClientSucc && !stackingOrder().contains(d->lastRaisedClientSucc)) + if (d->lastRaisedClientSucc && !lastRaisedSucc) d->lastRaisedClientSucc = 0; break; + } case TabBoxConfig::DesktopTabBox: d->desktopModel()->createDesktopList(); break; diff --git a/tabbox/tabboxhandler.h b/tabbox/tabboxhandler.h index 14427e8ca6..976f201735 100644 --- a/tabbox/tabboxhandler.h +++ b/tabbox/tabboxhandler.h @@ -81,7 +81,7 @@ class ClientModel; class TabBoxConfig; class TabBoxClient; class TabBoxHandlerPrivate; -typedef QList< TabBoxClient* > TabBoxClientList; +typedef QList< QWeakPointer< TabBoxClient > > TabBoxClientList; /** * This class is a wrapper around KWin Workspace. It is used for accessing the @@ -105,12 +105,12 @@ public: * @return The current active TabBoxClient or NULL * if there is no active client. */ - virtual TabBoxClient* activeClient() const = 0; + virtual QWeakPointer activeClient() const = 0; /** * @param client The client which is starting point to find the next client * @return The next TabBoxClient in focus chain */ - virtual TabBoxClient* nextClientFocusChain(TabBoxClient* client) const = 0; + virtual QWeakPointer nextClientFocusChain(TabBoxClient* client) const = 0; /** * @param client The client whose desktop name should be retrieved * @return The desktop name of the given TabBoxClient. If the client is @@ -172,11 +172,11 @@ public: * @param allDesktops Add clients from all desktops or only from current * @return The client to be included in the list or NULL if it isn't to be included */ - virtual TabBoxClient* clientToAddToList(TabBoxClient* client, int desktop) const = 0; + virtual QWeakPointer clientToAddToList(TabBoxClient* client, int desktop) const = 0; /** * @return The first desktop window in the stacking order. */ - virtual TabBoxClient* desktopClient() const = 0; + virtual QWeakPointer desktopClient() const = 0; /** * Activates the currently selected client and closes the TabBox. **/ @@ -278,7 +278,7 @@ public: * if the model does not contain the given TabBoxClient. * @see ClientModel::index */ - QModelIndex index(TabBoxClient* client) const; + QModelIndex index(QWeakPointer client) const; /** * @return Returns the current list of TabBoxClients. * If TabBoxMode is not TabBoxConfig::ClientTabBox an empty list will