Fix QObject ownership

Implement the JS vs. C++ ownership policy. QV4::WeakValue is a weak
reference that's now used instead of PersistentValue in QQmlData.
Whether or not to delete the QObject when the JS object is garbage
collected is decided in the ~QObjectWrapper destructor (conveniently).

Fixes four ownership tests.

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

8 files changed, 155 insertions, 20 deletions

diff --git a/src/qml/qml/qqmldata_p.h b/src/qml/qml/qqmldata_p.h
index 096e358a73..98407e53b5 100644
--- a/src/qml/qml/qqmldata_p.h
+++ b/src/qml/qml/qqmldata_p.h
@@ -179,7 +179,7 @@ public:
     unsigned int deferredIdx;
 
     quint32 v8objectid;
-    QV4::PersistentValue v8object;
+    QV4::WeakValue v8object;
 
     QQmlPropertyCache *propertyCache;
 
diff --git a/src/qml/qml/v4/qv4mm.cpp b/src/qml/qml/v4/qv4mm.cpp
index 9c0e4ad1e0..cb24c3c134 100644
--- a/src/qml/qml/v4/qv4mm.cpp
+++ b/src/qml/qml/v4/qv4mm.cpp
@@ -131,6 +131,7 @@ MemoryManager::MemoryManager()
     : m_d(new Data(true))
     , m_contextList(0)
     , m_persistentValues(0)
+    , m_weakValues(0)
 {
     setEnableGC(true);
 #ifdef V4_USE_VALGRIND
@@ -242,18 +243,16 @@ void MemoryManager::mark()
         it.key()->mark();
 
     PersistentValuePrivate *persistent = m_persistentValues;
-    PersistentValuePrivate **last = &m_persistentValues;
     while (persistent) {
         if (!persistent->refcount) {
-            *last = persistent->next;
             PersistentValuePrivate *n = persistent->next;
+            persistent->removeFromList();
             delete persistent;
             persistent = n;
             continue;
         }
         if (Managed *m = persistent->value.asManaged())
             m->mark();
-        last = &persistent->next;
         persistent = persistent->next;
     }
 
@@ -289,6 +288,25 @@ void MemoryManager::mark()
 
 std::size_t MemoryManager::sweep()
 {
+    PersistentValuePrivate *weak = m_weakValues;
+    while (weak) {
+        if (!weak->refcount) {
+            PersistentValuePrivate *n = weak->next;
+            weak->removeFromList();
+            delete weak;
+            weak = n;
+            continue;
+        }
+        if (Managed *m = weak->value.asManaged()) {
+            if (!m->markBit) {
+                weak->value = Value::emptyValue();
+                weak->removeFromList();
+                continue;
+            }
+        }
+        weak = weak->next;
+    }
+
     std::size_t freedCount = 0;
 
     for (QVector<Data::Chunk>::iterator i = m_d->heapChunks.begin(), ei = m_d->heapChunks.end(); i != ei; ++i)
diff --git a/src/qml/qml/v4/qv4mm_p.h b/src/qml/qml/v4/qv4mm_p.h
index fe858d8f7f..cb56c8e158 100644
--- a/src/qml/qml/v4/qv4mm_p.h
+++ b/src/qml/qml/v4/qv4mm_p.h
@@ -137,6 +137,7 @@ protected:
     ExecutionContext *m_contextList;
 public:
     PersistentValuePrivate *m_persistentValues;
+    PersistentValuePrivate *m_weakValues;
 };
 
 inline ExecutionContext *MemoryManager::allocContext(uint size)
diff --git a/src/qml/qml/v4/qv4value.cpp b/src/qml/qml/v4/qv4value.cpp
index 6e69d05744..a86e886c6c 100644
--- a/src/qml/qml/v4/qv4value.cpp
+++ b/src/qml/qml/v4/qv4value.cpp
@@ -284,7 +284,69 @@ PersistentValue::~PersistentValue()
         d->deref();
 }
 
-PersistentValuePrivate::PersistentValuePrivate(const Value &v)
+WeakValue::WeakValue(const Value &val)
+    : d(new PersistentValuePrivate(val, /*weak*/true))
+{
+}
+
+WeakValue::WeakValue(const WeakValue &other)
+    : d(other.d)
+{
+    if (d)
+        d->ref();
+}
+
+WeakValue &WeakValue::operator=(const WeakValue &other)
+{
+    if (d == other.d)
+        return *this;
+
+    // the memory manager cleans up those with a refcount of 0
+
+    if (d)
+        d->deref();
+    d = other.d;
+    if (d)
+        d->ref();
+}
+
+WeakValue &WeakValue::operator =(const Value &other)
+{
+    if (!d) {
+        d = new PersistentValuePrivate(other, /*weak*/true);
+        return *this;
+    }
+    d->value = other;
+    Managed *m = d->value.asManaged();
+    if (!d->prev) {
+        if (m) {
+            ExecutionEngine *engine = m->engine();
+            if (engine) {
+                d->prev = &engine->memoryManager->m_weakValues;
+                d->next = engine->memoryManager->m_weakValues;
+                *d->prev = d;
+                if (d->next)
+                    d->next->prev = &d->next;
+            }
+        }
+    } else if (!m) {
+        if (d->next)
+            d->next->prev = d->prev;
+        *d->prev = d->next;
+        d->prev = 0;
+        d->next = 0;
+    }
+}
+
+
+WeakValue::~WeakValue()
+{
+    if (d)
+        d->deref();
+}
+
+
+PersistentValuePrivate::PersistentValuePrivate(const Value &v, bool weak)
     : value(v)
     , refcount(1)
     , prev(0)
@@ -296,24 +358,33 @@ PersistentValuePrivate::PersistentValuePrivate(const Value &v)
 
     ExecutionEngine *engine = m->engine();
     if (engine) {
-        prev = &engine->memoryManager->m_persistentValues;
-        next = engine->memoryManager->m_persistentValues;
+        PersistentValuePrivate **listRoot = weak ? &engine->memoryManager->m_weakValues : &engine->memoryManager->m_persistentValues;
+
+        prev = listRoot;
+        next = *listRoot;
         *prev = this;
         if (next)
             next->prev = &this->next;
     }
 }
 
+void PersistentValuePrivate::removeFromList()
+{
+    if (prev) {
+        if (next)
+            next->prev = prev;
+        *prev = next;
+        prev = 0;
+    }
+}
+
 void PersistentValuePrivate::deref()
 {
     // if engine is not 0, they are registered with the memory manager
     // and will get cleaned up in the next gc run
     if (!--refcount) {
-        if (prev) {
-            if (next)
-                next->prev = prev;
-            *prev = next;
-        }
+        removeFromList();
         delete this;
     }
 }
+
diff --git a/src/qml/qml/v4/qv4value_p.h b/src/qml/qml/v4/qv4value_p.h
index ea1f1f86d3..673fa7c91d 100644
--- a/src/qml/qml/v4/qv4value_p.h
+++ b/src/qml/qml/v4/qv4value_p.h
@@ -563,12 +563,13 @@ inline Value Managed::call(ExecutionContext *context, const Value &thisObject, V
 
 struct PersistentValuePrivate
 {
-    PersistentValuePrivate(const Value &v);
+    PersistentValuePrivate(const Value &v, bool weak = false);
     Value value;
-    int refcount;
+    uint refcount;
     PersistentValuePrivate **prev;
     PersistentValuePrivate *next;
 
+    void removeFromList();
     void ref() { ++refcount; }
     void deref();
 };
@@ -605,6 +606,38 @@ private:
     PersistentValuePrivate *d;
 };
 
+class Q_QML_EXPORT WeakValue
+{
+public:
+    WeakValue() : d(0) {}
+    WeakValue(const Value &val);
+    WeakValue(const WeakValue &other);
+    WeakValue &operator=(const WeakValue &other);
+    WeakValue &operator=(const Value &other);
+    ~WeakValue();
+
+    Value value() const {
+        return d ? d->value : Value::emptyValue();
+    }
+
+    ExecutionEngine *engine() {
+        if (!d)
+            return 0;
+        Managed *m = d->value.asManaged();
+        return m ? m->engine() : 0;
+    }
+
+    operator Value() const { return value(); }
+
+    bool isEmpty() const { return !d || d->value.isEmpty(); }
+    void clear() {
+        *this = WeakValue();
+    }
+
+private:
+    PersistentValuePrivate *d;
+};
+
 } // namespace QV4
 
 QT_END_NAMESPACE
diff --git a/src/qml/qml/v8/qv8engine.cpp b/src/qml/qml/v8/qv8engine.cpp
index e0a7766886..8244d02600 100644
--- a/src/qml/qml/v8/qv8engine.cpp
+++ b/src/qml/qml/v8/qv8engine.cpp
@@ -722,7 +722,7 @@ void QV8Engine::setExtensionData(int index, Deletable *data)
 }
 
 
-QV4::PersistentValue *QV8Engine::findOwnerAndStrength(QObject *object, bool *shouldBeStrong)
+QV4::WeakValue *QV8Engine::findOwnerAndStrength(QObject *object, bool *shouldBeStrong)
 {
     QQmlData *data = QQmlData::get(object);
     if (data && data->rootObjectInCreation) { // When the object is still being created it may not show up to the GC.
@@ -765,7 +765,7 @@ void QV8Engine::addRelationshipForGC(QObject *object, const QV4::PersistentValue
         return;
 
     bool handleShouldBeStrong = false;
-    QV4::PersistentValue *implicitOwner = findOwnerAndStrength(object, &handleShouldBeStrong);
+    QV4::WeakValue *implicitOwner = findOwnerAndStrength(object, &handleShouldBeStrong);
     if (handleShouldBeStrong) {
         // ### FIXME
 //        v8::V8::AddImplicitReferences(m_strongReferencer, &handle, 1);
@@ -782,8 +782,8 @@ void QV8Engine::addRelationshipForGC(QObject *object, QObject *other)
         return;
 
     bool handleShouldBeStrong = false;
-    QV4::PersistentValue *implicitOwner = findOwnerAndStrength(object, &handleShouldBeStrong);
-    QV4::PersistentValue handle = QQmlData::get(other, true)->v8object;
+    QV4::WeakValue *implicitOwner = findOwnerAndStrength(object, &handleShouldBeStrong);
+    QV4::WeakValue handle = QQmlData::get(other, true)->v8object;
     if (handle.isEmpty()) // no JS data to keep alive.
         return;
     // ### FIXME
diff --git a/src/qml/qml/v8/qv8engine_p.h b/src/qml/qml/v8/qv8engine_p.h
index 254445d0b0..24096b7f68 100644
--- a/src/qml/qml/v8/qv8engine_p.h
+++ b/src/qml/qml/v8/qv8engine_p.h
@@ -437,7 +437,7 @@ private:
     QVariantMap variantMapFromJS(QV4::Object *object, V8ObjectSet &visitedObjects);
     QVariant variantFromJS(const QV4::Value &value, V8ObjectSet &visitedObjects);
 
-    static QV4::PersistentValue *findOwnerAndStrength(QObject *object, bool *shouldBeStrong);
+    static QV4::WeakValue *findOwnerAndStrength(QObject *object, bool *shouldBeStrong);
 
     Q_DISABLE_COPY(QV8Engine)
 };
diff --git a/src/qml/qml/v8/qv8qobjectwrapper.cpp b/src/qml/qml/v8/qv8qobjectwrapper.cpp
index 2303df3116..73d396d262 100644
--- a/src/qml/qml/v8/qv8qobjectwrapper.cpp
+++ b/src/qml/qml/v8/qv8qobjectwrapper.cpp
@@ -90,6 +90,18 @@ QObjectWrapper::QObjectWrapper(ExecutionEngine *engine, QObject *object)
 
 QObjectWrapper::~QObjectWrapper()
 {
+    if (!object)
+        return;
+    QQmlData *ddata = QQmlData::get(object, false);
+    if (!ddata)
+        return;
+    if (!object->parent() && !ddata->indestructible) {
+        // This object is notionally destroyed now
+        if (ddata->ownContext && ddata->context)
+            ddata->context->emitDestruction();
+        ddata->isQueuedForDeletion = true;
+        object->deleteLater();
+    }
 }
 
 QV4::Value QObjectWrapper::method_toString(QV4::SimpleCallContext *ctx)
@@ -270,7 +282,7 @@ public:
         delete this;
     }
 
-    QV4::PersistentValue v8object;
+    QV4::WeakValue v8object;
     QV8QObjectWrapper *wrapper;
 };