D6569: [client] Track all created ConnectionThreads and add API to access them
This revision was automatically updated to reflect the committed changes. Closed by commit R127:e4c90bf90717: [client] Track all created ConnectionThreads and add API to access them (authored by graesslin). REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6569?vs=16660=16957 REVISION DETAIL https://phabricator.kde.org/D6569 AFFECTED FILES autotests/client/test_wayland_connection_thread.cpp src/client/connection_thread.cpp src/client/connection_thread.h To: graesslin, #frameworks, #plasma, #kwin, davidedmundson Cc: davidedmundson, dfaure, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas
D6569: [client] Track all created ConnectionThreads and add API to access them
graesslin updated this revision to Diff 16660. graesslin added a comment. Restricted Application edited projects, added Plasma on Wayland; removed Plasma. Make this compile. Looks like kdevelop fooled me and compiled KWin instead of KWayland (that happens all the time to me) REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6569?vs=16406=16660 BRANCH all-connection-threads REVISION DETAIL https://phabricator.kde.org/D6569 AFFECTED FILES autotests/client/test_wayland_connection_thread.cpp src/client/connection_thread.cpp src/client/connection_thread.h To: graesslin, #frameworks, #plasma, #kwin, davidedmundson Cc: davidedmundson, dfaure, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas
D6569: [client] Track all created ConnectionThreads and add API to access them
graesslin updated this revision to Diff 16406. graesslin added a comment. Restricted Application edited projects, added Plasma; removed Plasma on Wayland. Use mutex to protect the potentially shared data structure REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6569?vs=16341=16406 BRANCH all-connection-threads REVISION DETAIL https://phabricator.kde.org/D6569 AFFECTED FILES autotests/client/test_wayland_connection_thread.cpp src/client/connection_thread.cpp src/client/connection_thread.h To: graesslin, #frameworks, #plasma, #kwin, davidedmundson Cc: davidedmundson, dfaure, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas
D6569: [client] Track all created ConnectionThreads and add API to access them
davidedmundson accepted this revision. This revision is now accepted and ready to land. REPOSITORY R127 KWayland BRANCH all-connection-threads REVISION DETAIL https://phabricator.kde.org/D6569 To: graesslin, #frameworks, #plasma, #kwin, davidedmundson Cc: davidedmundson, dfaure, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas
D6569: [client] Track all created ConnectionThreads and add API to access them
graesslin added a comment. @davidedmundson if you have another idea how to tackle the problem I'm absolutely open to it. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D6569 To: graesslin, #frameworks, #plasma, #kwin Cc: davidedmundson, dfaure, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas
D6569: [client] Track all created ConnectionThreads and add API to access them
graesslin added a comment. In https://phabricator.kde.org/D6569#123081, @davidedmundson wrote: > I don't think this is necessary. > > The only times we want need to delete the connection ourselves is when client code is creating it via ConnectionThread::fromApplication. If they create it themselves, we don't want to, and there's no need to track it. > > For the one instance in the bug you're fixing we have the ConnectionThread object already in kwin as it's in kwin's own QPA. The problem is not the one KWin creates, but the one Breeze creates and the one the KWindowSystem plugin creates and the one plasma-integration and so on and on... REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D6569 To: graesslin, #frameworks, #plasma, #kwin Cc: davidedmundson, dfaure, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas
D6569: [client] Track all created ConnectionThreads and add API to access them
davidedmundson added a comment. I don't think this is necessary. The only times we want need to delete the connection ourselves is when client code is creating it via ConnectionThread::fromApplication. If they create it themselves, we don't want to, and there's no need to track it. For the one instance in the bug you're fixing we have the ConnectionThread object already in kwin as it's in kwin's own QPA. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D6569 To: graesslin, #frameworks, #plasma, #kwin Cc: davidedmundson, dfaure, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas
D6569: [client] Track all created ConnectionThreads and add API to access them
graesslin added a dependent revision: D6571: Delete all Wayland connections by plugins prior to own Wayland connection. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D6569 To: graesslin, #frameworks, #plasma, #kwin Cc: dfaure, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas
D6569: [client] Track all created ConnectionThreads and add API to access them
dfaure added a comment. Isn't a mutex needed to protect this data structure that is apparently modified by different threads? REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D6569 To: graesslin, #frameworks, #plasma, #kwin Cc: dfaure, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas
D6569: [client] Track all created ConnectionThreads and add API to access them
graesslin created this revision. Restricted Application added projects: Plasma on Wayland, Frameworks. Restricted Application added a subscriber: plasma-devel. REVISION SUMMARY This is change needed by KWin. KWin has the problem that it destroys its internal Wayland connection (KWin as client for KWin as server) before shutting down the application. Other external libraries loaded into KWin (e.g. breeze window decoration) are unloaded later on, then try to clean up their Wayland resources and crash KWin due to accessing a no longer valid Wayland connection. With the help of this new API KWin can access all connections during the clean up and destroy them before shutting down the Wayland server and thus exit cleanly. REPOSITORY R127 KWayland BRANCH all-connection-threads REVISION DETAIL https://phabricator.kde.org/D6569 AFFECTED FILES autotests/client/test_wayland_connection_thread.cpp src/client/connection_thread.cpp src/client/connection_thread.h To: graesslin, #frameworks, #plasma, #kwin Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas
