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
Pick-to: 6.7 6.6 6.5
Change-Id: I0e21152b2173ebb5fb0dada1b99a903a321ca9c4
Reviewed-by: Ahmad Samir <a.samirh78@gmail.com>

3 files changed, 53 insertions, 7 deletions

diff --git a/src/corelib/kernel/qtimer.cpp b/src/corelib/kernel/qtimer.cpp
index 90f6dacd42..04e32d9ec7 100644
--- a/src/corelib/kernel/qtimer.cpp
+++ b/src/corelib/kernel/qtimer.cpp
@@ -192,8 +192,11 @@ void QTimer::start()
     Q_D(QTimer);
     if (d->id != QTimerPrivate::INV_TIMER) // stop running timer
         stop();
-    d->id = QObject::startTimer(std::chrono::milliseconds{d->inter}, d->type);
-    d->isActiveData.notify();
+    const int id = QObject::startTimer(std::chrono::milliseconds{d->inter}, d->type);
+    if (id > 0) {
+        d->id = id;
+        d->isActiveData.notify();
+    }
 }
 
 /*!
@@ -554,9 +557,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(std::chrono::milliseconds{msec}, d->type);
-        // No need to call markDirty() for d->isActiveData here,
-        // as timer state actually does not change
+        const int id = QObject::startTimer(std::chrono::milliseconds{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 f283a264fa..81e8a5fccb 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 18ffbe2908..c318c3a625 100644
--- a/tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp
+++ b/tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp
@@ -39,6 +39,8 @@ static bool glibDisabled = []() {
 }();
 #endif
 
+using namespace std::chrono_literals;
+
 class tst_QTimer : public QObject
 {
     Q_OBJECT
@@ -94,6 +96,8 @@ private slots:
     void bindToTimer();
     void bindTimer();
     void automatedBindingTests();
+
+    void negativeInterval();
 };
 
 void tst_QTimer::zeroTimer()
@@ -166,7 +170,6 @@ void tst_QTimer::singleShotNormalizes_data()
 
 void tst_QTimer::singleShotNormalizes()
 {
-    using namespace std::chrono_literals;
     static constexpr auto TestTimeout = 250ms;
     QFETCH(QByteArray, slotName);
     QEventLoop loop;
@@ -1287,6 +1290,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()
@@ -1367,6 +1380,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