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>

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) {

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;

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"