Make iteration over persistent values safer

This makes it safe to destruct persistents while
we are iterating over them.

Change-Id: I8797d0c553d3201859cdf03fb25df28836e55691
Reviewed-by: Simon Hausmann <simon.hausmann@theqtcompany.com>

2 files changed, 61 insertions, 10 deletions

diff --git a/src/qml/jsruntime/qv4persistent.cpp b/src/qml/jsruntime/qv4persistent.cpp
index 4ec7103644..4a0f84b685 100644
--- a/src/qml/jsruntime/qv4persistent.cpp
+++ b/src/qml/jsruntime/qv4persistent.cpp
@@ -93,6 +93,43 @@ Page *allocatePage(PersistentValueStorage *storage)
 }
 
 
+PersistentValueStorage::Iterator::Iterator(void *p, int idx)
+    : p(p), index(idx)
+{
+    Page *page = static_cast<Page *>(p);
+    if (page)
+        ++page->header.refCount;
+}
+
+PersistentValueStorage::Iterator::Iterator(const PersistentValueStorage::Iterator &o)
+    : p(o.p), index(o.index)
+{
+    Page *page = static_cast<Page *>(p);
+    if (page)
+        ++page->header.refCount;
+}
+
+PersistentValueStorage::Iterator &PersistentValueStorage::Iterator::operator=(const PersistentValueStorage::Iterator &o)
+{
+    Page *page = static_cast<Page *>(p);
+    if (page && !--page->header.refCount)
+        freePage(p);
+    p = o.p;
+    index = o.index;
+    page = static_cast<Page *>(p);
+    if (page)
+        ++page->header.refCount;
+
+    return *this;
+}
+
+PersistentValueStorage::Iterator::~Iterator()
+{
+    Page *page = static_cast<Page *>(p);
+    if (page && !--page->header.refCount)
+        freePage(page);
+}
+
 PersistentValueStorage::Iterator &PersistentValueStorage::Iterator::operator++() {
     while (p) {
         while (index < kEntriesPerPage - 1) {
@@ -101,7 +138,12 @@ PersistentValueStorage::Iterator &PersistentValueStorage::Iterator::operator++()
                 return *this;
         }
         index = -1;
-        p = static_cast<Page *>(p)->header.next;
+        Page *next = static_cast<Page *>(p)->header.next;
+        if (!--static_cast<Page *>(p)->header.refCount)
+            freePage(p);
+        p = next;
+        if (next)
+            ++next->header.refCount;
     }
     index = 0;
     return *this;
@@ -165,13 +207,8 @@ void PersistentValueStorage::free(Value *v)
     v->setTag(QV4::Value::Empty_Type);
     v->setInt_32(p->header.freeList);
     p->header.freeList = v - p->values;
-    if (!--p->header.refCount) {
-        if (p->header.prev)
-            *p->header.prev = p->header.next;
-        if (p->header.next)
-            p->header.next->header.prev = p->header.prev;
-        p->header.alloc.deallocate();
-    }
+    if (!--p->header.refCount)
+        freePage(p);
 }
 
 static void drainMarkStack(QV4::ExecutionEngine *engine, Value *markBase)
@@ -204,6 +241,16 @@ ExecutionEngine *PersistentValueStorage::getEngine(Value *v)
     return getPage(v)->header.engine;
 }
 
+void PersistentValueStorage::freePage(void *page)
+{
+    Page *p = static_cast<Page *>(page);
+    if (p->header.prev)
+        *p->header.prev = p->header.next;
+    if (p->header.next)
+        p->header.next->header.prev = p->header.prev;
+    p->header.alloc.deallocate();
+}
+
 
 PersistentValue::PersistentValue(const PersistentValue &other)
     : val(0)
diff --git a/src/qml/jsruntime/qv4persistent_p.h b/src/qml/jsruntime/qv4persistent_p.h
index 858734e9ed..67a76742d1 100644
--- a/src/qml/jsruntime/qv4persistent_p.h
+++ b/src/qml/jsruntime/qv4persistent_p.h
@@ -51,8 +51,10 @@ struct Q_QML_EXPORT PersistentValueStorage
     void mark(ExecutionEngine *e);
 
     struct Iterator {
-        Q_DECL_CONSTEXPR Iterator(void *p, int idx)
-            : p(p), index(idx) {}
+        Iterator(void *p, int idx);
+        Iterator(const Iterator &o);
+        Iterator & operator=(const Iterator &o);
+        ~Iterator();
         void *p;
         int index;
         Iterator &operator++();
@@ -68,6 +70,8 @@ struct Q_QML_EXPORT PersistentValueStorage
 
     ExecutionEngine *engine;
     void *firstPage;
+private:
+    static void freePage(void *page);
 };
 
 class Q_QML_EXPORT PersistentValue