diff options
author | Christian Tismer <tismer@stackless.com> | 2018-03-23 19:54:42 +0100 |
---|---|---|
committer | Christian Tismer <tismer@stackless.com> | 2018-04-19 11:20:51 +0000 |
commit | a89690409972501741c846ac8ad4a499f2982809 (patch) | |
tree | 7684abfa34b4c9aa66ab317452da37ccc1f8f31e /sources | |
parent | 4023ab3862eee7ca3084dd83ca76fba11b5db46b (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>
Diffstat (limited to 'sources')
-rw-r--r-- | sources/pyside2/libpyside/pyside.cpp | 2 | ||||
-rw-r--r-- | sources/pyside2/tests/QtWidgets/CMakeLists.txt | 1 | ||||
-rw-r--r-- | sources/pyside2/tests/QtWidgets/qapp_issue_585.py | 68 | ||||
-rw-r--r-- | sources/shiboken2/libshiboken/qapp_macro.cpp | 24 |
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; } |