gc: Fix stale pointers in WeakValues

WeakValue::set shold normally not mark objects, given that a weak value
is not supposed to keep an object alive.
However, if we are past GCState::HandleQObjectWrappers, nothing will
reset weak values referencing unmarked values, even if sweep collects
the referenced value.
That leads to stale pointers, and then most likely to crashes.
To avoid this, we mark the objects under this special condition.

The test is written in a way that would also allow for resetting the new
weak values instead, but the current implementation treats memory usage
for throughput and doesn't revisit weak values to reset them.

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

2 files changed, 53 insertions, 18 deletions

diff --git a/src/qml/jsruntime/qv4persistent.cpp b/src/qml/jsruntime/qv4persistent.cpp
index eea3621842..4f11d0a2ad 100644
--- a/src/qml/jsruntime/qv4persistent.cpp
+++ b/src/qml/jsruntime/qv4persistent.cpp
@@ -383,6 +383,56 @@ WeakValue::~WeakValue()
     free();
 }
 
+/*
+   WeakValue::set shold normally not mark objects, after all a weak value
+   is not supposed to keep an object alive.
+   However, if we are past GCState::HandleQObjectWrappers, nothing will
+   reset weak values referencing unmarked values, but those values will
+   still be swept.
+   That lead to stale pointers, and potentially to crashes. To avoid this,
+   we mark the objects here (they might still get collected in the next gc
+   run).
+   This is especially important due to the way we handle QObjectWrappers.
+ */
+void WeakValue::set(ExecutionEngine *engine, const Value &value)
+{
+    if (!val)
+        allocVal(engine);
+    QV4::WriteBarrier::markCustom(engine, [&](QV4::MarkStack *ms) {
+        if (engine->memoryManager->gcStateMachine->state <= GCState::HandleQObjectWrappers)
+            return;
+        if (auto *h = value.heapObject())
+            h->mark(ms);
+    });
+    *val = value;
+}
+
+void WeakValue::set(ExecutionEngine *engine, ReturnedValue value)
+{
+    if (!val)
+        allocVal(engine);
+    QV4::WriteBarrier::markCustom(engine, [&](QV4::MarkStack *ms) {
+        if (engine->memoryManager->gcStateMachine->state <= GCState::HandleQObjectWrappers)
+            return;
+        if (auto *h = QV4::Value::fromReturnedValue(value).heapObject())
+            h->mark(ms);
+    });
+
+    *val = value;
+}
+
+void WeakValue::set(ExecutionEngine *engine, Heap::Base *obj)
+{
+    if (!val)
+        allocVal(engine);
+    QV4::WriteBarrier::markCustom(engine, [&](QV4::MarkStack *ms) {
+        if (engine->memoryManager->gcStateMachine->state <= GCState::HandleQObjectWrappers)
+            return;
+        obj->mark(ms);
+    });
+    *val = obj;
+}
+
 void WeakValue::allocVal(ExecutionEngine *engine)
 {
     val = engine->memoryManager->m_weakValues->allocate();
diff --git a/src/qml/jsruntime/qv4persistent_p.h b/src/qml/jsruntime/qv4persistent_p.h
index c24d337b60..d0e29a166e 100644
--- a/src/qml/jsruntime/qv4persistent_p.h
+++ b/src/qml/jsruntime/qv4persistent_p.h
@@ -131,26 +131,11 @@ public:
     WeakValue &operator=(const WeakValue &other);
     ~WeakValue();
 
-    void set(ExecutionEngine *engine, const Value &value)
-    {
-        if (!val)
-            allocVal(engine);
-        *val = value;
-    }
+    void set(ExecutionEngine *engine, const Value &value);
 
-    void set(ExecutionEngine *engine, ReturnedValue value)
-    {
-        if (!val)
-            allocVal(engine);
-        *val = value;
-    }
+    void set(ExecutionEngine *engine, ReturnedValue value);
 
-    void set(ExecutionEngine *engine, Heap::Base *obj)
-    {
-        if (!val)
-            allocVal(engine);
-        *val = obj;
-    }
+    void set(ExecutionEngine *engine, Heap::Base *obj);
 
     ReturnedValue value() const {
         return (val ? val->asReturnedValue() : Encode::undefined());