From 157cf118cea4963fa49d2d5a8943c45b4f07dd35 Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Thu, 22 May 2025 04:43:27 -0700 Subject: [PATCH] fix: Align `TimerManager` sequential ids and function error handling with web standard (#51500) Summary: Calls to create timers should return sequential ids (integers greater than zero in the spec's words). This regressed in the `TimerManager` implementation, which instead starts at zero inclusively. This has two side-effects for code assuming a spec-compliant implementation of `setTimeout` and `setInterval`: - Calls to `clearTimeout(0)` or `clearInterval(0)` will potentially cancel scheduled timers, although it's supposed to be a noop - Predicates like `if (timeoutId)` will fail since they assume non-negative ids The change in this PR is to align with WHATWG HTML 8.6.2 (Timers): https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#timers > otherwise, let id be an [implementation-defined](https://infra.spec.whatwg.org/#implementation-defined) integer that is **greater than zero** and does not already [exist](https://infra.spec.whatwg.org/#map-exists) in global's [map of setTimeout and setInterval IDs](https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#map-of-settimeout-and-setinterval-ids). Specifically, - we should return `0` to indicate that no timer was scheduled - we should start generating timer IDs at `1` instead of `0` This was previously raised in review comments here: https://github.com/facebook/react-native/pull/45092/files#r1650790008 The spec-incompliant behaviour was raised in an issue here: https://github.com/apollographql/apollo-client/issues/12632#issue-3075269978 This PR does not, - add bounds checking on `timerIndex_` and add a search of an available id that isn't in the unordered map - exclude `0` from being an accepted `TimerHandle` in `TimerManager::createTimer` or `TimerManager::deleteTimer` since the above bounds checking hasn't been added either ## Changelog: [GENERAL] [FIXED] - Align timer IDs and timer function argument error handling with web standards. Pull Request resolved: https://github.com/facebook/react-native/pull/51500 Test Plan: - Run `setTimeout` / `setInterval`; before applied changes the timeout for the first timer will be `0` - Run `setTimeout(null)`; before applied changes the timer ID will be non-zero - Run `setInterval(null)`; before applied changes an error will be thrown rather than `0` being returned Reviewed By: cipolleschi Differential Revision: D75145909 Pulled By: rshest fbshipit-source-id: 6646439abd29cf3cfa9e5cf0a57448e3b7cd1b48 --- .../ReactCommon/react/runtime/TimerManager.cpp | 10 ++++++---- .../ReactCommon/react/runtime/TimerManager.h | 4 +++- .../react/runtime/tests/cxx/ReactInstanceTest.cpp | 14 +++++++++----- 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/packages/react-native/ReactCommon/react/runtime/TimerManager.cpp b/packages/react-native/ReactCommon/react/runtime/TimerManager.cpp index 5514a268370..da81670bb45 100644 --- a/packages/react-native/ReactCommon/react/runtime/TimerManager.cpp +++ b/packages/react-native/ReactCommon/react/runtime/TimerManager.cpp @@ -300,8 +300,9 @@ void TimerManager::attachGlobals(jsi::Runtime& runtime) { } if (!args[0].isObject() || !args[0].asObject(rt).isFunction(rt)) { - // Do not throw any error to match web spec - return timerIndex_++; + // Do not throw any error to match web spec; instead return 0, an + // invalid timer id + return 0; } auto callback = args[0].getObject(rt).getFunction(rt); @@ -358,8 +359,9 @@ void TimerManager::attachGlobals(jsi::Runtime& runtime) { } if (!args[0].isObject() || !args[0].asObject(rt).isFunction(rt)) { - throw jsi::JSError( - rt, "The first argument to setInterval must be a function."); + // Do not throw any error to match web spec; instead return 0, an + // invalid timer id + return 0; } auto callback = args[0].getObject(rt).getFunction(rt); auto delay = count > 1 diff --git a/packages/react-native/ReactCommon/react/runtime/TimerManager.h b/packages/react-native/ReactCommon/react/runtime/TimerManager.h index 879ba30430a..914dec0b167 100644 --- a/packages/react-native/ReactCommon/react/runtime/TimerManager.h +++ b/packages/react-native/ReactCommon/react/runtime/TimerManager.h @@ -93,7 +93,9 @@ class TimerManager { // Each timeout that is registered on this queue gets a sequential id. This // is the global count from which those are assigned. - TimerHandle timerIndex_{0}; + // As per WHATWG HTML 8.6.1 (Timers) ids must be greater than zero, i.e. start + // at 1 + TimerHandle timerIndex_{1}; // The React Native microtask queue is used to back public APIs including // `queueMicrotask`, `clearImmediate`, and `setImmediate` (which is used by diff --git a/packages/react-native/ReactCommon/react/runtime/tests/cxx/ReactInstanceTest.cpp b/packages/react-native/ReactCommon/react/runtime/tests/cxx/ReactInstanceTest.cpp index 6e93d7a2b8a..63b5aa4a385 100644 --- a/packages/react-native/ReactCommon/react/runtime/tests/cxx/ReactInstanceTest.cpp +++ b/packages/react-native/ReactCommon/react/runtime/tests/cxx/ReactInstanceTest.cpp @@ -267,7 +267,9 @@ TEST_F(ReactInstanceTest, testSetTimeoutWithoutDelay) { EXPECT_CALL( *mockRegistry_, createTimer(_, 0)); // If delay is not provided, it should use 0 - eval("setTimeout(() => {});"); + auto val = eval("setTimeout(() => {});"); + expectNoError(); + EXPECT_EQ(val.asNumber(), 1); // First timer id should start at 1 } TEST_F(ReactInstanceTest, testSetTimeoutWithPassThroughArgs) { @@ -299,8 +301,9 @@ TEST_F(ReactInstanceTest, testSetTimeoutWithInvalidArgs) { getErrorMessage("setTimeout();"), "setTimeout must be called with at least one argument (the function to call)."); - eval("setTimeout('invalid');"); + auto val = eval("setTimeout('invalid')"); expectNoError(); + EXPECT_EQ(val.asNumber(), 0); eval("setTimeout(() => {}, 'invalid');"); expectNoError(); @@ -417,9 +420,10 @@ TEST_F(ReactInstanceTest, testSetIntervalWithInvalidArgs) { EXPECT_EQ( getErrorMessage("setInterval();"), "setInterval must be called with at least one argument (the function to call)."); - EXPECT_EQ( - getErrorMessage("setInterval('invalid', 100);"), - "The first argument to setInterval must be a function."); + + auto val = eval("setInterval('invalid', 100)"); + expectNoError(); + EXPECT_EQ(val.asNumber(), 0); } TEST_F(ReactInstanceTest, testClearInterval) {