Fix asserts and crashes in QWinEventNotifier activation loop

The backwards iteration was done under the assumption that the only
valid modification of the winEventNotifierList in a slot connected to
activated() would be the removal of the notifier itself. This is wrong.

Instead, iterate forwards, like before 85403d0a, and check the index
against the current list size in every iteration. This ensures that we
do not run out of bounds while the list is modified.

Also, retry the activation loop if the list was modified by a slot
connected to activated(). This ensures that all notifiers with signaled
handles are activated.

Task-number: QTBUG-65940
Change-Id: I25f305463b9234f391abc51fe0628d02f49b6931
Reviewed-by: Friedemann Kleint <Friedemann.Kleint@qt.io>
Reviewed-by: Oswald Buddenhagen <oswald.buddenhagen@qt.io>

3 files changed, 75 insertions, 9 deletions

diff --git a/src/corelib/kernel/qeventdispatcher_win.cpp b/src/corelib/kernel/qeventdispatcher_win.cpp
index bbd442d570..330870f219 100644
--- a/src/corelib/kernel/qeventdispatcher_win.cpp
+++ b/src/corelib/kernel/qeventdispatcher_win.cpp
@@ -905,6 +905,7 @@ bool QEventDispatcherWin32::registerEventNotifier(QWinEventNotifier *notifier)
         return true;
 
     d->winEventNotifierList.append(notifier);
+    d->winEventNotifierListModified = true;
 
     if (!d->winEventNotifierActivatedEvent)
         d->winEventNotifierActivatedEvent = CreateEvent(0, TRUE, FALSE, nullptr);
@@ -928,6 +929,7 @@ void QEventDispatcherWin32::unregisterEventNotifier(QWinEventNotifier *notifier)
     if (i == -1)
         return;
     d->winEventNotifierList.takeAt(i);
+    d->winEventNotifierListModified = true;
     QWinEventNotifierPrivate *nd = QWinEventNotifierPrivate::get(notifier);
     if (nd->waitHandle)
         nd->unregisterWaitObject();
@@ -938,16 +940,19 @@ void QEventDispatcherWin32::activateEventNotifiers()
     Q_D(QEventDispatcherWin32);
     ResetEvent(d->winEventNotifierActivatedEvent);
 
-    // Iterate backwards, because the notifier might remove itself on activate().
-    for (int i = d->winEventNotifierList.count(); --i >= 0;) {
-        QWinEventNotifier *notifier = d->winEventNotifierList.at(i);
-        QWinEventNotifierPrivate *nd = QWinEventNotifierPrivate::get(notifier);
-        if (nd->signaledCount.load() != 0) {
-            --nd->signaledCount;
-            nd->unregisterWaitObject();
-            d->activateEventNotifier(notifier);
+    // Activate signaled notifiers. Our winEventNotifierList can be modified in activation slots.
+    do {
+        d->winEventNotifierListModified = false;
+        for (int i = 0; i < d->winEventNotifierList.count(); ++i) {
+            QWinEventNotifier *notifier = d->winEventNotifierList.at(i);
+            QWinEventNotifierPrivate *nd = QWinEventNotifierPrivate::get(notifier);
+            if (nd->signaledCount.load() != 0) {
+                --nd->signaledCount;
+                nd->unregisterWaitObject();
+                d->activateEventNotifier(notifier);
+            }
         }
-    }
+    } while (d->winEventNotifierListModified);
 
     // Re-register the remaining activated notifiers.
     for (int i = 0; i < d->winEventNotifierList.count(); ++i) {
diff --git a/src/corelib/kernel/qeventdispatcher_win_p.h b/src/corelib/kernel/qeventdispatcher_win_p.h
index 683c7f8f36..a7ed8dda8a 100644
--- a/src/corelib/kernel/qeventdispatcher_win_p.h
+++ b/src/corelib/kernel/qeventdispatcher_win_p.h
@@ -195,6 +195,7 @@ public:
 
     HANDLE winEventNotifierActivatedEvent;
     QList<QWinEventNotifier *> winEventNotifierList;
+    bool winEventNotifierListModified = false;
     void activateEventNotifier(QWinEventNotifier * wen);
 
     QList<MSG> queuedUserInputEvents;
diff --git a/tests/auto/corelib/kernel/qwineventnotifier/tst_qwineventnotifier.cpp b/tests/auto/corelib/kernel/qwineventnotifier/tst_qwineventnotifier.cpp
index 64e78ec3c3..76efa008f7 100644
--- a/tests/auto/corelib/kernel/qwineventnotifier/tst_qwineventnotifier.cpp
+++ b/tests/auto/corelib/kernel/qwineventnotifier/tst_qwineventnotifier.cpp
@@ -29,8 +29,11 @@
 #include <QtTest/QtTest>
 #include <qwineventnotifier.h>
 #include <qtimer.h>
+#include <qvarlengtharray.h>
+#include <qvector.h>
 #include <qt_windows.h>
 
+#include <algorithm>
 #include <memory>
 
 class tst_QWinEventNotifier : public QObject
@@ -44,6 +47,8 @@ private slots:
     void simple_data();
     void simple();
     void manyNotifiers();
+    void disableNotifiersInActivatedSlot_data();
+    void disableNotifiersInActivatedSlot();
 
 private:
     HANDLE simpleHEvent;
@@ -119,6 +124,7 @@ public:
 
     HANDLE eventHandle() const { return notifier.handle(); }
     int numberOfTimesActivated() const { return activatedCount; }
+    void setEnabled(bool b) { notifier.setEnabled(b); }
 
 signals:
     void activated();
@@ -180,6 +186,60 @@ void tst_QWinEventNotifier::manyNotifiers()
     }));
 }
 
+using Indices = QVector<int>;
+
+void tst_QWinEventNotifier::disableNotifiersInActivatedSlot_data()
+{
+    QTest::addColumn<int>("count");
+    QTest::addColumn<Indices>("notifiersToSignal");
+    QTest::addColumn<Indices>("notifiersToDisable");
+    QTest::addColumn<bool>("deleteNotifiers");
+    QTest::newRow("disable_signaled") << 3 << Indices{1} << Indices{1} << false;
+    QTest::newRow("disable_signaled2") << 3 << Indices{1, 2} << Indices{1} << false;
+    QTest::newRow("disable_before_signaled") << 3 << Indices{1} << Indices{0, 1} << false;
+    QTest::newRow("disable_after_signaled") << 3 << Indices{1} << Indices{1, 2} << false;
+    QTest::newRow("delete_signaled") << 3 << Indices{1} << Indices{1} << true;
+    QTest::newRow("delete_before_signaled1") << 3 << Indices{1} << Indices{0} << true;
+    QTest::newRow("delete_before_signaled2") << 3 << Indices{1} << Indices{0, 1} << true;
+    QTest::newRow("delete_before_signaled3") << 4 << Indices{3, 1} << Indices{0, 1} << true;
+    QTest::newRow("delete_after_signaled1") << 3 << Indices{1} << Indices{1, 2} << true;
+    QTest::newRow("delete_after_signaled2") << 4 << Indices{1, 3} << Indices{1, 2} << true;
+    QTest::newRow("delete_after_signaled3") << 5 << Indices{1} << Indices{1, 4} << true;
+}
+
+void tst_QWinEventNotifier::disableNotifiersInActivatedSlot()
+{
+    QFETCH(int, count);
+    QFETCH(Indices, notifiersToSignal);
+    QFETCH(Indices, notifiersToDisable);
+    QFETCH(bool, deleteNotifiers);
+
+    QVarLengthArray<std::unique_ptr<EventWithNotifier>, 10> events(count);
+    for (int i = 0; i < count; ++i)
+        events[i].reset(new EventWithNotifier);
+
+    auto isActivatedOrNull = [&events](int i) {
+        return !events.at(i) || events.at(i)->numberOfTimesActivated() > 0;
+    };
+
+    for (auto &e : events) {
+        connect(e.get(), &EventWithNotifier::activated, [&]() {
+            for (int i : notifiersToDisable) {
+                if (deleteNotifiers)
+                    events[i].reset();
+                else
+                    events.at(i)->setEnabled(false);
+            }
+            if (std::all_of(notifiersToSignal.begin(), notifiersToSignal.end(), isActivatedOrNull))
+                QTimer::singleShot(0, &QTestEventLoop::instance(), SLOT(exitLoop()));
+        });
+    }
+    for (int i : notifiersToSignal)
+        SetEvent(events.at(i)->eventHandle());
+    QTestEventLoop::instance().enterLoop(30);
+    QVERIFY(!QTestEventLoop::instance().timeout());
+}
+
 QTEST_MAIN(tst_QWinEventNotifier)
 
 #include "tst_qwineventnotifier.moc"