[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-commits
Subject: [kwin] /: [libkwinxrenderutils] Clean up static blend picture before going down
From: Martin_Gräßlin <mgraesslin () kde ! org>
Date: 2016-06-13 13:32:36
Message-ID: E1bCRym-0005Wb-FJ () scm ! kde ! org
[Download RAW message or body]
Git commit d49fba5d30cbc4a7d8530def7656f1bd6e71484a by Martin Gräßlin.
Committed on 13/06/2016 at 13:29.
Pushed by graesslin into branch 'master'.
[libkwinxrenderutils] Clean up static blend picture before going down
Summary:
The method xRenderBlendPicture created a static XRenderPicture on
first usage. To cleanup a XRenderPicture an xcb_connection_t* is needed.
As it's static the cleanup happens on exit handler and at that time Qt
already destroyed the xcb_connection_t*. With a certain chance this will
crash.
To expose the problem a Q_ASSERT(qApp) is added in the destructor of
XRenderPicture. Using xrenderBlendPicture() will hit this assert on
application exit. This is demonstrated by the added auto test.
The actual fix to the problem is moving the static variable out of
the method and introduce a global cleanup method just like the init
method. This is now called from Workspace dtor, so before application
goes down.
CCBUG: 363251
Reviewers: #plasma
Subscribers: plasma-devel
Tags: #plasma
Differential Revision: https://phabricator.kde.org/D1731
M +1 -0 autotests/CMakeLists.txt
A +11 -0 autotests/libxrenderutils/CMakeLists.txt
A +59 -0 autotests/libxrenderutils/blendpicture_test.cpp [License: GPL \
(v2)] M +16 -6 libkwineffects/kwinxrenderutils.cpp
M +5 -0 libkwineffects/kwinxrenderutils.h
M +3 -0 workspace.cpp
http://commits.kde.org/kwin/d49fba5d30cbc4a7d8530def7656f1bd6e71484a
diff --git a/autotests/CMakeLists.txt b/autotests/CMakeLists.txt
index cb78712..9cdf796 100644
--- a/autotests/CMakeLists.txt
+++ b/autotests/CMakeLists.txt
@@ -1,5 +1,6 @@
add_definitions(-DKWIN_UNIT_TEST)
remove_definitions(-DQT_USE_QSTRINGBUILDER)
+add_subdirectory(libxrenderutils)
add_subdirectory(wayland)
if (HAVE_INPUT)
add_subdirectory(libinput)
diff --git a/autotests/libxrenderutils/CMakeLists.txt \
b/autotests/libxrenderutils/CMakeLists.txt new file mode 100644
index 0000000..b73291c
--- /dev/null
+++ b/autotests/libxrenderutils/CMakeLists.txt
@@ -0,0 +1,11 @@
+add_executable(blendPictureTest blendpicture_test.cpp)
+target_link_libraries(blendPictureTest
+ kwinxrenderutils
+ Qt5::Test
+ Qt5::Gui
+ Qt5::X11Extras
+ XCB::XCB
+ XCB::RENDER
+ XCB::XFIXES
+ )
+ecm_mark_as_test(blendPictureTest)
diff --git a/autotests/libxrenderutils/blendpicture_test.cpp \
b/autotests/libxrenderutils/blendpicture_test.cpp new file mode 100644
index 0000000..d00ad3d
--- /dev/null
+++ b/autotests/libxrenderutils/blendpicture_test.cpp
@@ -0,0 +1,59 @@
+/********************************************************************
+KWin - the KDE window manager
+This file is part of the KDE project.
+
+Copyright (C) 2016 Martin Gräßlin <mgraesslin@kde.org>
+
+This program is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 2 of the License, or
+(at your option) any later version.
+
+This program is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with this program. If not, see <http://www.gnu.org/licenses/>.
+*********************************************************************/
+#include <QtTest/QtTest>
+#include <QLoggingCategory>
+#include <QX11Info>
+
+#include "../../libkwineffects/kwinxrenderutils.h"
+
+class BlendPictureTest : public QObject
+{
+ Q_OBJECT
+private Q_SLOTS:
+ void initTestCase();
+ void cleanupTestCase();
+
+ void testDontCrashOnTeardown();
+};
+
+void BlendPictureTest::initTestCase()
+{
+ KWin::XRenderUtils::init(QX11Info::connection(), QX11Info::appRootWindow());
+}
+
+void BlendPictureTest::cleanupTestCase()
+{
+ KWin::XRenderUtils::cleanup();
+}
+
+void BlendPictureTest::testDontCrashOnTeardown()
+{
+ // this test uses xrenderBlendPicture - the only idea is to trigger the creation
+ // closing the application should not crash
+ // see BUG 363251
+ const auto picture = KWin::xRenderBlendPicture(0.5);
+ // and a second one
+ const auto picture2 = KWin::xRenderBlendPicture(0.6);
+ Q_UNUSED(picture)
+ Q_UNUSED(picture2)
+}
+
+QTEST_MAIN(BlendPictureTest)
+#include "blendpicture_test.moc"
diff --git a/libkwineffects/kwinxrenderutils.cpp \
b/libkwineffects/kwinxrenderutils.cpp index da8c249..1c2108b 100644
--- a/libkwineffects/kwinxrenderutils.cpp
+++ b/libkwineffects/kwinxrenderutils.cpp
@@ -21,6 +21,7 @@ along with this program. If not, see \
<http://www.gnu.org/licenses/>. #include "kwinxrenderutils.h"
#include "logging_p.h"
+#include <QCoreApplication>
#include <QStack>
#include <QPixmap>
#include <QGlobalStatic>
@@ -32,6 +33,7 @@ namespace XRenderUtils
{
static xcb_connection_t *s_connection = nullptr;
static xcb_window_t s_rootWindow = XCB_WINDOW_NONE;
+static XRenderPicture s_blendPicture(XCB_RENDER_PICTURE_NONE);
void init(xcb_connection_t *connection, xcb_window_t rootWindow)
{
@@ -39,6 +41,13 @@ void init(xcb_connection_t *connection, xcb_window_t rootWindow)
s_rootWindow = rootWindow;
}
+void cleanup()
+{
+ s_blendPicture = XRenderPicture(XCB_RENDER_PICTURE_NONE);
+ s_connection = nullptr;
+ s_rootWindow = XCB_WINDOW_NONE;
+}
+
} // namespace
// adapted from Qt, because this really sucks ;)
@@ -78,16 +87,15 @@ XRenderPicture xRenderFill(const QColor &c)
XRenderPicture xRenderBlendPicture(double opacity)
{
- static XRenderPicture s_blendPicture(XCB_RENDER_PICTURE_NONE);
static xcb_render_color_t s_blendColor = {0, 0, 0, 0};
s_blendColor.alpha = uint16_t(opacity * 0xffff);
- if (s_blendPicture == XCB_RENDER_PICTURE_NONE) {
- s_blendPicture = xRenderFill(s_blendColor);
+ if (XRenderUtils::s_blendPicture == XCB_RENDER_PICTURE_NONE) {
+ XRenderUtils::s_blendPicture = xRenderFill(s_blendColor);
} else {
xcb_rectangle_t rect = {0, 0, 1, 1};
- xcb_render_fill_rectangles(XRenderUtils::s_connection, \
XCB_RENDER_PICT_OP_SRC, s_blendPicture, s_blendColor, 1, &rect); + \
xcb_render_fill_rectangles(XRenderUtils::s_connection, XCB_RENDER_PICT_OP_SRC, \
XRenderUtils::s_blendPicture, s_blendColor, 1, &rect); }
- return s_blendPicture;
+ return XRenderUtils::s_blendPicture;
}
static xcb_render_picture_t createPicture(xcb_pixmap_t pix, int depth)
@@ -150,8 +158,10 @@ XRenderPicture::XRenderPicture(xcb_pixmap_t pix, int depth)
XRenderPictureData::~XRenderPictureData()
{
- if (picture != XCB_RENDER_PICTURE_NONE)
+ if (picture != XCB_RENDER_PICTURE_NONE) {
+ Q_ASSERT(qApp);
xcb_render_free_picture(XRenderUtils::s_connection, picture);
+ }
}
XFixesRegion::XFixesRegion(const QRegion ®ion)
diff --git a/libkwineffects/kwinxrenderutils.h b/libkwineffects/kwinxrenderutils.h
index f24f90d..2413fa1 100644
--- a/libkwineffects/kwinxrenderutils.h
+++ b/libkwineffects/kwinxrenderutils.h
@@ -180,6 +180,11 @@ KWINXRENDERUTILS_EXPORT xcb_render_pictformat_t \
findPictFormat(xcb_visualid_t vi
*/
KWINXRENDERUTILS_EXPORT const xcb_render_directformat_t \
*findPictFormatInfo(xcb_render_pictformat_t format);
+/**
+ * @internal
+ **/
+KWINXRENDERUTILS_EXPORT void cleanup();
+
} // namespace XRenderUtils
} // namespace KWin
diff --git a/workspace.cpp b/workspace.cpp
index a179889..85fc170 100644
--- a/workspace.cpp
+++ b/workspace.cpp
@@ -465,6 +465,9 @@ Workspace::~Workspace()
// TODO: ungrabXServer();
+ if (kwinApp()->operationMode() == Application::OperationModeX11) {
+ XRenderUtils::cleanup();
+ }
Xcb::Extensions::destroy();
_self = 0;
}
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic