Fix crash when garbage collecting in a non-GUI thread

If a GUI class happens to be detected unreferenced when garbage
collecting in a non-GUI thread and is subsequently deleted, crashes
can occur for QWidgets and similar classes.

The hitherto unimplemented delete-in-main-thread" attribute should be
used.

Add the missing implementation. Add the field to shiboken's type entry
and SbkObjectTypePrivate class and pass it via newly introduced flags
to introduceWrapperType().

Defer the deletion when invoked from the background thread and store
the list of destructors in a list in binding manager run by
Py_AddPendingCall().

Task-number: PYSIDE-743
Task-number: PYSIDE-810
Change-Id: Id4668a6a1e32392be9dcf1229e1e10c492b2a5f5
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Cristian Maureira-Fredes <cristian.maureira-fredes@qt.io>
Reviewed-by: Alexandru Croitor <alexandru.croitor@qt.io>

13 files changed, 124 insertions, 10 deletions

diff --git a/sources/shiboken2/ApiExtractor/abstractmetalang.cpp b/sources/shiboken2/ApiExtractor/abstractmetalang.cpp
index 35e12f780..c65d7e0bd 100644
--- a/sources/shiboken2/ApiExtractor/abstractmetalang.cpp
+++ b/sources/shiboken2/ApiExtractor/abstractmetalang.cpp
@@ -1613,6 +1613,13 @@ void AbstractMetaClass::setTemplateBaseClassInstantiations(AbstractMetaTypeList&
     metaClassBaseTemplateInstantiations()->insert(this, instantiations);
 }
 
+// Does any of the base classes require deletion in the main thread?
+bool AbstractMetaClass::deleteInMainThread() const
+{
+    return typeEntry()->deleteInMainThread()
+        || (m_baseClass && m_baseClass->deleteInMainThread());
+}
+
 static bool functions_contains(const AbstractMetaFunctionList &l, const AbstractMetaFunction *func)
 {
     for (const AbstractMetaFunction *f : l) {
diff --git a/sources/shiboken2/ApiExtractor/abstractmetalang.h b/sources/shiboken2/ApiExtractor/abstractmetalang.h
index 42129e9b7..aaefa32d5 100644
--- a/sources/shiboken2/ApiExtractor/abstractmetalang.h
+++ b/sources/shiboken2/ApiExtractor/abstractmetalang.h
@@ -1699,6 +1699,8 @@ public:
         return m_hasToStringCapability;
     }
 
+    bool deleteInMainThread() const;
+
     static AbstractMetaClass *findClass(const AbstractMetaClassList &classes,
                                         const QString &name);
     static AbstractMetaClass *findClass(const AbstractMetaClassList &classes,
diff --git a/sources/shiboken2/ApiExtractor/typedatabase.cpp b/sources/shiboken2/ApiExtractor/typedatabase.cpp
index 69cddca4c..dcfd9f740 100644
--- a/sources/shiboken2/ApiExtractor/typedatabase.cpp
+++ b/sources/shiboken2/ApiExtractor/typedatabase.cpp
@@ -782,6 +782,7 @@ void ComplexTypeEntry::formatDebug(QDebug &d) const
     FORMAT_BOOL("QObject", m_qobject)
     FORMAT_BOOL("polymorphicBase", m_polymorphicBase)
     FORMAT_BOOL("genericClass", m_genericClass)
+    FORMAT_BOOL("deleteInMainThread", m_deleteInMainThread)
     if (m_typeFlags != 0)
         d << ", typeFlags=" << m_typeFlags;
     d << ", copyableFlag=" << m_copyableFlag
diff --git a/sources/shiboken2/ApiExtractor/typesystem.cpp b/sources/shiboken2/ApiExtractor/typesystem.cpp
index ba219cf5f..21c35bda6 100644
--- a/sources/shiboken2/ApiExtractor/typesystem.cpp
+++ b/sources/shiboken2/ApiExtractor/typesystem.cpp
@@ -1301,8 +1301,8 @@ void Handler::applyComplexTypeAttributes(const QXmlStreamReader &reader,
             if (convertBoolean(attributes->takeAt(i).value(), deprecatedAttribute(), false))
                 ctype->setTypeFlags(ctype->typeFlags() | ComplexTypeEntry::Deprecated);
         } else if (name == deleteInMainThreadAttribute()) {
-            qCWarning(lcShiboken, "%s",
-                      qPrintable(msgUnimplementedAttributeWarning(reader, name)));
+            if (convertBoolean(attributes->takeAt(i).value(), deleteInMainThreadAttribute(), false))
+                ctype->setDeleteInMainThread(true);
         } else if (name == QLatin1String("target-type")) {
             ctype->setTargetType(attributes->takeAt(i).value().toString());
         }
@@ -3221,7 +3221,8 @@ ComplexTypeEntry::ComplexTypeEntry(const QString &name, TypeEntry::Type t,
     m_qualifiedCppName(name),
     m_qobject(false),
     m_polymorphicBase(false),
-    m_genericClass(false)
+    m_genericClass(false),
+    m_deleteInMainThread(false)
 {
 }
 
diff --git a/sources/shiboken2/ApiExtractor/typesystem.h b/sources/shiboken2/ApiExtractor/typesystem.h
index 028f016f3..721d19f29 100644
--- a/sources/shiboken2/ApiExtractor/typesystem.h
+++ b/sources/shiboken2/ApiExtractor/typesystem.h
@@ -1346,6 +1346,9 @@ public:
         m_genericClass = isGeneric;
     }
 
+    bool deleteInMainThread() const { return m_deleteInMainThread; }
+    void setDeleteInMainThread(bool d) { m_deleteInMainThread = d; }
+
     CopyableFlag copyable() const
     {
         return m_copyableFlag;
@@ -1403,6 +1406,7 @@ private:
     uint m_qobject : 1;
     uint m_polymorphicBase : 1;
     uint m_genericClass : 1;
+    uint m_deleteInMainThread : 1;
 
     QString m_polymorphicIdValue;
     QString m_lookupName;
diff --git a/sources/shiboken2/generator/shiboken2/cppgenerator.cpp b/sources/shiboken2/generator/shiboken2/cppgenerator.cpp
index 14bc99f7b..f0d6c9082 100644
--- a/sources/shiboken2/generator/shiboken2/cppgenerator.cpp
+++ b/sources/shiboken2/generator/shiboken2/cppgenerator.cpp
@@ -4947,8 +4947,16 @@ void CppGenerator::writeClassRegister(QTextStream &s,
         else
             s << INDENT << "0," << endl;
 
-        // 9:isInnerClass
-        s << INDENT << (hasEnclosingClass ? "true" : "false") << endl;
+        // 9:wrapperflags
+        QByteArrayList wrapperFlags;
+        if (hasEnclosingClass)
+            wrapperFlags.append(QByteArrayLiteral("Shiboken::ObjectType::WrapperFlags::InnerClass"));
+        if (metaClass->deleteInMainThread())
+            wrapperFlags.append(QByteArrayLiteral("Shiboken::ObjectType::WrapperFlags::DeleteInMainThread"));
+        if (wrapperFlags.isEmpty())
+            s << INDENT << '0';
+        else
+            s << INDENT << wrapperFlags.join(" | ");
     }
     s << INDENT << ");" << endl;
     s << INDENT << endl;
diff --git a/sources/shiboken2/libshiboken/basewrapper.cpp b/sources/shiboken2/libshiboken/basewrapper.cpp
index 2b27f2881..2f5f27989 100644
--- a/sources/shiboken2/libshiboken/basewrapper.cpp
+++ b/sources/shiboken2/libshiboken/basewrapper.cpp
@@ -40,6 +40,7 @@
 #include "basewrapper.h"
 #include "basewrapper_p.h"
 #include "bindingmanager.h"
+#include "helper.h"
 #include "sbkconverter.h"
 #include "sbkenum.h"
 #include "sbkstring.h"
@@ -190,6 +191,12 @@ SbkObjectType *SbkObject_TypeF(void)
     return reinterpret_cast<SbkObjectType *>(type);
 }
 
+static int mainThreadDeletionHandler(void *)
+{
+    if (Py_IsInitialized())
+        Shiboken::BindingManager::instance().runDeletionInMainThread();
+    return 0;
+}
 
 static void SbkDeallocWrapperCommon(PyObject* pyObj, bool canDelete)
 {
@@ -218,8 +225,27 @@ static void SbkDeallocWrapperCommon(PyObject* pyObj, bool canDelete)
         PyObject_ClearWeakRefs(pyObj);
 
     // If I have ownership and is valid delete C++ pointer
-    if (canDelete && sbkObj->d->hasOwnership && sbkObj->d->validCppObject) {
-        SbkObjectTypePrivate *sotp = PepType_SOTP(pyType);
+    SbkObjectTypePrivate *sotp{nullptr};
+    canDelete &= sbkObj->d->hasOwnership && sbkObj->d->validCppObject;
+    if (canDelete) {
+        sotp = PepType_SOTP(pyType);
+        if (sotp->delete_in_main_thread && Shiboken::currentThreadId() != Shiboken::mainThreadId()) {
+            auto &bindingManager = Shiboken::BindingManager::instance();
+            if (sotp->is_multicpp) {
+                 Shiboken::DtorAccumulatorVisitor visitor(sbkObj);
+                 Shiboken::walkThroughClassHierarchy(Py_TYPE(pyObj), &visitor);
+                 for (const auto &e : visitor.entries())
+                     bindingManager.addToDeletionInMainThread(e);
+            } else {
+                Shiboken::DestructorEntry e{sotp->cpp_dtor, sbkObj->d->cptr[0]};
+                bindingManager.addToDeletionInMainThread(e);
+            }
+            Py_AddPendingCall(mainThreadDeletionHandler, nullptr);
+            canDelete = false;
+        }
+    }
+
+    if (canDelete) {
         if (sotp->is_multicpp) {
             Shiboken::DtorAccumulatorVisitor visitor(sbkObj);
             Shiboken::walkThroughClassHierarchy(Py_TYPE(pyObj), &visitor);
@@ -533,6 +559,8 @@ bool DtorAccumulatorVisitor::visit(SbkObjectType *node)
     return false;
 }
 
+void _initMainThreadId(); // helper.cpp
+
 namespace Conversions { void init(); }
 
 void init()
@@ -541,6 +569,8 @@ void init()
     if (shibokenAlreadInitialised)
         return;
 
+    _initMainThreadId();
+
     Conversions::init();
 
     PyEval_InitThreads();
@@ -735,7 +765,7 @@ introduceWrapperType(PyObject *enclosingObject,
                      ObjectDestructor cppObjDtor,
                      SbkObjectType *baseType,
                      PyObject *baseTypes,
-                     bool isInnerClass)
+                     unsigned wrapperFlags)
 {
     if (baseType) {
         typeSpec->slots[0].pfunc = reinterpret_cast<void *>(baseType);
@@ -760,10 +790,14 @@ introduceWrapperType(PyObject *enclosingObject,
         return nullptr;
 
     initPrivateData(type);
+    auto sotp = PepType_SOTP(type);
+    if (wrapperFlags & DeleteInMainThread)
+        sotp->delete_in_main_thread = 1;
+
     setOriginalName(type, originalName);
     setDestructorFunction(type, cppObjDtor);
 
-    if (isInnerClass) {
+    if (wrapperFlags & InnerClass) {
         if (PyDict_SetItemString(enclosingObject, typeName, reinterpret_cast<PyObject *>(type)) == 0)
             return type;
         else
diff --git a/sources/shiboken2/libshiboken/basewrapper.h b/sources/shiboken2/libshiboken/basewrapper.h
index 06b17a151..f8940b842 100644
--- a/sources/shiboken2/libshiboken/basewrapper.h
+++ b/sources/shiboken2/libshiboken/basewrapper.h
@@ -192,6 +192,12 @@ LIBSHIBOKEN_API void        setDestructorFunction(SbkObjectType* self, ObjectDes
 
 LIBSHIBOKEN_API void        initPrivateData(SbkObjectType* self);
 
+enum WrapperFlags
+{
+    InnerClass = 0x1,
+    DeleteInMainThread = 0x2
+};
+
 /**
  *  Initializes a Shiboken wrapper type and adds it to the module,
  *  or to the enclosing class if the type is an inner class.
@@ -217,7 +223,7 @@ LIBSHIBOKEN_API SbkObjectType *introduceWrapperType(PyObject *enclosingObject,
                                                     ObjectDestructor cppObjDtor,
                                                     SbkObjectType *baseType,
                                                     PyObject *baseTypes,
-                                                    bool isInnerClass);
+                                                    unsigned wrapperFlags = 0);
 
 /**
  *  Set the subtype init hook for a type.
diff --git a/sources/shiboken2/libshiboken/basewrapper_p.h b/sources/shiboken2/libshiboken/basewrapper_p.h
index d99ca19ea..f8a381078 100644
--- a/sources/shiboken2/libshiboken/basewrapper_p.h
+++ b/sources/shiboken2/libshiboken/basewrapper_p.h
@@ -137,6 +137,7 @@ struct SbkObjectTypePrivate
     /// Tells is the type is a value type or an object-type, see BEHAVIOUR_* constants.
     // TODO-CONVERTERS: to be deprecated/removed
     int type_behaviour : 2;
+    int delete_in_main_thread : 1;
     /// C++ name
     char* original_name;
     /// Type user data
diff --git a/sources/shiboken2/libshiboken/bindingmanager.cpp b/sources/shiboken2/libshiboken/bindingmanager.cpp
index 82c5bd65f..8a9e912fd 100644
--- a/sources/shiboken2/libshiboken/bindingmanager.cpp
+++ b/sources/shiboken2/libshiboken/bindingmanager.cpp
@@ -136,8 +136,11 @@ static void showWrapperMap(const WrapperMap& wrapperMap)
 #endif
 
 struct BindingManager::BindingManagerPrivate {
+    using DestructorEntries = std::vector<DestructorEntry>;
+
     WrapperMap wrapperMapper;
     Graph classHierarchy;
+    DestructorEntries deleteInMainThread;
     bool destroying;
 
     BindingManagerPrivate() : destroying(false) {}
@@ -249,6 +252,18 @@ void BindingManager::releaseWrapper(SbkObject* sbkObj)
     sbkObj->d->validCppObject = false;
 }
 
+void BindingManager::runDeletionInMainThread()
+{
+    for (const DestructorEntry &e : m_d->deleteInMainThread)
+        e.destructor(e.cppInstance);
+    m_d->deleteInMainThread.clear();
+}
+
+void BindingManager::addToDeletionInMainThread(const DestructorEntry &e)
+{
+    m_d->deleteInMainThread.push_back(e);
+}
+
 SbkObject* BindingManager::retrieveWrapper(const void* cptr)
 {
     WrapperMap::iterator iter = m_d->wrapperMapper.find(cptr);
diff --git a/sources/shiboken2/libshiboken/bindingmanager.h b/sources/shiboken2/libshiboken/bindingmanager.h
index c09b985dc..d03aa999a 100644
--- a/sources/shiboken2/libshiboken/bindingmanager.h
+++ b/sources/shiboken2/libshiboken/bindingmanager.h
@@ -50,6 +50,8 @@ struct SbkObjectType;
 namespace Shiboken
 {
 
+struct DestructorEntry;
+
 typedef void (*ObjectVisitor)(SbkObject*, void*);
 
 class LIBSHIBOKEN_API BindingManager
@@ -67,6 +69,9 @@ public:
     void registerWrapper(SbkObject* pyObj, void* cptr);
     void releaseWrapper(SbkObject* wrapper);
 
+    void runDeletionInMainThread();
+    void addToDeletionInMainThread(const DestructorEntry &);
+
     SbkObject* retrieveWrapper(const void* cptr);
     PyObject* getOverride(const void* cptr, const char* methodName);
 
diff --git a/sources/shiboken2/libshiboken/helper.cpp b/sources/shiboken2/libshiboken/helper.cpp
index 472924723..e42daff07 100644
--- a/sources/shiboken2/libshiboken/helper.cpp
+++ b/sources/shiboken2/libshiboken/helper.cpp
@@ -41,6 +41,12 @@
 #include "sbkstring.h"
 #include <stdarg.h>
 
+#ifdef _WIN32
+#  include <windows.h>
+#else
+#  include <pthread.h>
+#endif
+
 namespace Shiboken
 {
 
@@ -141,4 +147,24 @@ int warning(PyObject* category, int stacklevel, const char* format, ...)
     return result;
 }
 
+ThreadId currentThreadId()
+{
+#if defined(_WIN32)
+    return GetCurrentThreadId();
+#elif defined(__APPLE_CC__)
+    return reinterpret_cast<ThreadId>(pthread_self());
+#else
+    return pthread_self();
+#endif
+}
+
+// Internal, used by init() from main thread
+static ThreadId _mainThreadId{0};
+void _initMainThreadId() { _mainThreadId =  currentThreadId(); }
+
+ThreadId mainThreadId()
+{
+    return _mainThreadId;
+}
+
 } // namespace Shiboken
diff --git a/sources/shiboken2/libshiboken/helper.h b/sources/shiboken2/libshiboken/helper.h
index 79a30871a..9b6d8903f 100644
--- a/sources/shiboken2/libshiboken/helper.h
+++ b/sources/shiboken2/libshiboken/helper.h
@@ -90,6 +90,10 @@ class AutoArrayPointer
         T* data;
 };
 
+typedef unsigned long long ThreadId;
+LIBSHIBOKEN_API ThreadId currentThreadId();
+LIBSHIBOKEN_API ThreadId mainThreadId();
+
 /**
  * An utility function used to call PyErr_WarnEx with a formatted message.
  */