Let qApp be noApp instead of pretending to be None

qApp should stay almost as it is with only two cosmetic changes:

When qApp's return value has Type(Py_None), the value now reports
"noApp" instead of "None".

Also the feature of "del __builtins__.qApp" is replaced by function
qApp.shutdown() . This makes things easier to explain and avoids
refcounting hacks.

The embedding problem (Falkon browser) was too complicated.
We finally solved it by disabling qApp in embedded mode.

Change-Id: I0d99661137130684823aa3d1978b494d8ab08e59
Fixes: PYSIDE-1158
Fixes: PYSIDE-1178
Fixes: PYSIDE-1135
Reviewed-by: Cristian Maureira-Fredes <cristian.maureira-fredes@qt.io>

7 files changed, 64 insertions, 100 deletions

diff --git a/sources/pyside2/PySide2/QtCore/typesystem_core_common.xml b/sources/pyside2/PySide2/QtCore/typesystem_core_common.xml
index eec33ea68..9b139668c 100644
--- a/sources/pyside2/PySide2/QtCore/typesystem_core_common.xml
+++ b/sources/pyside2/PySide2/QtCore/typesystem_core_common.xml
@@ -2486,6 +2486,13 @@
     <add-function signature="QCoreApplication()">
         <inject-code file="../glue/qtcore.cpp" snippet="qcoreapplication-2"/>
     </add-function>
+    <!-- Addition for qApp.
+         To be fixed: This function deletes a little too much ATM that is missing later
+         when creating a new qApp. -->
+    <add-function signature="shutdown()">
+      <inject-code class="target" position="beginning" file="../glue/qtcore.cpp" snippet="moduleshutdown"/>
+    </add-function>
+
     <!-- blocking functions -->
     <modify-function signature="processEvents(QFlags&lt;QEventLoop::ProcessEventsFlag&gt;,int)" allow-thread="yes"/>
     <modify-function signature="processEvents(QFlags&lt;QEventLoop::ProcessEventsFlag&gt;)" allow-thread="yes"/>
diff --git a/sources/pyside2/tests/QtWidgets/application_test.py b/sources/pyside2/tests/QtWidgets/application_test.py
index 0b8f73cd6..efb41684a 100644
--- a/sources/pyside2/tests/QtWidgets/application_test.py
+++ b/sources/pyside2/tests/QtWidgets/application_test.py
@@ -46,10 +46,11 @@ class QApplicationInstance(unittest.TestCase):
         app1.setObjectName("MyApp")
         self.assertEqual(app1, app2)
         self.assertEqual(app2.objectName(), app1.objectName())
-        if len(all) > 3:
-            # an import triggers qApp initialization
-            __import__("PySide2." + all[-1])
-            self.assertEqual(app1, qApp)
+        # We no longer support qApp when embedding
+        # if len(all) > 3:
+        #     # an import triggers qApp initialization
+        #     __import__("PySide2." + all[-1])
+        #     self.assertEqual(app1, qApp)
         app1.destroyed.connect(self.appDestroyed)
 
 if __name__ == '__main__':
diff --git a/sources/pyside2/tests/QtWidgets/private_mangle_test.py b/sources/pyside2/tests/QtWidgets/private_mangle_test.py
index 31a870691..5ad10c7b3 100644
--- a/sources/pyside2/tests/QtWidgets/private_mangle_test.py
+++ b/sources/pyside2/tests/QtWidgets/private_mangle_test.py
@@ -91,7 +91,7 @@ class TestMangle(unittest.TestCase):
         QApplication()
 
     def tearDown(self):
-        del QtWidgets.qApp
+        qApp.shutdown()
 
     def testPrivateMangle(self):
         harness = Harness()
diff --git a/sources/pyside2/tests/pysidetest/qapp_like_a_macro_test.py b/sources/pyside2/tests/pysidetest/qapp_like_a_macro_test.py
index 1c0d5d55d..43f455ea9 100644
--- a/sources/pyside2/tests/pysidetest/qapp_like_a_macro_test.py
+++ b/sources/pyside2/tests/pysidetest/qapp_like_a_macro_test.py
@@ -57,20 +57,12 @@ class qAppMacroTest(unittest.TestCase):
                    QtWidgets.QApplication)
         for klass in classes:
             print("created", klass([]))
-            del __builtins__.qApp
-            print("deleted qApp")
+            qApp.shutdown()
+            print("deleted qApp", qApp)
         # creating without deletion raises:
         QtCore.QCoreApplication([])
         with self.assertRaises(RuntimeError):
             QtCore.QCoreApplication([])
-        # assigning qApp is obeyed
-        QtCore.qApp = 42
-        del __builtins__.qApp
-        self.assertEqual(QtCore.qApp, 42)
-        self.assertNotEqual(__builtins__, 42)
-        # delete it and it re-appears
-        del QtCore.qApp
-        QtCore.QCoreApplication([])
         self.assertEqual(QtCore.QCoreApplication.instance(), QtCore.qApp)
         # and they are again all the same
         self.assertTrue(qApp is QtCore.qApp is QtGui.qApp is QtWidgets.qApp)
@@ -87,7 +79,7 @@ class qAppMacroTest(unittest.TestCase):
         if app is None:
             app = QtCore.QCoreApplication([])
         self.assertTrue(QtCore.QObject.staticMetaObject is not None)
-        del __builtins__.qApp
+        qApp.shutdown()
 
 
 if __name__ == '__main__':
diff --git a/sources/shiboken2/libshiboken/pep384impl.cpp b/sources/shiboken2/libshiboken/pep384impl.cpp
index 41d56cba1..c7ca98c46 100644
--- a/sources/shiboken2/libshiboken/pep384impl.cpp
+++ b/sources/shiboken2/libshiboken/pep384impl.cpp
@@ -85,24 +85,28 @@ static PyGetSetDef probe_getseters[] = {
     {nullptr}  /* Sentinel */
 };
 
-#define probe_tp_call       make_dummy(1)
-#define probe_tp_str        make_dummy(2)
-#define probe_tp_traverse   make_dummy(3)
-#define probe_tp_clear      make_dummy(4)
-#define probe_tp_iternext   make_dummy(5)
+#define probe_tp_dealloc    make_dummy(1)
+#define probe_tp_repr       make_dummy(2)
+#define probe_tp_call       make_dummy(3)
+#define probe_tp_str        make_dummy(4)
+#define probe_tp_traverse   make_dummy(5)
+#define probe_tp_clear      make_dummy(6)
+#define probe_tp_iternext   make_dummy(7)
 #define probe_tp_methods    probe_methoddef
 #define probe_tp_getset     probe_getseters
-#define probe_tp_descr_get  make_dummy(8)
-#define probe_tp_init       make_dummy(9)
-#define probe_tp_alloc      make_dummy(10)
-#define probe_tp_new        make_dummy(11)
-#define probe_tp_free       make_dummy(12)
-#define probe_tp_is_gc      make_dummy(13)
+#define probe_tp_descr_get  make_dummy(10)
+#define probe_tp_init       make_dummy(11)
+#define probe_tp_alloc      make_dummy(12)
+#define probe_tp_new        make_dummy(13)
+#define probe_tp_free       make_dummy(14)
+#define probe_tp_is_gc      make_dummy(15)
 
 #define probe_tp_name       "type.probe"
 #define probe_tp_basicsize  make_dummy_int(42)
 
 static PyType_Slot typeprobe_slots[] = {
+    {Py_tp_dealloc,     probe_tp_dealloc},
+    {Py_tp_repr,        probe_tp_repr},
     {Py_tp_call,        probe_tp_call},
     {Py_tp_str,         probe_tp_str},
     {Py_tp_traverse,    probe_tp_traverse},
@@ -144,6 +148,8 @@ check_PyTypeObject_valid()
     if (false
         || strcmp(probe_tp_name, check->tp_name) != 0
         || probe_tp_basicsize       != check->tp_basicsize
+        || probe_tp_dealloc         != check->tp_dealloc
+        || probe_tp_repr            != check->tp_repr
         || probe_tp_call            != check->tp_call
         || probe_tp_str             != check->tp_str
         || probe_tp_traverse        != check->tp_traverse
diff --git a/sources/shiboken2/libshiboken/pep384impl.h b/sources/shiboken2/libshiboken/pep384impl.h
index e9f65e446..6d0fa2450 100644
--- a/sources/shiboken2/libshiboken/pep384impl.h
+++ b/sources/shiboken2/libshiboken/pep384impl.h
@@ -88,12 +88,12 @@ typedef struct _typeobject {
     const char *tp_name;
     Py_ssize_t tp_basicsize;
     void *X03; // Py_ssize_t tp_itemsize;
-    void *X04; // destructor tp_dealloc;
+    destructor tp_dealloc;
     void *X05; // Py_ssize_t tp_vectorcall_offset;
     void *X06; // getattrfunc tp_getattr;
     void *X07; // setattrfunc tp_setattr;
     void *X08; // PyAsyncMethods *tp_as_async;
-    void *X09; // reprfunc tp_repr;
+    reprfunc tp_repr;
     void *X10; // PyNumberMethods *tp_as_number;
     void *X11; // PySequenceMethods *tp_as_sequence;
     void *X12; // PyMappingMethods *tp_as_mapping;
diff --git a/sources/shiboken2/libshiboken/qapp_macro.cpp b/sources/shiboken2/libshiboken/qapp_macro.cpp
index 827c240c5..979638d17 100644
--- a/sources/shiboken2/libshiboken/qapp_macro.cpp
+++ b/sources/shiboken2/libshiboken/qapp_macro.cpp
@@ -54,7 +54,6 @@ extern "C"
 // This variable is also able to destroy the app by deleting qApp.
 //
 static const char *mod_names[3] = {"PySide2.QtCore", "PySide2.QtGui", "PySide2.QtWidgets"};
-static const char *app_names[3] = {"QCoreApplication", "QGuiApplication", "QApplication"};
 
 static int
 qApp_module_index(PyObject *module)
@@ -88,8 +87,6 @@ static SbkObject _Py_ChameleonQAppWrapper_Struct = {
 static PyObject *qApp_var = nullptr;
 static PyObject *qApp_content = reinterpret_cast<PyObject *>(&_Py_ChameleonQAppWrapper_Struct);
 static PyObject *qApp_moduledicts[5] = {nullptr, nullptr, nullptr, nullptr, nullptr};
-static int qApp_var_ref = 0;
-static int qApp_content_ref = 0;
 
 static int
 reset_qApp_var(void)
@@ -109,14 +106,6 @@ reset_qApp_var(void)
 
 static bool app_created = false;
 
-/*
- * 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)
 {
@@ -131,36 +120,11 @@ MakeSingletonQAppWrapper(PyTypeObject *type)
     }
     if (reset_qApp_var() < 0)
         return nullptr;
-    // always know the max of the refs
-    if (Py_REFCNT(qApp_var) > qApp_var_ref)
-        qApp_var_ref = Py_REFCNT(qApp_var);
-    if (Py_REFCNT(qApp_content) > qApp_content_ref)
-        qApp_content_ref = Py_REFCNT(qApp_content);
-
-    if (Py_TYPE(qApp_content) != Py_NONE_TYPE) {
-        // Remove the "_" variable which might hold a reference to qApp.
-        Shiboken::AutoDecRef pymain(PyImport_ImportModule("__main__"));
-        if (pymain.object() && PyObject_HasAttrString(pymain.object(), "_"))
-            PyObject_DelAttrString(pymain.object(), "_");
-        Py_REFCNT(qApp_var) = 1; // fuse is armed...
-    }
     if (type == Py_NONE_TYPE) {
         // PYSIDE-1093: Ignore None when no instance has ever been created.
         if (!app_created)
             Py_RETURN_NONE;
-        // 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");
-        // 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) = Py_REFCNT(Py_None);
-        if (__moduleShutdown != nullptr)
-            Py_XDECREF(PyObject_CallFunction(__moduleShutdown, const_cast<char *>("()")));
     } else {
         PyObject_Init(qApp_content, type);
         app_created = true;
@@ -169,6 +133,28 @@ MakeSingletonQAppWrapper(PyTypeObject *type)
     return qApp_content;
 }
 
+// PYSIDE-1158: Be clear that the QApp none has the type of None but is a
+// different thing.
+
+static PyObject *
+none_repr(PyObject *op)
+{
+    if (op == qApp_content)
+        return PyUnicode_FromString("noApp");
+    return PyUnicode_FromString("None");
+}
+
+static void
+none_dealloc(PyObject *ignore)
+{
+    if (ignore == qApp_content)
+        return;
+    /* This should never get called, but we also don't want to SEGV if
+     * we accidentally decref None out of existence.
+     */
+    Py_FatalError("deallocating None");
+}
+
 #if PYTHON_IS_PYTHON2
 
 // Install support in Py_NONE_TYPE for Python 2: 'bool(qApp) == False'.
@@ -201,6 +187,8 @@ setup_qApp_var(PyObject *module)
     static int init_done = 0;
 
     if (!init_done) {
+        Py_NONE_TYPE->tp_repr = &none_repr;
+        Py_NONE_TYPE->tp_dealloc = &none_dealloc;
 #if PYTHON_IS_PYTHON2
         Py_NONE_TYPE->tp_as_number = &none_as_number;
 #endif
@@ -229,7 +217,6 @@ setup_qApp_var(PyObject *module)
 void
 NotifyModuleForQApp(PyObject *module, void *qApp)
 {
-    setup_qApp_var(module);
     /*
      * PYSIDE-571: Check if an QApplication instance exists before the import.
      * This happens in scriptableapplication and application_test.py .
@@ -249,41 +236,12 @@ NotifyModuleForQApp(PyObject *module, void *qApp)
     // That problem exists when a derived instance is created in C++.
     // PYSIDE-1164: Use the highest Q*Application module possible,
     // because in embedded mode the instance() seems to be sticky.
-    static bool oneshot_active = false;
-    if (qApp == nullptr || app_created || oneshot_active)
-        return;
 
-    // qApp exists without an application created.
-    // We assume that we are embedded, and we simply try to import all three modules.
-    oneshot_active = true;
-    int mod_found = 0;
-    const char *mod_name, *app_name;
-    const char *thismod_name = PyModule_GetName(module);
-
-    // First go through all three modules, import and set qApp_moduledicts.
-    for (int idx = 0; idx < 3; idx++) {
-        // only import if it is not already the module
-        PyObject *mod = strcmp(thismod_name, mod_names[idx]) == 0 ? module
-                      : PyImport_ImportModule(mod_names[idx]);
-        if (mod != nullptr) {
-            mod_found = idx + 1;
-            qApp_moduledicts[mod_found] = PyModule_GetDict(mod);
-            mod_name = PyModule_GetName(mod);
-            app_name = app_names[idx];
-            continue;
-        }
-        PyErr_Clear();
-    }
-
-    // Then take the highest module and call instance() on it.
-    if (mod_found) {
-        PyObject *mod_dict = qApp_moduledicts[mod_found];
-        PyObject *app_class = PyDict_GetItemString(mod_dict, app_name);
-        qApp_content = PyObject_CallMethod(app_class, const_cast<char *>("instance"),
-                                                      const_cast<char *>(""));
-        app_created = true;
-        reset_qApp_var();
-    }
+    // PYSIDE-1135 again:
+    // The problem of late initialization is not worth the effort.
+    // We simply don't create the qApp variable when we are embedded.
+    if (qApp == nullptr)
+        setup_qApp_var(module);
 }