aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorChristian Tismer <tismer@stackless.com>2018-03-23 19:54:42 +0100
committerChristian Tismer <tismer@stackless.com>2018-04-19 11:20:51 +0000
commita89690409972501741c846ac8ad4a499f2982809 (patch)
tree7684abfa34b4c9aa66ab317452da37ccc1f8f31e
parent4023ab3862eee7ca3084dd83ca76fba11b5db46b (diff)
fix more qApp crashes
When building PySide with a debug Python, a lot more problems become visible. They were triggered by some malicious ordering of the shutdown code, which must come *after* the refcounts of the variables are adjusted. The initial issue PYSIDE-585 was caused because the shutdown code is not only used for every created Q*Application, but also for the module shutdown, which deletes qApp_contents too often. Instead of special-casing that or adding some refcount, it was much more intuitive in that context to set qApp_content's refcount to the same value as Py_None, which also is not supposed to be garbage collected. Btw., the reason for the error message is that Py_None has it, too. When we set qApp_content's type to Py_None's type, it inherits the protection code that prevents someone from garbage collecting Py_None. Task-number: PYSIDE-585 Change-Id: I4af9de1192730f06054a5aca099a32e2392e367d Reviewed-by: Friedemann Kleint <Friedemann.Kleint@qt.io>
-rw-r--r--sources/pyside2/libpyside/pyside.cpp2
-rw-r--r--sources/pyside2/tests/QtWidgets/CMakeLists.txt1
-rw-r--r--sources/pyside2/tests/QtWidgets/qapp_issue_585.py68
-rw-r--r--sources/shiboken2/libshiboken/qapp_macro.cpp24
4 files changed, 86 insertions, 9 deletions
diff --git a/sources/pyside2/libpyside/pyside.cpp b/sources/pyside2/libpyside/pyside.cpp
index b223edc6c..4c7e6471c 100644
--- a/sources/pyside2/libpyside/pyside.cpp
+++ b/sources/pyside2/libpyside/pyside.cpp
@@ -162,10 +162,10 @@ static void destructionVisitor(SbkObject* pyObj, void* data)
void destroyQCoreApplication()
{
- SignalManager::instance().clear();
QCoreApplication* app = QCoreApplication::instance();
if (!app)
return;
+ SignalManager::instance().clear();
Shiboken::BindingManager& bm = Shiboken::BindingManager::instance();
SbkObject* pyQApp = bm.retrieveWrapper(app);
diff --git a/sources/pyside2/tests/QtWidgets/CMakeLists.txt b/sources/pyside2/tests/QtWidgets/CMakeLists.txt
index fa64d1c3b..c22981251 100644
--- a/sources/pyside2/tests/QtWidgets/CMakeLists.txt
+++ b/sources/pyside2/tests/QtWidgets/CMakeLists.txt
@@ -82,6 +82,7 @@ PYSIDE_TEST(parent_method_test.py)
PYSIDE_TEST(python_properties_test.py)
PYSIDE_TEST(qabstracttextdocumentlayout_test.py)
PYSIDE_TEST(qaction_test.py)
+PYSIDE_TEST(qapp_issue_585.py)
PYSIDE_TEST(qapp_test.py)
PYSIDE_TEST(qapplication_exit_segfault_test.py)
PYSIDE_TEST(qapplication_singleton_test.py)
diff --git a/sources/pyside2/tests/QtWidgets/qapp_issue_585.py b/sources/pyside2/tests/QtWidgets/qapp_issue_585.py
new file mode 100644
index 000000000..9dd2014c0
--- /dev/null
+++ b/sources/pyside2/tests/QtWidgets/qapp_issue_585.py
@@ -0,0 +1,68 @@
+#############################################################################
+##
+## Copyright (C) 2018 The Qt Company Ltd.
+## Contact: https://www.qt.io/licensing/
+##
+## This file is part of the test suite of PySide2.
+##
+## $QT_BEGIN_LICENSE:GPL-EXCEPT$
+## Commercial License Usage
+## Licensees holding valid commercial Qt licenses may use this file in
+## accordance with the commercial license agreement provided with the
+## Software or, alternatively, in accordance with the terms contained in
+## a written agreement between you and The Qt Company. For licensing terms
+## and conditions see https://www.qt.io/terms-conditions. For further
+## information use the contact form at https://www.qt.io/contact-us.
+##
+## GNU General Public License Usage
+## Alternatively, this file may be used under the terms of the GNU
+## General Public License version 3 as published by the Free Software
+## Foundation with exceptions as appearing in the file LICENSE.GPL3-EXCEPT
+## included in the packaging of this file. Please review the following
+## information to ensure the GNU General Public License requirements will
+## be met: https://www.gnu.org/licenses/gpl-3.0.html.
+##
+## $QT_END_LICENSE$
+##
+#############################################################################
+
+"""
+The bug was caused by this commit:
+"Support the qApp macro correctly, final version incl. debug"
+e30e0c161b2b4d50484314bf006e9e5e8ff6b380
+2017-10-27
+
+The bug was first solved by this commit:
+"Fix qApp macro refcount"
+b811c874dedd14fd8b072bc73761d39255216073
+2018-03-21
+
+This test triggers the refcounting bug of qApp, issue PYSIDE-585.
+Finally, the real patch included more changes, because another error
+was in the ordering of shutdown calls. It was found using the following
+Python configuration:
+
+ In Python 3.6 create a directory 'debug' and cd into it.
+
+ ../configure --with-pydebug --prefix=$HOME/pydebug/ --enable-shared
+
+Then a lot more refcounting errors show up, which are due to a bug in
+the code position of the shutdown procedure.
+The reason for the initial refcount bug was that the shutdown code is once
+more often called than the creation of the qApp wrapper.
+Finally, it was easiest and more intuitive to simply make the refcount of
+qApp_content equal to that of Py_None, which is also not supposed to be
+garbage-collected.
+
+For some reason, the test does not work as a unittest because it creates
+no crash. We leave it this way.
+"""
+
+from PySide2.QtCore import QTimer
+from PySide2 import QtWidgets
+
+app_instance = QtWidgets.QApplication([])
+# If the following line is commented, application doesn't crash on exit anymore.
+app_instance2 = app_instance
+QTimer.singleShot(0, qApp.quit)
+app_instance.exec_()
diff --git a/sources/shiboken2/libshiboken/qapp_macro.cpp b/sources/shiboken2/libshiboken/qapp_macro.cpp
index 9b940aaaa..e3f23de4f 100644
--- a/sources/shiboken2/libshiboken/qapp_macro.cpp
+++ b/sources/shiboken2/libshiboken/qapp_macro.cpp
@@ -105,7 +105,14 @@ reset_qApp_var()
return 0;
}
-
+/*
+ * Note:
+ * The PYSIDE-585 problem was that shutdown is called one more often
+ * than Q*Application is created. We could special-case that last
+ * shutdown or add a refcount, initially, but actually it was easier
+ * and more intuitive in that context to make the refcount of
+ * qApp_content equal to the refcount of Py_None.
+ */
PyObject *
MakeSingletonQAppWrapper(PyTypeObject *type)
{
@@ -132,15 +139,16 @@ MakeSingletonQAppWrapper(PyTypeObject *type)
// Debug mode showed that we need to do more than just remove the
// reference. To keep everything in the right order, it is easiest
// to do a full shutdown, using QtCore.__moduleShutdown().
+ // restore the "None-state"
PyObject *__moduleShutdown = PyDict_GetItemString(qApp_moduledicts[1],
"__moduleShutdown");
- if (__moduleShutdown != NULL) {
- Py_DECREF(PyObject_CallFunction(__moduleShutdown, (char *)"()"));
- }
- // restore the "None-state"
+ // PYSIDE-585: It was crucial to update the refcounts *before*
+ // calling the shutdown.
Py_TYPE(qApp_content) = Py_NONE_TYPE;
Py_REFCNT(qApp_var) = qApp_var_ref;
- Py_REFCNT(qApp_content) = qApp_content_ref;
+ Py_REFCNT(qApp_content) = Py_REFCNT(Py_None);
+ if (__moduleShutdown != NULL)
+ Py_DECREF(PyObject_CallFunction(__moduleShutdown, (char *)"()"));
}
else
(void)PyObject_INIT(qApp_content, type);
@@ -160,7 +168,7 @@ setup_qApp_var(PyObject *module)
return -1;
// This is a borrowed reference
qApp_moduledicts[0] = PyEval_GetBuiltins();
- Py_INCREF(qApp_content);
+ Py_INCREF(qApp_moduledicts[0]);
init_done = 1;
}
@@ -170,7 +178,7 @@ setup_qApp_var(PyObject *module)
if (module_index) {
// This line gets a borrowed reference
qApp_moduledicts[module_index] = PyModule_GetDict(module);
- Py_INCREF(qApp_content);
+ Py_INCREF(qApp_moduledicts[module_index]);
if (reset_qApp_var() < 0)
return -1;
}