Fix possible stack overflow with many property bindings

When there are a lot of bindings to the same property (like 20 000),
we would get stack overflows because the notify list for the
changed signal was traversed recursively.

Changing this also speeds up the traversal. I see something like
~40% reduction in the case of layout() for a notify list of around
200 items.

Note: To make it possible to traverse the double-linked list backwards,
the next-pointer needs to be moved to the beginning of the struct,
because the implementation pattern assumes this
(node->next->prev = &node->next).

I think this code has rotted after it was added, since the prev pointer
was never actually used anywhere before.

Change-Id: Icdfac50b7c8584a908efa65694c7f5f416cb153b
Reviewed-by: Lars Knoll <lars.knoll@theqtcompany.com>

diff --git a/src/qml/qml/qqmlengine.cpp b/src/qml/qml/qqmlengine.cpp
index 1f27e4ecb35e45da3bc83bb81f660c3db96536e7..392cbe9371da57d5aca544860f2d589e3da9574e 100644 (file)
--- a/src/qml/qml/qqmlengine.cpp
+++ b/src/qml/qml/qqmlengine.cpp
@@ -1540,16 +1540,28 @@ QQmlDataExtended::~QQmlDataExtended()
 
 void QQmlData::NotifyList::layout(QQmlNotifierEndpoint *endpoint)
 {
-    if (endpoint->next)
-        layout(endpoint->next);
+    // Add a temporary sentinel at beginning of list. This will be overwritten
+    // when the end point is inserted into the notifies further down.
+    endpoint->prev = 0;
 
-    int index = endpoint->sourceSignal;
-    index = qMin(index, 0xFFFF - 1);
+    while (endpoint->next) {
+        Q_ASSERT(reinterpret_cast<QQmlNotifierEndpoint *>(endpoint->next->prev) == endpoint);
+        endpoint = endpoint->next;
+    }
+
+    while (endpoint) {
+        QQmlNotifierEndpoint *ep = (QQmlNotifierEndpoint *) endpoint->prev;
+
+        int index = endpoint->sourceSignal;
+        index = qMin(index, 0xFFFF - 1);
 
-    endpoint->next = notifies[index];
-    if (endpoint->next) endpoint->next->prev = &endpoint->next;
-    endpoint->prev = &notifies[index];
-    notifies[index] = endpoint;
+        endpoint->next = notifies[index];
+        if (endpoint->next) endpoint->next->prev = &endpoint->next;
+        endpoint->prev = &notifies[index];
+        notifies[index] = endpoint;
+
+        endpoint = ep;
+    }
 }
 
 void QQmlData::NotifyList::layout()

diff --git a/src/qml/qml/qqmlnotifier.cpp b/src/qml/qml/qqmlnotifier.cpp
index 4ce5be4d1a114c258af86662f9073e7f286d0798..ea3f7a15301bcb57c05b4798079d8be318bf8941 100644 (file)
--- a/src/qml/qml/qqmlnotifier.cpp
+++ b/src/qml/qml/qqmlnotifier.cpp
@@ -51,30 +51,52 @@ static Callback QQmlNotifier_callbacks[] = {
     QQmlVMEMetaObjectEndpoint_callback
 };
 
+namespace {
+    struct NotifyListTraversalData {
+        NotifyListTraversalData(QQmlNotifierEndpoint *ep = 0)
+            : originalSenderPtr(0)
+            , disconnectWatch(0)
+            , endpoint(ep)
+        {}
+
+        qintptr originalSenderPtr;
+        qintptr *disconnectWatch;
+        QQmlNotifierEndpoint *endpoint;
+    };
+}
+
 void QQmlNotifier::emitNotify(QQmlNotifierEndpoint *endpoint, void **a)
 {
-    qintptr originalSenderPtr;
-    qintptr *disconnectWatch;
-
-    if (!endpoint->isNotifying()) {
-        originalSenderPtr = endpoint->senderPtr;
-        disconnectWatch = &originalSenderPtr;
-        endpoint->senderPtr = qintptr(disconnectWatch) | 0x1;
-    } else {
-        disconnectWatch = (qintptr *)(endpoint->senderPtr & ~0x1);
+    QVarLengthArray<NotifyListTraversalData> stack;
+    while (endpoint) {
+        stack.append(NotifyListTraversalData(endpoint));
+        endpoint = endpoint->next;
     }
 
-    if (endpoint->next)
-        emitNotify(endpoint->next, a);
+    int i = 0;
+    for (; i < stack.size(); ++i) {
+        NotifyListTraversalData &data = stack[i];
+
+        if (!data.endpoint->isNotifying()) {
+            data.originalSenderPtr = data.endpoint->senderPtr;
+            data.disconnectWatch = &data.originalSenderPtr;
+            data.endpoint->senderPtr = qintptr(data.disconnectWatch) | 0x1;
+        } else {
+            data.disconnectWatch = (qintptr *)(data.endpoint->senderPtr & ~0x1);
+        }
+    }
 
-    if (*disconnectWatch) {
+    while (--i >= 0) {
+        const NotifyListTraversalData &data = stack.at(i);
+        if (*data.disconnectWatch) {
 
-        Q_ASSERT(QQmlNotifier_callbacks[endpoint->callback]);
-        QQmlNotifier_callbacks[endpoint->callback](endpoint, a);
+            Q_ASSERT(QQmlNotifier_callbacks[data.endpoint->callback]);
+            QQmlNotifier_callbacks[data.endpoint->callback](data.endpoint, a);
 
-        if (disconnectWatch == &originalSenderPtr && originalSenderPtr) {
-            // End of notifying, restore values
-            endpoint->senderPtr = originalSenderPtr;
+            if (data.disconnectWatch == &data.originalSenderPtr && data.originalSenderPtr) {
+                // End of notifying, restore values
+                data.endpoint->senderPtr = data.originalSenderPtr;
+            }
         }
     }
 }

diff --git a/src/qml/qml/qqmlnotifier_p.h b/src/qml/qml/qqmlnotifier_p.h
index 2a35dcda12cadaeae80f9fc5f48774c776ee7129..2742bfc84b9e3d3209ce94e949c47b77656f0dcc 100644 (file)
--- a/src/qml/qml/qqmlnotifier_p.h
+++ b/src/qml/qml/qqmlnotifier_p.h
@@ -60,6 +60,8 @@ private:
 class QQmlEngine;
 class QQmlNotifierEndpoint
 {
+    QQmlNotifierEndpoint  *next;
+    QQmlNotifierEndpoint **prev;
 public:
     inline QQmlNotifierEndpoint();
     inline ~QQmlNotifierEndpoint();
@@ -103,9 +105,6 @@ private:
     // The index is in the range returned by QObjectPrivate::signalIndex().
     // This is different from QMetaMethod::methodIndex().
     signed int sourceSignal:28;
-
-    QQmlNotifierEndpoint  *next;
-    QQmlNotifierEndpoint **prev;
 };
 
 QQmlNotifier::QQmlNotifier()
@@ -137,7 +136,7 @@ void QQmlNotifier::notify()
 }
 
 QQmlNotifierEndpoint::QQmlNotifierEndpoint()
-: senderPtr(0), callback(None), sourceSignal(-1), next(0), prev(0)
+: next(0), prev(0), senderPtr(0), callback(None), sourceSignal(-1)
 {
 }
 

diff --git a/tests/auto/qml/qqmlnotifier/tst_qqmlnotifier.cpp b/tests/auto/qml/qqmlnotifier/tst_qqmlnotifier.cpp
index 81215f7a18ad3b4669a2fb7d548c2290425fd72b..4f1c9ae53e0575d0c1001dea0d8d228f901a02af 100644 (file)
--- a/tests/auto/qml/qqmlnotifier/tst_qqmlnotifier.cpp
+++ b/tests/auto/qml/qqmlnotifier/tst_qqmlnotifier.cpp
@@ -165,6 +165,7 @@ private slots:
     void readProperty();
     void propertyChange();
     void disconnectOnDestroy();
+    void lotsOfBindings();
 
 private:
     void createObjects();
@@ -312,6 +313,39 @@ void tst_qqmlnotifier::disconnectOnDestroy()
     exportedObject->verifyReceiverCount();
 }
 
+class TestObject : public QObject
+{
+    Q_OBJECT
+    Q_PROPERTY(int a READ a NOTIFY aChanged)
+
+public:
+    int a() const { return 0; }
+
+signals:
+    void aChanged();
+};
+
+void tst_qqmlnotifier::lotsOfBindings()
+{
+    TestObject o;
+    QQmlEngine *e = new QQmlEngine;
+
+    e->rootContext()->setContextProperty(QStringLiteral("test"), &o);
+
+    QList<QQmlComponent *> components;
+    for (int i = 0; i < 20000; ++i) {
+        QQmlComponent *component = new QQmlComponent(e);
+        component->setData("import QtQuick 2.0; Item { width: test.a; }", QUrl());
+        component->create(e->rootContext());
+        components.append(component);
+    }
+
+    o.aChanged();
+
+    qDeleteAll(components);
+    delete e;
+}
+
 QTEST_MAIN(tst_qqmlnotifier)
 
 #include "tst_qqmlnotifier.moc"