[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kwin
Subject:    [PATCH] Reentrancy problem in KWindowSystem
From:       Christoph Feck <christoph () maxiom ! de>
Date:       2009-07-18 12:21:51
Message-ID: 200907181421.51978.christoph () maxiom ! de
[Download RAW message or body]

Hi (especially Lubos :),

Still trying to resolve bug 170519 I found a possible reason and fix.

The current implementation of update() uses some arrays to check which clients 
are added and removed. During this, it emit signals with its findings.

The problem is for example in ksystray app that connects to windowAdded() 
signal, and calls for example KWindowSystem::activate() during its connected 
handler. This in turn calls into update() of netwm.cpp, but this function is 
not prepared to be called again, as the p->clients list is still in use.

The attached patch tries to fix it, and I am looking for comments, as I am not 
really familiar with the stuff.

Thanks,
Christoph Feck (kdepepo)

References:

https://bugs.kde.org/show_bug.cgi?id=170519
https://bugs.kde.org/show_bug.cgi?id=196552
https://bugs.kde.org/show_bug.cgi?id=199242

["windowmanagement.diff" (text/x-diff)]

Index: windowmanagement/netwm.cpp
===================================================================
--- windowmanagement/netwm.cpp	(Revision 998676)
+++ windowmanagement/netwm.cpp	(Arbeitskopie)
@@ -2108,6 +2108,9 @@
     }
 
     if (dirty & ClientList) {
+        QList<Window> clientsToRemove;
+        QList<Window> clientsToAdd;
+
         bool read_ok = false;
 	if (XGetWindowProperty(p->display, p->root, net_client_list,
 			       0l, MAX_PROP_SIZE, False, XA_WINDOW, &type_ret,
@@ -2126,16 +2129,16 @@
 
 			while (old_index < old_count || new_index < new_count) {
 			    if (old_index == old_count) {
-				addClient(wins[new_index++]);
+				clientsToAdd.append(wins[new_index++]);
 			    } else if (new_index == new_count) {
-				removeClient(p->clients[old_index++]);
+				clientsToRemove.append(p->clients[old_index++]);
 			    } else {
 				if (p->clients[old_index] <
 				    wins[new_index]) {
-				    removeClient(p->clients[old_index++]);
+				    clientsToRemove.append(p->clients[old_index++]);
 				} else if (wins[new_index] <
 					   p->clients[old_index]) {
-				    addClient(wins[new_index++]);
+				    clientsToAdd.append(wins[new_index++]);
 				} else {
 				    new_index++;
 				    old_index++;
@@ -2152,7 +2155,7 @@
 
 		    unsigned long n;
 		    for (n = 0; n < nitems_ret; n++) {
-			addClient(wins[n]);
+			clientsToAdd.append(wins[n]);
 		    }
 		}
 
@@ -2166,7 +2169,7 @@
 	}
         if( !read_ok ) {
             for( unsigned int i = 0; i < p->clients_count; ++ i )
-	        removeClient(p->clients[i]);
+	        clientsToRemove.append(p->clients[i]);
             p->clients_count = 0;
             delete[] p->clients;
             p->clients = NULL;
@@ -2176,6 +2179,12 @@
 	fprintf(stderr, "NETRootInfo::update: client list updated (%ld clients)\n",
 		p->clients_count);
 #endif
+        for (int i = 0; i < clientsToRemove.size(); ++i) {
+            removeClient(clientsToRemove.at(i));
+        }
+        for (int i = 0; i < clientsToAdd.size(); ++i) {
+            addClient(clientsToAdd.at(i));
+        }
     }
 
     if (dirty & ClientListStacking) {


_______________________________________________
kwin mailing list
kwin@kde.org
https://mail.kde.org/mailman/listinfo/kwin


[prev in list] [next in list] [prev in thread] [next in thread]