gc: Prevent recursing into gc from onDestroyed

We end up in a rather weird state otherwise.
Also test that we don't mistakenly collect ObjectWrappers created in
onDestroyed.

Change-Id: Iab158e5b34510979c8ac9a51a75247a2cee100f3
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>

3 files changed, 82 insertions, 2 deletions

diff --git a/src/qml/memory/qv4mm.cpp b/src/qml/memory/qv4mm.cpp
index f1910a4a56..8f6a6503fc 100644
--- a/src/qml/memory/qv4mm.cpp
+++ b/src/qml/memory/qv4mm.cpp
@@ -805,9 +805,14 @@ GCState initCallDestroyObjects(GCStateMachine *that, ExtraData &stateData)
     stateData = GCIteratorStorage { that->mm->m_weakValues->begin() };
     return CallDestroyObjects;
 }
-GCState callDestroyObject(GCStateMachine *, ExtraData &stateData)
+GCState callDestroyObject(GCStateMachine *that, ExtraData &stateData)
 {
     PersistentValueStorage::Iterator& it = get<GCIteratorStorage>(stateData).it;
+    // destroyObject might call user code, which really shouldn't call back into the gc
+    auto oldState = std::exchange(that->mm->gcBlocked, QV4::MemoryManager::Blockness::InCriticalSection);
+    auto cleanup = qScopeGuard([&]() {
+        that->mm->gcBlocked = oldState;
+    });
     // avoid repeatedly hitting the timer constantly by batching iterations
     for (int i = 0; i < markLoopIterationCount; ++i) {
         if (!it.p)
@@ -1235,8 +1240,9 @@ bool MemoryManager::tryForceGCCompletion()
     const bool incrementalGCIsAlreadyRunning = m_markStack != nullptr;
     Q_ASSERT(incrementalGCIsAlreadyRunning);
     auto oldTimeLimit = std::exchange(gcStateMachine->timeLimit, std::chrono::microseconds::max());
-    while (gcStateMachine->inProgress())
+    while (gcStateMachine->inProgress()) {
         gcStateMachine->step();
+    }
     gcStateMachine->timeLimit = oldTimeLimit;
     return true;
 }
diff --git a/tests/auto/qml/qv4mm/data/simpleObject.qml b/tests/auto/qml/qv4mm/data/simpleObject.qml
new file mode 100644
index 0000000000..8fc36a40da
--- /dev/null
+++ b/tests/auto/qml/qv4mm/data/simpleObject.qml
@@ -0,0 +1,3 @@
+import QtQml
+
+QtObject {}
diff --git a/tests/auto/qml/qv4mm/tst_qv4mm.cpp b/tests/auto/qml/qv4mm/tst_qv4mm.cpp
index 28926f02f3..3bf68f7f89 100644
--- a/tests/auto/qml/qv4mm/tst_qv4mm.cpp
+++ b/tests/auto/qml/qv4mm/tst_qv4mm.cpp
@@ -12,6 +12,7 @@
 #include <private/qqmlengine_p.h>
 #include <private/qv4identifiertable_p.h>
 #include <private/qv4arraydata_p.h>
+#include <private/qqmlcomponentattached_p.h>
 
 #include <QtQuickTestUtils/private/qmlutils_p.h>
 
@@ -34,6 +35,7 @@ private slots:
     void cleanInternalClasses();
     void createObjectsOnDestruction();
     void sharedInternalClassDataMarking();
+    void gcTriggeredInOnDestroyed();
 };
 
 tst_qv4mm::tst_qv4mm()
@@ -387,6 +389,75 @@ void tst_qv4mm::sharedInternalClassDataMarking()
     QCOMPARE(val.toUInt32(), 42u);
 }
 
+void tst_qv4mm::gcTriggeredInOnDestroyed()
+{
+    QQmlEngine engine;
+    QV4::ExecutionEngine &v4 = *engine.handle();
+
+    QPointer<QObject> testObject = new QObject; // unparented, will be deleted
+    auto cleanup = qScopeGuard([&]() {
+        if (testObject)
+            testObject->deleteLater();
+    });
+
+    QQmlComponent component(&engine, testFileUrl("simpleObject.qml"));
+    auto toBeCollected = component.create();
+    QVERIFY(toBeCollected);
+    QJSEngine::setObjectOwnership(toBeCollected, QJSEngine::JavaScriptOwnership);
+    QV4::QObjectWrapper::ensureWrapper(&v4, toBeCollected);
+    QVERIFY(qmlEngine(toBeCollected));
+    QQmlComponentAttached *attached = QQmlComponent::qmlAttachedProperties(toBeCollected);
+    QVERIFY(attached);
+
+
+    QV4::Scope scope(v4.rootContext());
+    QCOMPARE(v4.memoryManager->gcBlocked, QV4::MemoryManager::Unblocked);
+
+
+
+    // let the gc run up to CallDestroyObjects
+    auto sm = v4.memoryManager->gcStateMachine.get();
+    sm->reset();
+    v4.memoryManager->gcBlocked = QV4::MemoryManager::NormalBlocked;
+    while (sm->state != QV4::GCState::CallDestroyObjects && sm->state != QV4::GCState::Invalid) {
+        QV4::GCStateInfo& stateInfo = sm->stateInfoMap[int(sm->state)];
+        sm->state = stateInfo.execute(sm, sm->stateData);
+    }
+    QCOMPARE(sm->state, QV4::GCState::CallDestroyObjects);
+
+    QV4::ScopedValue val(scope);
+    bool calledOnDestroyed = false;
+    auto con = connect(attached, &QQmlComponentAttached::destruction, this, [&]() {
+        calledOnDestroyed = true;
+        // we trigger uncommon code paths:
+        // create ObjectWrapper in destroyed hadnler
+        auto ddata = QQmlData::get(testObject.get(), false);
+        QVERIFY(!ddata); // we don't have ddata yet (otherwise we'd already have an object wrapper)
+        val = QV4::QObjectWrapper::wrap(&v4, testObject.get());
+        QJSEngine::setObjectOwnership(testObject, QJSEngine::JavaScriptOwnership);
+
+        // and also try to trigger a force gc completion
+        bool gcComplete = v4.memoryManager->tryForceGCCompletion();
+        QVERIFY(!gcComplete);
+    });
+    while (!calledOnDestroyed && sm->state != QV4::GCState::Invalid) {
+        QV4::GCStateInfo& stateInfo = sm->stateInfoMap[int(sm->state)];
+        sm->state = stateInfo.execute(sm, sm->stateData);
+    }
+    QVERIFY(!QTest::currentTestFailed());
+    QObject::disconnect(con);
+    QVERIFY(calledOnDestroyed);
+
+    bool gcComplete = v4.memoryManager->tryForceGCCompletion();
+    QVERIFY(gcComplete);
+    val = QV4::Value::undefinedValue(); // no longer keep a reference on the stack
+    QCOMPARE(sm->state, QV4::GCState::Invalid);
+    QVERIFY(testObject); // must not have be deleted, referenced by val
+
+    gc(v4); // run another gc cycle
+    QVERIFY(!testObject); // now collcted by gc
+}
+
 QTEST_MAIN(tst_qv4mm)
 
 #include "tst_qv4mm.moc"