Fix sweep step for tainted QObject JavaScript wrappers

Currently, whenever the garbage collector runs, it will destroy all
valid tainted wrappers.

Only null or undefined wrappers will be preserved in the
m_multiplyWrappedQObjects map.

It seems like "!" was overlooked in
3b5d37ce38. Prior to that change, it
was "!it.value()->markBit()", so calling erase() in the then branch
did make sense. But with "!it.value().isNullOrUndefined()", erase()
will be called for every valid wrapper, which is the opposite what we
want.

Pick-to: 5.15 6.2
Change-Id: I2bf2630f538af8cbd4bfffcff29d67be6c278265
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>

src/qml/memory/qv4mm.cpp

@@ -995,7 +995,7 @@ void MemoryManager::sweep(bool lastSweep, ClassDestroyStatsCallback classCountPt
 
     if (MultiplyWrappedQObjectMap *multiplyWrappedQObjects = engine->m_multiplyWrappedQObjects) {
         for (MultiplyWrappedQObjectMap::Iterator it = multiplyWrappedQObjects->begin(); it != multiplyWrappedQObjects->end();) {
-            if (!it.value().isNullOrUndefined())
+            if (it.value().isNullOrUndefined())
                 it = multiplyWrappedQObjects->erase(it);
             else
                 ++it;

tests/auto/qml/qjsengine/tst_qjsengine.cpp

@@ -104,6 +104,7 @@ private slots:
     void valueConversion_RegularExpression();
     void castWithMultipleInheritance();
     void collectGarbage();
+    void collectGarbageNestedWrappersTwoEngines();
     void gcWithNestedDataStructure();
     void stacktrace();
     void numberParsing_data();
@@ -1837,6 +1838,44 @@ void tst_QJSEngine::collectGarbage()
     QVERIFY(ptr.isNull());
 }
 
+class TestObjectContainer : public QObject
+{
+    Q_OBJECT
+    Q_PROPERTY(QObject *dummy MEMBER m_dummy CONSTANT)
+
+public:
+    TestObjectContainer() : m_dummy(new QObject(this)) {}
+
+private:
+    QObject *m_dummy;
+};
+
+void tst_QJSEngine::collectGarbageNestedWrappersTwoEngines()
+{
+    QJSEngine engine1;
+    QJSEngine engine2;
+
+    TestObjectContainer container;
+    QQmlEngine::setObjectOwnership(&container, QQmlEngine::CppOwnership);
+
+    engine1.globalObject().setProperty("foobar", engine1.newQObject(&container));
+    engine2.globalObject().setProperty("foobar", engine2.newQObject(&container));
+
+    engine1.evaluate("foobar.dummy.baz = 42");
+    engine2.evaluate("foobar.dummy.baz = 43");
+
+    QCOMPARE(engine1.evaluate("foobar.dummy.baz").toInt(), 42);
+    QCOMPARE(engine2.evaluate("foobar.dummy.baz").toInt(), 43);
+
+    engine1.collectGarbage();
+    engine2.collectGarbage();
+
+    // The GC should not collect dummy object wrappers neither in engine1 nor engine2, we
+    // verify that by checking whether the baz property still has its previous value.
+    QCOMPARE(engine1.evaluate("foobar.dummy.baz").toInt(), 42);
+    QCOMPARE(engine2.evaluate("foobar.dummy.baz").toInt(), 43);
+}
+
 void tst_QJSEngine::gcWithNestedDataStructure()
 {
     // The GC must be able to traverse deeply nested objects, otherwise this

tests/auto/qml/qv4mm/tst_qv4mm.cpp

@@ -85,10 +85,10 @@ void tst_qv4mm::multiWrappedQObjects()
         QCOMPARE(engine1.memoryManager->m_pendingFreedObjectWrapperValue.size(), 1);
         QCOMPARE(engine2.memoryManager->m_pendingFreedObjectWrapperValue.size(), 0);
 
-        // Moves the additional WeakValue from m_multiplyWrappedQObjects to
-        // m_pendingFreedObjectWrapperValue. It's still alive after all.
+        // The additional WeakValue from m_multiplyWrappedQObjects hasn't been moved
+        // to m_pendingFreedObjectWrapperValue yet. It's still alive after all.
         engine1.memoryManager->runGC();
-        QCOMPARE(engine1.memoryManager->m_pendingFreedObjectWrapperValue.size(), 2);
+        QCOMPARE(engine1.memoryManager->m_pendingFreedObjectWrapperValue.size(), 1);
 
         // engine2 doesn't own the object as engine1 was the first to wrap it above.
         // Therefore, no effect here.