Qt Timers: disallow setting negative intervals

As disccussed in the code review. Replace negative values with 1ms (not
0ms as that would spin up the event-loop too many times).

Move QTimer::start(std::chrono::milliseconds msec) API docs next to the
method definition so as to not forget changing it when the
implementation changes.

Drive-by, qWarning() and co. are marked cold so there is no need to use
Q_UNLIKELY in an if-condition leading to calling these methods.

[ChangeLog][QtCore][Important Behvaior Change] Timers with negative
intervals aren't allowed anymore, that is, if you try to start a timer
(or set the interval) with a negative value, that interval will be set
to 1ms. Previously Qt timers would let you set the interval to a
negative value, but behave in surprising ways (for example stop the
timer if it was running or not start it at all). This change affects
QTimer, QChronoTimer, QBasicTimer and QObject::startTimer().

Fixes: QTBUG-132359
Change-Id: I09309063665db051337f91160971993d9ce7911e
Reviewed-by: Thiago Macieira <[email protected]>

8 files changed, 133 insertions, 68 deletions

diff --git a/src/corelib/kernel/qbasictimer.cpp b/src/corelib/kernel/qbasictimer.cpp
index b7e2f114519..60387381da6 100644
--- a/src/corelib/kernel/qbasictimer.cpp
+++ b/src/corelib/kernel/qbasictimer.cpp
@@ -145,6 +145,8 @@ QT_BEGIN_NAMESPACE
 
     The given \a object will receive timer events.
 
+    \include timers-common.qdocinc negative-intervals-not-allowed
+
 //! [start-nanoseconds-note]
     \note Starting from Qt 6.9 this method takes std::chrono::nanoseconds,
           before that it took std::chrono::milliseconds. This change is
@@ -170,6 +172,8 @@ QT_BEGIN_NAMESPACE
     given \a timerType. See Qt::TimerType for information on the different
     timer types.
 
+    \include timers-common.qdocinc negative-intervals-not-allowed
+
     \a obj will receive timer events.
 
     \include qbasictimer.cpp start-nanoseconds-note
@@ -179,9 +183,10 @@ QT_BEGIN_NAMESPACE
 void QBasicTimer::start(Duration duration, Qt::TimerType timerType, QObject *obj)
 {
     QAbstractEventDispatcher *eventDispatcher = QAbstractEventDispatcher::instance();
-    if (Q_UNLIKELY(duration < 0ns)) {
-        qWarning("QBasicTimer::start: Timers cannot have negative timeouts");
-        return;
+    if (duration < 0ns) {
+        qWarning("QBasicTimer::start: negative intervals aren't allowed; the "
+                 "interval will be set to 1ms.");
+        duration = 1ms;
     }
     if (Q_UNLIKELY(!eventDispatcher)) {
         qWarning("QBasicTimer::start: QBasicTimer can only be used with threads started with QThread");
diff --git a/src/corelib/kernel/qchronotimer.cpp b/src/corelib/kernel/qchronotimer.cpp
index a0f89f3a208..0d36d22bcb7 100644
--- a/src/corelib/kernel/qchronotimer.cpp
+++ b/src/corelib/kernel/qchronotimer.cpp
@@ -279,10 +279,18 @@ QBindable<bool> QChronoTimer::bindableSingleShot()
     stop() and then start() the timer, and acquire a new id().
     If the timer is not running, only the interval is changed.
 
+    \include timers-common.qdocinc negative-intervals-not-allowed
+
     \sa singleShot
 */
 void QChronoTimer::setInterval(std::chrono::nanoseconds nsec)
 {
+    if (nsec < 0ns) {
+        qWarning("QChronoTimer::setInterval: negative intervals aren't allowed; the "
+                 "interval will be set to 1ms.");
+        nsec = 1ms;
+    }
+
     auto *d = d_func();
     d->intervalDuration.removeBindingUnlessInWrapper();
     const bool intervalChanged = nsec != d->intervalDuration.valueBypassingBindings();
diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp
index d3566b1081e..707c12ae0fd 100644
--- a/src/corelib/kernel/qobject.cpp
+++ b/src/corelib/kernel/qobject.cpp
@@ -1821,6 +1821,8 @@ void QObjectPrivate::setThreadData_helper(QThreadData *currentData, QThreadData
     startTimer(std::chrono::milliseconds{interval}, timerType);
     \endcode
 
+    \include timers-common.qdocinc negative-intervals-not-allowed
+
     \sa timerEvent(), killTimer(), QChronoTimer, QBasicTimer
 */
 
@@ -1843,6 +1845,8 @@ int QObject::startTimer(int interval, Qt::TimerType timerType)
     then the timer event occurs once every time there are no more window
     system events to process.
 
+    \include timers-common.qdocinc negative-intervals-not-allowed
+
     The virtual timerEvent() function is called with the QTimerEvent
     event parameter class when a timer event occurs. Reimplement this
     function to get timer events.
@@ -1887,9 +1891,10 @@ int QObject::startTimer(std::chrono::nanoseconds interval, Qt::TimerType timerTy
 
     using namespace std::chrono_literals;
 
-    if (Q_UNLIKELY(interval < 0ns)) {
-        qWarning("QObject::startTimer: Timers cannot have negative intervals");
-        return 0;
+    if (interval < 0ns) {
+        qWarning("QObject::startTimer: negative intervals aren't allowed; the "
+                 "interval will be set to 1ms.");
+        interval = 1ms;
     }
 
     auto thisThreadData = d->threadData.loadRelaxed();
diff --git a/src/corelib/kernel/qtimer.cpp b/src/corelib/kernel/qtimer.cpp
index e3b4b0ec36d..796a82afe0b 100644
--- a/src/corelib/kernel/qtimer.cpp
+++ b/src/corelib/kernel/qtimer.cpp
@@ -233,6 +233,8 @@ void QTimer::start()
         timer.start();
     \endcode
 
+    \include timers-common.qdocinc negative-intervals-not-allowed
+
     \note   Keeping the event loop busy with a zero-timer is bound to
             cause trouble and highly erratic behavior of the UI.
 */
@@ -241,9 +243,40 @@ void QTimer::start(int msec)
     start(msec * 1ms);
 }
 
+static std::chrono::milliseconds
+checkInterval(const char *caller, std::chrono::milliseconds interval)
+{
+    if (interval < 0ms) {
+        qWarning("%s: negative intervals aren't allowed; the interval will be set to 1ms.", caller);
+        interval = 1ms;
+    }
+    return interval;
+}
+
+/*!
+    \since 5.8
+    \overload
+
+    Starts or restarts the timer with a timeout of duration \a interval milliseconds.
+
+    \include qtimer.cpp stop-restart-timer
+
+    \include qtimer.cpp singleshot-activation
+    This is equivalent to:
+
+    \code
+        timer.setInterval(interval);
+        timer.start();
+    \endcode
+
+    \include timers-common.qdocinc negative-intervals-not-allowed
+*/
 void QTimer::start(std::chrono::milliseconds interval)
 {
     Q_D(QTimer);
+
+    interval = checkInterval("QTimer::start", interval);
+
     // This could be narrowing as the interval is stored in an `int` QProperty,
     // and the type can't be changed in Qt6.
     const int msec = interval.count();
@@ -370,6 +403,8 @@ void QTimer::singleShotImpl(std::chrono::nanoseconds ns, Qt::TimerType timerType
     The \a receiver is the receiving object and the \a member is the
     slot. The time interval is \a msec milliseconds.
 
+    \include timers-common.qdocinc negative-intervals-not-allowed
+
     \sa start()
 */
 
@@ -388,6 +423,8 @@ void QTimer::singleShotImpl(std::chrono::nanoseconds ns, Qt::TimerType timerType
     time interval is \a msec milliseconds. The \a timerType affects the
     accuracy of the timer.
 
+    \include timers-common.qdocinc negative-intervals-not-allowed
+
     \sa start()
 */
 
@@ -395,8 +432,9 @@ void QTimer::singleShot(std::chrono::nanoseconds ns, Qt::TimerType timerType,
                         const QObject *receiver, const char *member)
 {
     if (ns < 0ns) {
-        qWarning("QTimer::singleShot: Timers cannot have negative timeouts");
-        return;
+        qWarning("QTimer::singleShot: negative intervals aren't allowed; the "
+                 "interval will be set to 1ms.");
+        ns = 1ms;
     }
     if (receiver && member) {
         if (ns == 0ns) {
@@ -440,6 +478,8 @@ void QTimer::singleShot(std::chrono::nanoseconds ns, Qt::TimerType timerType,
     The \a interval parameter can be an \c int (interpreted as a millisecond
     count) or a \c std::chrono type that implicitly converts to nanoseconds.
 
+    \include timers-common.qdocinc negative-intervals-not-allowed
+
     \note In Qt versions prior to 6.8, the chrono overloads took chrono::milliseconds,
     not chrono::nanoseconds. The compiler will automatically convert for you,
     but the conversion may overflow for extremely large milliseconds counts.
@@ -462,6 +502,8 @@ void QTimer::singleShot(std::chrono::nanoseconds ns, Qt::TimerType timerType,
     The \a receiver is the receiving object and the \a member is the slot. The
     time interval is given in the duration object \a nsec.
 
+    \include timers-common.qdocinc negative-intervals-not-allowed
+
 //! [qtimer-ns-overflow]
     \note In Qt versions prior to 6.8, this function took chrono::milliseconds,
     not chrono::nanoseconds. The compiler will automatically convert for you,
@@ -487,6 +529,9 @@ void QTimer::singleShot(std::chrono::nanoseconds ns, Qt::TimerType timerType,
     time interval is given in the duration object \a nsec. The \a timerType affects the
     accuracy of the timer.
 
+
+    \include timers-common.qdocinc negative-intervals-not-allowed
+
     \include qtimer.cpp qtimer-ns-overflow
 
     \sa start()
@@ -527,24 +572,6 @@ void QTimer::singleShot(std::chrono::nanoseconds ns, Qt::TimerType timerType,
 */
 
 /*!
-    \fn void QTimer::start(std::chrono::milliseconds msec)
-    \since 5.8
-    \overload
-
-    Starts or restarts the timer with a timeout of duration \a msec milliseconds.
-
-    \include qtimer.cpp stop-restart-timer
-
-    \include qtimer.cpp singleshot-activation
-    This is equivalent to:
-
-    \code
-        timer.setInterval(msec);
-        timer.start();
-    \endcode
-*/
-
-/*!
     \fn std::chrono::milliseconds QTimer::intervalAsDuration() const
     \since 5.8
 
@@ -604,6 +631,8 @@ QBindable<bool> QTimer::bindableSingleShot()
     stop() and then start() the timer, and acquire a new id().
     If the timer is not running, only the interval is changed.
 
+    \include timers-common.qdocinc negative-intervals-not-allowed
+
     \sa singleShot
 */
 void QTimer::setInterval(int msec)
@@ -614,6 +643,9 @@ void QTimer::setInterval(int msec)
 void QTimer::setInterval(std::chrono::milliseconds interval)
 {
     Q_D(QTimer);
+
+    interval = checkInterval("QTimer::setInterval", interval);
+
     // This could be narrowing as the interval is stored in an `int` QProperty,
     // and the type can't be changed in Qt6.
     const int msec = interval.count();
diff --git a/src/corelib/kernel/timers-common.qdocinc b/src/corelib/kernel/timers-common.qdocinc
index 2c87d6cf577..e801df3b2a3 100644
--- a/src/corelib/kernel/timers-common.qdocinc
+++ b/src/corelib/kernel/timers-common.qdocinc
@@ -22,3 +22,11 @@
     A disadvantage of using timerEvent() is that some high-level features,
     such as single-shot timers and signals, aren't supported.
 //! [q-chrono-timer-alternatives]
+
+// \1 is the method argument's name e.g. `\a interval`
+//! [negative-intervals-not-allowed]
+    Starting from Qt 6.10, setting a negative interval will result in a run-time warning
+    and the value being reset to 1ms. Before Qt 6.10 a Qt Timer would let you set a negative
+    interval but behave in surprising ways (for example stop the timer if it was running or
+    not start it at all).
+//! [negative-intervals-not-allowed]
diff --git a/tests/auto/corelib/kernel/qchronotimer/tst_qchronotimer.cpp b/tests/auto/corelib/kernel/qchronotimer/tst_qchronotimer.cpp
index 22ad3d49286..96bbd60ab83 100644
--- a/tests/auto/corelib/kernel/qchronotimer/tst_qchronotimer.cpp
+++ b/tests/auto/corelib/kernel/qchronotimer/tst_qchronotimer.cpp
@@ -1090,14 +1090,15 @@ void tst_QChronoTimer::bindToTimer()
 
     auto ignoreMsg = [] {
         QTest::ignoreMessage(QtWarningMsg,
-                             "QObject::startTimer: Timers cannot have negative intervals");
+                             "QChronoTimer::setInterval: negative intervals aren't allowed; the "
+                             "interval will be set to 1ms.");
     };
 
     ignoreMsg();
     timer.setInterval(-100ms);
-    ignoreMsg();
     timer.start();
-    QVERIFY(!active);
+    QVERIFY(active);
+    QCOMPARE(timer.interval(), 1ms);
 
     timer.setInterval(100ms);
     timer.start();
@@ -1105,9 +1106,9 @@ void tst_QChronoTimer::bindToTimer()
 
     ignoreMsg();
     timer.setInterval(-100ms);
-    ignoreMsg();
     timer.start();
-    QVERIFY(!active);
+    QVERIFY(active);
+    QCOMPARE(timer.interval(), 1ms);
 }
 
 void tst_QChronoTimer::bindTimer()
@@ -1196,30 +1197,24 @@ void tst_QChronoTimer::negativeInterval()
 
     auto ignoreMsg = [] {
         QTest::ignoreMessage(QtWarningMsg,
-                             "QObject::startTimer: Timers cannot have negative intervals");
+                             "QChronoTimer::setInterval: negative intervals aren't allowed; the "
+                             "interval will be set to 1ms.");
     };
 
     ignoreMsg();
-    // Setting a negative interval does not change the active state.
     timer.setInterval(-100ms);
-    ignoreMsg();
     timer.start();
-    QVERIFY(!timer.isActive());
+    QVERIFY(timer.isActive());
+    QCOMPARE(timer.interval(), 1ms);
 
-    // Starting a timer that has a positive interval, the active state is changed
     timer.setInterval(100ms);
     timer.start();
     QVERIFY(timer.isActive());
 
     ignoreMsg();
-    // Setting a negative interval on an already running timer...
     timer.setInterval(-100ms);
-    // ... the timer is stopped and the active state is changed
-    QVERIFY(!timer.isActive());
-
-    // Calling start on a timer that has a negative interval, does not change the active state
-    timer.start();
-    QVERIFY(!timer.isActive());
+    QVERIFY(timer.isActive());
+    QCOMPARE(timer.interval(), 1ms);
 }
 
 class OrderHelper : public QObject
diff --git a/tests/auto/corelib/kernel/qobject/tst_qobject.cpp b/tests/auto/corelib/kernel/qobject/tst_qobject.cpp
index 4e547bad255..6c804044378 100644
--- a/tests/auto/corelib/kernel/qobject/tst_qobject.cpp
+++ b/tests/auto/corelib/kernel/qobject/tst_qobject.cpp
@@ -39,6 +39,7 @@
 
 #include <math.h>
 
+using namespace std::chrono_literals;
 using namespace Qt::StringLiterals;
 
 class tst_QObject : public QObject
@@ -156,6 +157,7 @@ private slots:
     void declarativeData();
     void asyncCallbackHelper();
     void disconnectQueuedConnection_pendingEventsAreDelivered();
+    void timerWithNegativeInterval();
 };
 
 struct QObjectCreatedOnShutdown
@@ -8967,5 +8969,15 @@ void tst_QObject::disconnectQueuedConnection_pendingEventsAreDelivered()
     QTRY_COMPARE(receiver.count_slot1, 1);
 }
 
+void tst_QObject::timerWithNegativeInterval()
+{
+    QObject obj;
+    QTest::ignoreMessage(QtWarningMsg,
+                         "QObject::startTimer: negative intervals aren't allowed; the "
+                         "interval will be set to 1ms.");
+    int id = obj.startTimer(-100ms);
+    QCOMPARE_NE(Qt::TimerId{id}, Qt::TimerId::Invalid);
+}
+
 QTEST_MAIN(tst_QObject)
 #include "tst_qobject.moc"
diff --git a/tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp b/tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp
index e2485e8fd3e..aff1ae07008 100644
--- a/tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp
+++ b/tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp
@@ -1307,22 +1307,23 @@ void tst_QTimer::bindToTimer()
     timer.stop();
     QVERIFY(!active);
 
-    auto ignoreMsg = [] {
-        QTest::ignoreMessage(QtWarningMsg,
-                             "QObject::startTimer: Timers cannot have negative intervals");
-    };
-
     // also test that using negative interval updates the binding correctly
     timer.start(100);
     QVERIFY(active);
-    ignoreMsg();
+    QTest::ignoreMessage(QtWarningMsg,
+                         "QTimer::setInterval: negative intervals aren't allowed; the interval "
+                         "will be set to 1ms.");
     timer.setInterval(-100);
-    QVERIFY(!active);
+    QVERIFY(active);
+    QCOMPARE(timer.intervalAsDuration(), 1ms);
     timer.start(100);
     QVERIFY(active);
-    ignoreMsg();
+    QTest::ignoreMessage(QtWarningMsg,
+                         "QTimer::start: negative intervals aren't allowed; the interval "
+                         "will be set to 1ms.");
     timer.start(-100);
-    QVERIFY(!active);
+    QVERIFY(active);
+    QCOMPARE(timer.intervalAsDuration(), 1ms);
 }
 
 void tst_QTimer::bindTimer()
@@ -1405,33 +1406,32 @@ void tst_QTimer::automatedBindingTests()
 
 void tst_QTimer::negativeInterval()
 {
-    auto ignoreMsg = [] {
-        QTest::ignoreMessage(QtWarningMsg,
-                             "QObject::startTimer: Timers cannot have negative intervals");
-    };
-
     QTimer timer;
 
-    // Starting with a negative interval does not change active state.
-    ignoreMsg();
+    QTest::ignoreMessage(QtWarningMsg,
+                         "QTimer::start: negative intervals aren't allowed; the interval "
+                         "will be set to 1ms.");
     timer.start(-100ms);
-    QVERIFY(!timer.isActive());
+    QVERIFY(timer.isActive());
+    QCOMPARE(timer.intervalAsDuration(), 1ms);
 
-    // Updating the interval to a negative value stops the timer and changes
-    // the active state.
     timer.start(100ms);
     QVERIFY(timer.isActive());
-    ignoreMsg();
+    QTest::ignoreMessage(QtWarningMsg,
+                         "QTimer::setInterval: negative intervals aren't allowed; the interval "
+                         "will be set to 1ms.");
     timer.setInterval(-100);
-    QVERIFY(!timer.isActive());
+    QVERIFY(timer.isActive());
+    QCOMPARE(timer.intervalAsDuration(), 1ms);
 
-    // Starting with a negative interval when already started leads to stop
-    // and inactive state.
     timer.start(100);
     QVERIFY(timer.isActive());
-    ignoreMsg();
+    QTest::ignoreMessage(QtWarningMsg,
+                         "QTimer::start: negative intervals aren't allowed; the interval "
+                         "will be set to 1ms.");
     timer.start(-100ms);
-    QVERIFY(!timer.isActive());
+    QVERIFY(timer.isActive());
+    QCOMPARE(timer.intervalAsDuration(), 1ms);
 }
 
 class OrderHelper : public QObject