Rework QObject deletion mechanism

Instead of the qv4mm knowing about different ways of deleting the QObjects
of the QObject JS wrappers (and their timing!), provide an interface between
the two.

Before destroying a managed object, we allow for the object to register a
Deletable instance in a singly linked list of deletable objects that will be
deleted right after the sweep, but before the gc returns to application.

This allows for a behavior that is identical with V8: When GC runs, the
QML Component.onDestruction callbacks are called _after_ the sweep but
before returning to the application. The QObject however is deleted later,
unless this is the very last run of the GC (as indicated by the lastCall
property).

Change-Id: I1a2f127f4634c7ecc8c89b45e8b0a575c0ef772e
Reviewed-by: Lars Knoll <lars.knoll@digia.com>

diff --git a/src/qml/qml/v4/qv4managed.cpp b/src/qml/qml/v4/qv4managed.cpp
index efba827..ca81d50 100644 (file)
--- a/src/qml/qml/v4/qv4managed.cpp
+++ b/src/qml/qml/v4/qv4managed.cpp
@@ -51,6 +51,7 @@ const ManagedVTable Managed::static_vtbl =
     construct,
     0 /*markObjects*/,
     destroy,
+    0 /*collectDeletables*/,
     hasInstance,
     0,
     0,

diff --git a/src/qml/qml/v4/qv4managed_p.h b/src/qml/qml/v4/qv4managed_p.h
index ab7d902..f3acdad 100644 (file)
--- a/src/qml/qml/v4/qv4managed_p.h
+++ b/src/qml/qml/v4/qv4managed_p.h
@@ -89,6 +89,13 @@ inline void qYouForgotTheQ_MANAGED_Macro(T1, T2) {}
         Q_MANAGED_CHECK \
         static const QV4::ManagedVTable static_vtbl;
 
+struct GCDeletable
+{
+    GCDeletable() : next(0), lastCall(false) {}
+    virtual ~GCDeletable() {}
+    GCDeletable *next;
+    bool lastCall;
+};
 
 struct ManagedVTable
 {
@@ -96,6 +103,7 @@ struct ManagedVTable
     Value (*construct)(Managed *, ExecutionContext *context, Value *args, int argc);
     void (*markObjects)(Managed *);
     void (*destroy)(Managed *);
+    void (*collectDeletables)(Managed *, GCDeletable **deletable);
     bool (*hasInstance)(Managed *, ExecutionContext *ctx, const Value &value);
     Value (*get)(Managed *, ExecutionContext *ctx, String *name, bool *hasProperty);
     Value (*getIndexed)(Managed *, ExecutionContext *ctx, uint index, bool *hasProperty);
@@ -119,6 +127,7 @@ const QV4::ManagedVTable classname::static_vtbl =    \
     construct,                                  \
     markObjects,                                \
     destroy,                                    \
+    0,                                          \
     hasInstance,                                \
     get,                                        \
     getIndexed,                                 \
@@ -135,6 +144,29 @@ const QV4::ManagedVTable classname::static_vtbl =    \
     #classname                                  \
 }
 
+#define DEFINE_MANAGED_VTABLE_WITH_DELETABLES(classname) \
+const QV4::ManagedVTable classname::static_vtbl =    \
+{                                               \
+    call,                                       \
+    construct,                                  \
+    markObjects,                                \
+    destroy,                                    \
+    collectDeletables,                          \
+    hasInstance,                                \
+    get,                                        \
+    getIndexed,                                 \
+    put,                                        \
+    putIndexed,                                 \
+    query,                                      \
+    queryIndexed,                               \
+    deleteProperty,                             \
+    deleteIndexedProperty,                      \
+    getLookup,                                  \
+    setLookup,                                  \
+    isEqualTo,                                  \
+    advanceIterator,                            \
+    #classname                                  \
+}
 
 struct Q_QML_EXPORT Managed
 {

diff --git a/src/qml/qml/v4/qv4mm.cpp b/src/qml/qml/v4/qv4mm.cpp
index b5e1544..13c377f 100644 (file)
--- a/src/qml/qml/v4/qv4mm.cpp
+++ b/src/qml/qml/v4/qv4mm.cpp
@@ -318,7 +318,7 @@ void MemoryManager::mark()
     }
 }
 
-std::size_t MemoryManager::sweep()
+std::size_t MemoryManager::sweep(bool lastSweep)
 {
     PersistentValuePrivate *weak = m_weakValues;
     while (weak) {
@@ -351,9 +351,11 @@ std::size_t MemoryManager::sweep()
     }
 
     std::size_t freedCount = 0;
+    GCDeletable *deletable = 0;
+    GCDeletable **firstDeletable = &deletable;
 
     for (QVector<Data::Chunk>::iterator i = m_d->heapChunks.begin(), ei = m_d->heapChunks.end(); i != ei; ++i)
-        freedCount += sweep(reinterpret_cast<char*>(i->memory.base()), i->memory.size(), i->chunkSize);
+        freedCount += sweep(reinterpret_cast<char*>(i->memory.base()), i->memory.size(), i->chunkSize, &deletable);
 
     ExecutionContext *ctx = m_contextList;
     ExecutionContext **n = &m_contextList;
@@ -369,10 +371,18 @@ std::size_t MemoryManager::sweep()
         ctx = next;
     }
 
+    deletable = *firstDeletable;
+    while (deletable) {
+        GCDeletable *next = deletable->next;
+        deletable->lastCall = lastSweep;
+        delete deletable;
+        deletable = next;
+    }
+
     return freedCount;
 }
 
-std::size_t MemoryManager::sweep(char *chunkStart, std::size_t chunkSize, size_t size)
+std::size_t MemoryManager::sweep(char *chunkStart, std::size_t chunkSize, size_t size, GCDeletable **deletable)
 {
 //    qDebug("chunkStart @ %p, size=%x, pos=%x (%x)", chunkStart, size, size>>4, m_d->smallItems[size >> 4]);
     std::size_t freedCount = 0;
@@ -397,6 +407,8 @@ std::size_t MemoryManager::sweep(char *chunkStart, std::size_t chunkSize, size_t
 #ifdef V4_USE_VALGRIND
                 VALGRIND_ENABLE_ERROR_REPORTING;
 #endif
+                if (m->vtbl->collectDeletables)
+                    m->vtbl->collectDeletables(m, deletable);
                 m->vtbl->destroy(m);
 
                 m->setNextFree(*f);
@@ -458,18 +470,6 @@ void MemoryManager::setEnableGC(bool enableGC)
 
 MemoryManager::~MemoryManager()
 {
-    PersistentValuePrivate *weak = m_weakValues;
-    while (weak) {
-        if (QObjectWrapper *qobjectWrapper = weak->value.as<QObjectWrapper>()) {
-            weak->ref();
-            qobjectWrapper->deleteQObject(/*deleteInstantly*/ true);
-            PersistentValuePrivate *n = weak->next;
-            weak->deref();
-            weak = n;
-        } else
-            weak = weak->next;
-    }
-
     PersistentValuePrivate *persistent = m_persistentValues;
     while (persistent) {
         PersistentValuePrivate *n = persistent->next;
@@ -479,7 +479,7 @@ MemoryManager::~MemoryManager()
         persistent = n;
     }
 
-    sweep();
+    sweep(/*lastSweep*/true);
 #ifdef V4_USE_VALGRIND
     VALGRIND_DESTROY_MEMPOOL(this);
 #endif

diff --git a/src/qml/qml/v4/qv4mm_p.h b/src/qml/qml/v4/qv4mm_p.h
index cb56c8e..c673c01 100644 (file)
--- a/src/qml/qml/v4/qv4mm_p.h
+++ b/src/qml/qml/v4/qv4mm_p.h
@@ -56,6 +56,7 @@ namespace QV4 {
 struct ExecutionEngine;
 struct ExecutionContext;
 struct Managed;
+struct GCDeletable;
 
 class Q_QML_EXPORT MemoryManager
 {
@@ -129,8 +130,8 @@ protected:
 private:
     void collectFromStack() const;
     void mark();
-    std::size_t sweep();
-    std::size_t sweep(char *chunkStart, std::size_t chunkSize, size_t size);
+    std::size_t sweep(bool lastSweep = false);
+    std::size_t sweep(char *chunkStart, std::size_t chunkSize, size_t size, GCDeletable **deletable);
 
 protected:
     QScopedPointer<Data> m_d;

diff --git a/src/qml/qml/v4/qv4qobjectwrapper.cpp b/src/qml/qml/v4/qv4qobjectwrapper.cpp
index 6aa9ece..5da472b 100644 (file)
--- a/src/qml/qml/v4/qv4qobjectwrapper.cpp
+++ b/src/qml/qml/v4/qv4qobjectwrapper.cpp
@@ -253,61 +253,12 @@ QObjectWrapper::QObjectWrapper(ExecutionEngine *engine, QObject *object)
     m_toString = engine->newIdentifier(QStringLiteral("toString"));
 }
 
-QObjectWrapper::~QObjectWrapper()
-{
-    deleteQObject();
-}
-
 void QObjectWrapper::initializeBindings(ExecutionEngine *engine)
 {
     engine->functionPrototype->defineDefaultProperty(engine, QStringLiteral("connect"), method_connect);
     engine->functionPrototype->defineDefaultProperty(engine, QStringLiteral("disconnect"), method_disconnect);
 }
 
-namespace {
-    struct SafeQMLObjectDeleter : public QObject
-    {
-        SafeQMLObjectDeleter(QObject *objectToDelete)
-            : m_objectToDelete(objectToDelete)
-        {}
-
-        ~SafeQMLObjectDeleter()
-        {
-            QQmlData *ddata = QQmlData::get(m_objectToDelete, false);
-            if (ddata && ddata->ownContext && ddata->context)
-                ddata->context->emitDestruction();
-            // This object is notionally destroyed now
-            ddata->isQueuedForDeletion = true;
-            delete m_objectToDelete;
-        }
-
-        QObject *m_objectToDelete;
-    };
-}
-
-void QObjectWrapper::deleteQObject(bool deleteInstantly)
-{
-    if (!m_object)
-        return;
-    QQmlData *ddata = QQmlData::get(m_object, false);
-    if (!ddata)
-        return;
-    if (!m_object->parent() && !ddata->indestructible) {
-        if (deleteInstantly) {
-            QQmlData *ddata = QQmlData::get(m_object, false);
-            if (ddata->ownContext && ddata->context)
-                ddata->context->emitDestruction();
-            // This object is notionally destroyed now
-            ddata->isQueuedForDeletion = true;
-            delete m_object;
-        } else {
-            QObject *deleter = new SafeQMLObjectDeleter(m_object);
-            deleter->deleteLater();
-            m_object = 0;
-        }
-    }
-}
-
 QQmlPropertyData *QObjectWrapper::findProperty(ExecutionEngine *engine, QQmlContextData *qmlContext, String *name, RevisionMode revisionMode, QQmlPropertyData *local) const
 {
     QHashedV4String propertystring(QV4::Value::fromString(name));
@@ -916,7 +867,50 @@ void QObjectWrapper::markObjects(Managed *that)
     QV4::Object::markObjects(that);
 }
 
-DEFINE_MANAGED_VTABLE(QObjectWrapper);
+namespace {
+    struct QObjectDeleter : public QV4::GCDeletable
+    {
+        QObjectDeleter(QObject *o)
+            : m_objectToDelete(o)
+        {}
+        ~QObjectDeleter()
+        {
+            QQmlData *ddata = QQmlData::get(m_objectToDelete, false);
+            if (ddata && ddata->ownContext && ddata->context)
+                ddata->context->emitDestruction();
+            // This object is notionally destroyed now
+            ddata->isQueuedForDeletion = true;
+            if (lastCall)
+                delete m_objectToDelete;
+            else
+                m_objectToDelete->deleteLater();
+        }
+
+        QObject *m_objectToDelete;
+    };
+}
+
+void QObjectWrapper::collectDeletables(Managed *m, GCDeletable **deletable)
+{
+    QObjectWrapper *This = static_cast<QObjectWrapper*>(m);
+    QQmlGuard<QObject> &object = This->m_object;
+    if (!object)
+        return;
+
+    QQmlData *ddata = QQmlData::get(object, false);
+    if (!ddata)
+        return;
+
+    if (object->parent() || ddata->indestructible)
+        return;
+
+    QObjectDeleter *deleter = new QObjectDeleter(object);
+    object = 0;
+    deleter->next = *deletable;
+    *deletable = deleter;
+}
+
+DEFINE_MANAGED_VTABLE_WITH_DELETABLES(QObjectWrapper);
 
 // XXX TODO: Need to review all calls to QQmlEngine *engine() to confirm QObjects work
 // correctly in a worker thread

diff --git a/src/qml/qml/v4/qv4qobjectwrapper_p.h b/src/qml/qml/v4/qv4qobjectwrapper_p.h
index 97d2fb4..5aa418a 100644 (file)
--- a/src/qml/qml/v4/qv4qobjectwrapper_p.h
+++ b/src/qml/qml/v4/qv4qobjectwrapper_p.h
@@ -81,14 +81,10 @@ struct Q_QML_EXPORT QObjectWrapper : public QV4::Object
 
     enum RevisionMode { IgnoreRevision, CheckRevision };
 
-    ~QObjectWrapper();
-
     static void initializeBindings(ExecutionEngine *engine);
 
     QObject *object() const { return m_object.data(); }
 
-    void deleteQObject(bool deleteInstantly = false);
-
     Value getQmlProperty(ExecutionContext *ctx, QQmlContextData *qmlContext, String *name, RevisionMode revisionMode, bool *hasProperty = 0, bool includeImports = false);
     static Value getQmlProperty(ExecutionContext *ctx, QQmlContextData *qmlContext, QObject *object, String *name, RevisionMode revisionMode, bool *hasProperty = 0);
 
@@ -114,7 +110,7 @@ private:
     static PropertyAttributes query(const Managed *, String *name);
     static Property *advanceIterator(Managed *m, ObjectIterator *it, String **name, uint *index, PropertyAttributes *attributes);
     static void markObjects(Managed *that);
-
+    static void collectDeletables(Managed *m, GCDeletable **deletable);
     static void destroy(Managed *that)
     {
         static_cast<QObjectWrapper *>(that)->~QObjectWrapper();

diff --git a/src/qml/qml/v4/qv4string.cpp b/src/qml/qml/v4/qv4string.cpp
index 52e9949..3853371 100644 (file)
--- a/src/qml/qml/v4/qv4string.cpp
+++ b/src/qml/qml/v4/qv4string.cpp
@@ -80,6 +80,7 @@ const ManagedVTable String::static_vtbl =
     construct,
     0 /*markObjects*/,
     destroy,
+    0 /*collectDeletables*/,
     hasInstance,
     get,
     getIndexed,