From 858e5f3ee88b1fa26a347ab453aa1df461a95f88 Mon Sep 17 00:00:00 2001
From: Ivan Solovev <ivan.solovev@qt.io>
Date: Thu, 8 Feb 2024 17:19:26 +0100
Subject: [PATCH] QTimer: do not set active state when setting a negative
 interval

QObject::startTimer() returns 0 in case of failure, for example when
someone tries to register a timer with a negative interval.

However, QTimer internally uses -1 as an invalid timer id.
This could lead to a situation when the timer was not really started,
but QTimer::isActive() returned true.

This patch fixes it in two ways:
- check the return value of QObject::startTimer() and treat 0 as an
  error.
- do not treat 0 as a valid timer id when calculating the active state.

As a drive-by: move the `using namespace std::chrono_literals;`
declaration to the top of tst_qtimer.cpp, so that we do not need to
repeat it in each test case.

Fixes: QTBUG-122087
Change-Id: I0e21152b2173ebb5fb0dada1b99a903a321ca9c4
Reviewed-by: Ahmad Samir <a.samirh78@gmail.com>
(cherry picked from commit 612b67cf13cedb832e082308b620f948377ddf21)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
(cherry picked from commit 40e56294f1a082e9066df01e7ed82a0681983f01)
Reviewed-by: Ivan Solovev <ivan.solovev@qt.io>
(cherry picked from commit 148ec9eb9d955b808026f533e3b56917686985cd)
Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
---
 src/corelib/kernel/qtimer.cpp                 | 20 +++++++---
 src/corelib/kernel/qtimer_p.h                 |  2 +-
 .../auto/corelib/kernel/qtimer/tst_qtimer.cpp | 38 ++++++++++++++++++-
 3 files changed, 53 insertions(+), 7 deletions(-)

diff --git a/src/corelib/kernel/qtimer.cpp b/src/corelib/kernel/qtimer.cpp
index d8187cd0dab..a59607e2bc8 100644
--- a/src/corelib/kernel/qtimer.cpp
+++ b/src/corelib/kernel/qtimer.cpp
@@ -188,8 +188,11 @@ void QTimer::start()
     Q_D(QTimer);
     if (d->id != QTimerPrivate::INV_TIMER) // stop running timer
         stop();
-    d->id = QObject::startTimer(d->inter, d->type);
-    d->isActiveData.notify();
+    const int id = QObject::startTimer(d->inter, d->type);
+    if (id > 0) {
+        d->id = id;
+        d->isActiveData.notify();
+    }
 }
 
 /*!
@@ -705,9 +708,16 @@ void QTimer::setInterval(int msec)
     d->inter.setValueBypassingBindings(msec);
     if (d->id != QTimerPrivate::INV_TIMER) { // create new timer
         QObject::killTimer(d->id);                        // restart timer
-        d->id = QObject::startTimer(msec, d->type);
-        // No need to call markDirty() for d->isActiveData here,
-        // as timer state actually does not change
+        const int id = QObject::startTimer(msec, d->type);
+        if (id > 0) {
+            // Restarted successfully. No need to update the active state.
+            d->id = id;
+        } else {
+            // Failed to start the timer.
+            // Need to notify about active state change.
+            d->id = QTimerPrivate::INV_TIMER;
+            d->isActiveData.notify();
+        }
     }
     if (intervalChanged)
         d->inter.notify();
diff --git a/src/corelib/kernel/qtimer_p.h b/src/corelib/kernel/qtimer_p.h
index e296f83441f..de22f432a2f 100644
--- a/src/corelib/kernel/qtimer_p.h
+++ b/src/corelib/kernel/qtimer_p.h
@@ -25,7 +25,7 @@ public:
     static constexpr int INV_TIMER = -1; // invalid timer id
 
     void setInterval(int msec) { q_func()->setInterval(msec); }
-    bool isActiveActualCalculation() const { return id >= 0; }
+    bool isActiveActualCalculation() const { return id > 0; }
 
     int id = INV_TIMER;
     Q_OBJECT_COMPAT_PROPERTY_WITH_ARGS(QTimerPrivate, int, inter, &QTimerPrivate::setInterval, 0)
diff --git a/tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp b/tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp
index f71037f6ed5..3dcb4f664d8 100644
--- a/tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp
+++ b/tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp
@@ -22,6 +22,8 @@
 #include <unistd.h>
 #endif
 
+using namespace std::chrono_literals;
+
 class tst_QTimer : public QObject
 {
     Q_OBJECT
@@ -77,6 +79,8 @@ private slots:
     void bindToTimer();
     void bindTimer();
     void automatedBindingTests();
+
+    void negativeInterval();
 };
 
 void tst_QTimer::zeroTimer()
@@ -149,7 +153,6 @@ void tst_QTimer::singleShotNormalizes_data()
 
 void tst_QTimer::singleShotNormalizes()
 {
-    using namespace std::chrono_literals;
     static constexpr int TestTimeout = 250 * 1000;
     QFETCH(QByteArray, slotName);
     QEventLoop loop;
@@ -1259,6 +1262,16 @@ void tst_QTimer::bindToTimer()
 
     timer.stop();
     QVERIFY(!active);
+
+    // also test that using negative interval updates the binding correctly
+    timer.start(100);
+    QVERIFY(active);
+    timer.setInterval(-100);
+    QVERIFY(!active);
+    timer.start(100);
+    QVERIFY(active);
+    timer.start(-100);
+    QVERIFY(!active);
 }
 
 void tst_QTimer::bindTimer()
@@ -1339,6 +1352,29 @@ void tst_QTimer::automatedBindingTests()
     }
 }
 
+void tst_QTimer::negativeInterval()
+{
+    QTimer timer;
+
+    // Starting with a negative interval does not change active state.
+    timer.start(-100ms);
+    QVERIFY(!timer.isActive());
+
+    // Updating the interval to a negative value stops the timer and changes
+    // the active state.
+    timer.start(100ms);
+    QVERIFY(timer.isActive());
+    timer.setInterval(-100);
+    QVERIFY(!timer.isActive());
+
+    // Starting with a negative interval when already started leads to stop
+    // and inactive state.
+    timer.start(100);
+    QVERIFY(timer.isActive());
+    timer.start(-100ms);
+    QVERIFY(!timer.isActive());
+}
+
 class OrderHelper : public QObject
 {
     Q_OBJECT