SharedInternalClassData: Mark newly inserted values

If a Heap value is inserted into a SharedInternalClassData instance
which was already  marked, we would potentially never mark the entry at
all.

Note that this is only necessary for the PropertyKey specialization, as
PropertyAttributes does not contain any heap values.

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

2 files changed, 64 insertions, 0 deletions

diff --git a/src/qml/jsruntime/qv4internalclass_p.h b/src/qml/jsruntime/qv4internalclass_p.h
index d14579b4ce..68c0fe57a0 100644
--- a/src/qml/jsruntime/qv4internalclass_p.h
+++ b/src/qml/jsruntime/qv4internalclass_p.h
@@ -133,6 +133,8 @@ struct SharedInternalClassDataPrivate<PropertyAttributes> {
 
     void grow();
 
+    void markIfNecessary(const PropertyAttributes &) {}
+
     uint alloc() const { return m_alloc; }
     uint size() const { return m_size; }
     void setSize(uint s) { m_size = s; }
@@ -180,6 +182,9 @@ struct SharedInternalClassDataPrivate<PropertyKey> {
     SharedInternalClassDataPrivate(const SharedInternalClassDataPrivate &other, uint pos, PropertyKey value);
     ~SharedInternalClassDataPrivate() {}
 
+    template<typename StringOrSymbol = Heap::StringOrSymbol>
+    void markIfNecessary(const PropertyKey &value);
+
     void grow();
     uint alloc() const;
     uint size() const;
@@ -196,6 +201,17 @@ private:
     Heap::MemberData *data;
 };
 
+template<typename StringOrSymbol>
+void QV4::SharedInternalClassDataPrivate<PropertyKey>::markIfNecessary(const PropertyKey &value)
+{
+    QV4::WriteBarrier::markCustom(engine, [&](QV4::MarkStack *stack) {
+        if constexpr (QV4::WriteBarrier::isInsertionBarrier) {
+            if (auto s = value.asStringOrSymbol<StringOrSymbol>())
+                s->mark(stack);
+        }
+    });
+}
+
 template <typename T>
 struct SharedInternalClassData {
     using Private = SharedInternalClassDataPrivate<T>;
@@ -223,6 +239,7 @@ struct SharedInternalClassData {
     }
 
     void add(uint pos, T value) {
+        d->markIfNecessary(value);
         if (pos < d->size()) {
             Q_ASSERT(d->refcount > 1);
             // need to detach
@@ -244,6 +261,7 @@ struct SharedInternalClassData {
 
     void set(uint pos, T value) {
         Q_ASSERT(pos < d->size());
+        d->markIfNecessary(value);
         if (d->refcount > 1) {
             // need to detach
             Private *dd = new Private(*d);
diff --git a/tests/auto/qml/qv4mm/tst_qv4mm.cpp b/tests/auto/qml/qv4mm/tst_qv4mm.cpp
index dec49324d5..806eb00f0f 100644
--- a/tests/auto/qml/qv4mm/tst_qv4mm.cpp
+++ b/tests/auto/qml/qv4mm/tst_qv4mm.cpp
@@ -10,6 +10,7 @@
 #include <private/qv4qobjectwrapper_p.h>
 #include <private/qjsvalue_p.h>
 #include <private/qqmlengine_p.h>
+#include <private/qv4identifiertable_p.h>
 
 #include <QtQuickTestUtils/private/qmlutils_p.h>
 
@@ -30,6 +31,7 @@ private slots:
     void accessParentOnDestruction();
     void cleanInternalClasses();
     void createObjectsOnDestruction();
+    void sharedInternalClassDataMarking();
 };
 
 tst_qv4mm::tst_qv4mm()
@@ -307,6 +309,50 @@ void tst_qv4mm::createObjectsOnDestruction()
     QCOMPARE(obj->property("ok").toBool(), true);
 }
 
+void tst_qv4mm::sharedInternalClassDataMarking()
+{
+    QV4::ExecutionEngine engine;
+    QV4::Scope scope(engine.rootContext());
+    QV4::ScopedObject object(scope, engine.newObject());
+    QVERIFY(!engine.memoryManager->gcBlocked);
+    // no scoped classes, as that would defeat the point of the test
+    // we block the gc instead so that the allocation can't trigger the gc
+    engine.memoryManager->gcBlocked = true;
+    QV4::Heap::String *s = engine.newString(QString::fromLatin1("test"));
+    QV4::PropertyKey id = engine.identifierTable->asPropertyKeyImpl(s);
+    engine.memoryManager->gcBlocked = false;
+    QVERIFY(!id.asStringOrSymbol()->isMarked());
+
+    auto sm = engine.memoryManager->gcStateMachine.get();
+    sm->reset();
+    while (sm->state != QV4::GCState::MarkGlobalObject) {
+        QV4::GCStateInfo& stateInfo = sm->stateInfoMap[int(sm->state)];
+        sm->state = stateInfo.execute(sm, sm->stateData);
+    }
+
+    // simulate partial marking caused by drain due mark stack running out of space
+    // and running out of time during drain phase for complete marking
+    // the last part is necessary for us to find not-already marked name/value pair to put into
+    // the object
+
+    QVERIFY(engine.memoryManager->markStack()->isEmpty());
+    QVERIFY(!id.asStringOrSymbol()->isMarked());
+    {
+
+        // for simplcity's sake we create a new PropertyKey - if gc were actually ongoing that would
+        // already mark it. In practice we would need to retrieve an existing one from an unmarked
+        // object, and then make that object unreachable afterwards.
+        object->put(id, QV4::Value::fromUInt32(42));
+        engine.memoryManager->markStack()->drain();
+        QVERIFY(id.asStringOrSymbol()->isMarked());
+    }
+    gc(engine);
+    // sanity check that we still can lookup the value
+    QV4::ScopedString s2(scope, engine.newString(QString::fromLatin1("test")));
+    auto val = QV4::Value::fromReturnedValue(object->get(s2->toPropertyKey()));
+    QCOMPARE(val.toUInt32(), 42u);
+}
+
 QTEST_MAIN(tst_qv4mm)
 
 #include "tst_qv4mm.moc"