Fix crash with Component.onDestruction

A call to a handler of Component.onDestruction may end up causing WeakValues
such as QQmlData::jsWrapper to be set again, even though they've been set to
undefined in an earlier iteration of the loop that walks through the weak
references. That in turn may result in invalid object references to objects
that are scheduled for destruction by the collector.

So after calling all destroy handlers for QObjects, reset all of the weak
values again.

Task-number: QTBUG-54939
Change-Id: I00ebabb76274e296fb1bd90d8d3e21dbbb920b57
Reviewed-by: Lars Knoll <lars.knoll@qt.io>

src/qml/memory/qv4mm.cpp

@@ -428,7 +428,7 @@ void MemoryManager::sweep(bool lastSweep)
         Managed *m = (*it).as<Managed>();
         if (m->markBit())
             continue;
-        // we need to call detroyObject on qobjectwrappers now, so that they can emit the destroyed
+        // we need to call destroyObject on qobjectwrappers now, so that they can emit the destroyed
         // signal before we start sweeping the heap
         if (QObjectWrapper *qobjectWrapper = (*it).as<QObjectWrapper>())
             qobjectWrapper->destroyObject(lastSweep);
@@ -436,6 +436,17 @@ void MemoryManager::sweep(bool lastSweep)
         (*it) = Primitive::undefinedValue();
     }
 
+    // onDestruction handlers may have accessed other QObject wrappers and reset their value, so ensure
+    // that they are all set to undefined.
+    for (PersistentValueStorage::Iterator it = m_weakValues->begin(); it != m_weakValues->end(); ++it) {
+        if (!(*it).isManaged())
+            continue;
+        Managed *m = (*it).as<Managed>();
+        if (m->markBit())
+            continue;
+        (*it) = Primitive::undefinedValue();
+    }
+
     // Now it is time to free QV4::QObjectWrapper Value, we must check the Value's tag to make sure its object has been destroyed
     const int pendingCount = m_pendingFreedObjectWrapperValue.count();
     if (pendingCount) {

tests/auto/qml/qqmlecmascript/data/DestructionHelper.qml

@@ -0,0 +1,3 @@
+import Test 1.0
+WeakReferenceMutator {
+}

tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp

@@ -49,6 +49,8 @@
 #include <private/qv4scopedvalue_p.h>
 #include <private/qv4alloca_p.h>
 #include <private/qv4runtime_p.h>
+#include <private/qv4object_p.h>
+#include <private/qqmlcomponentattached_p.h>
 
 #ifdef Q_CC_MSVC
 #define NO_INLINE __declspec(noinline)
@@ -287,6 +289,7 @@ private slots:
     void replaceBinding();
     void deleteRootObjectInCreation();
     void onDestruction();
+    void onDestructionViaGC();
     void bindingSuppression();
     void signalEmitted();
     void threadSignal();
@@ -7157,6 +7160,105 @@ void tst_qqmlecmascript::onDestruction()
     }
 }
 
+class WeakReferenceMutator : public QObject
+{
+    Q_OBJECT
+public:
+    WeakReferenceMutator()
+        : resultPtr(Q_NULLPTR)
+        , weakRef(Q_NULLPTR)
+    {}
+
+    void init(QV4::ExecutionEngine *v4, QV4::WeakValue *weakRef, bool *resultPtr)
+    {
+        QV4::QObjectWrapper::wrap(v4, this);
+        QQmlEngine::setObjectOwnership(this, QQmlEngine::JavaScriptOwnership);
+
+        this->resultPtr = resultPtr;
+        this->weakRef = weakRef;
+
+        QObject::connect(QQmlComponent::qmlAttachedProperties(this), &QQmlComponentAttached::destruction, this, &WeakReferenceMutator::reviveFirstWeakReference);
+    }
+
+private slots:
+    void reviveFirstWeakReference() {
+        *resultPtr = weakRef->valueRef() && weakRef->isNullOrUndefined();
+        if (!*resultPtr)
+            return;
+        QV4::ExecutionEngine *v4 = QV8Engine::getV4(qmlEngine(this));
+        weakRef->set(v4, v4->newObject());
+        *resultPtr = weakRef->valueRef() && !weakRef->isNullOrUndefined();
+    }
+
+public:
+    bool *resultPtr;
+
+    QV4::WeakValue *weakRef;
+};
+
+QT_BEGIN_NAMESPACE
+
+namespace QV4 {
+
+namespace Heap {
+struct WeakReferenceSentinel : public Object {
+    WeakReferenceSentinel(WeakValue *weakRef, bool *resultPtr)
+        : weakRef(weakRef)
+        , resultPtr(resultPtr) {
+
+    }
+
+    ~WeakReferenceSentinel() {
+        *resultPtr = weakRef->isNullOrUndefined();
+    }
+
+    WeakValue *weakRef;
+    bool *resultPtr;
+};
+} // namespace Heap
+
+struct WeakReferenceSentinel : public Object {
+    V4_OBJECT2(WeakReferenceSentinel, Object)
+    V4_NEEDS_DESTROY
+};
+
+} // namespace QV4
+
+QT_END_NAMESPACE
+
+DEFINE_OBJECT_VTABLE(QV4::WeakReferenceSentinel);
+
+void tst_qqmlecmascript::onDestructionViaGC()
+{
+    qmlRegisterType<WeakReferenceMutator>("Test", 1, 0, "WeakReferenceMutator");
+
+    QQmlEngine engine;
+    QV4::ExecutionEngine *v4 = QV8Engine::getV4(&engine);
+
+    QQmlComponent component(&engine, testFileUrl("DestructionHelper.qml"));
+
+    QScopedPointer<QV4::WeakValue> weakRef;
+
+    bool mutatorResult = false;
+    bool sentinelResult = false;
+
+    {
+        weakRef.reset(new QV4::WeakValue);
+        weakRef->set(v4, v4->newObject());
+        QVERIFY(!weakRef->isNullOrUndefined());
+
+        QPointer<WeakReferenceMutator> weakReferenceMutator = qobject_cast<WeakReferenceMutator *>(component.create());
+        QVERIFY2(!weakReferenceMutator.isNull(), qPrintable(component.errorString()));
+        weakReferenceMutator->init(v4, weakRef.data(), &mutatorResult);
+
+        v4->memoryManager->allocObject<QV4::WeakReferenceSentinel>(weakRef.data(), &sentinelResult);
+    }
+    gc(engine);
+
+    QVERIFY2(mutatorResult, "We failed to re-assign the weak reference a new value during GC");
+    QVERIFY2(sentinelResult, "The weak reference was not cleared properly");
+}
+
 struct EventProcessor : public QObject
 {
     Q_OBJECT