From 3d419f51466c16c67edc42beb4e0d32dfefa2382 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Pasi=C5=84ski?= Date: Fri, 21 Jun 2024 14:47:57 +0200 Subject: [PATCH] fix: timers error handling and add timeout argument number coercion (fixes #45085) --- .../react/runtime/TimerManager.cpp | 42 ++++++++++++------- .../runtime/tests/cxx/ReactInstanceTest.cpp | 24 +++++------ .../js/examples/Timer/TimerExample.js | 4 +- 3 files changed, 42 insertions(+), 28 deletions(-) diff --git a/packages/react-native/ReactCommon/react/runtime/TimerManager.cpp b/packages/react-native/ReactCommon/react/runtime/TimerManager.cpp index 1f4cade5f856..db7b722bb586 100644 --- a/packages/react-native/ReactCommon/react/runtime/TimerManager.cpp +++ b/packages/react-native/ReactCommon/react/runtime/TimerManager.cpp @@ -13,6 +13,26 @@ namespace facebook::react { +double coerceNumberTimeout(jsi::Runtime& rt, const jsi::Value& timeout) { + double delay = 0.0; + + // fast-path + if (timeout.isNumber()) { + delay = timeout.getNumber(); + } + else { + // perform number coercion for timeout to be web spec compliant + auto numberCtor = rt.global().getPropertyAsObject(rt, "Number"); + auto delayNumberObject = numberCtor.getFunction(rt).callAsConstructor(rt, timeout).getObject(rt); + auto delayNumericValue = delayNumberObject.getPropertyAsFunction(rt, "valueOf").callWithThis(rt, delayNumberObject); + + delay = delayNumericValue.isNumber() ? delayNumericValue.getNumber() : 0; + } + + delay = std::isnan(delay) ? 0.0 : std::max(0.0, delay); + return delay; +} + namespace { inline const char* getTimerSourceName(TimerSource source) { switch (source) { @@ -221,9 +241,8 @@ void TimerManager::attachGlobals(jsi::Runtime& runtime) { } if (!args[0].isObject() || !args[0].asObject(rt).isFunction(rt)) { - throw jsi::JSError( - rt, - "The first argument to setImmediate must be a function."); + // Do not throw any error to match web spec + return timerIndex_++; } auto callback = args[0].getObject(rt).getFunction(rt); @@ -276,18 +295,12 @@ void TimerManager::attachGlobals(jsi::Runtime& runtime) { } if (!args[0].isObject() || !args[0].asObject(rt).isFunction(rt)) { - throw jsi::JSError( - rt, "The first argument to setTimeout must be a function."); + // Do not throw any error to match web spec + return timerIndex_++; } - auto callback = args[0].getObject(rt).getFunction(rt); - if (count > 1 && !args[1].isNumber() && !args[1].isUndefined()) { - throw jsi::JSError( - rt, - "The second argument to setTimeout must be a number or undefined."); - } - auto delay = - count > 1 && args[1].isNumber() ? args[1].getNumber() : 0; + auto callback = args[0].getObject(rt).getFunction(rt); + auto delay = count > 1 ? coerceNumberTimeout(rt, jsi::Value {rt, args[1] }) : 0.0; // Package up the remaining argument values into one place. std::vector moreArgs; @@ -344,8 +357,7 @@ void TimerManager::attachGlobals(jsi::Runtime& runtime) { rt, "The first argument to setInterval must be a function."); } auto callback = args[0].getObject(rt).getFunction(rt); - auto delay = - count > 1 && args[1].isNumber() ? args[1].getNumber() : 0; + auto delay = count > 1 ? coerceNumberTimeout(rt, jsi::Value { rt, args[1] }) : 0.0; // Package up the remaining argument values into one place. std::vector moreArgs; 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 b69d831aade2..c16928836a6b 100644 --- a/packages/react-native/ReactCommon/react/runtime/tests/cxx/ReactInstanceTest.cpp +++ b/packages/react-native/ReactCommon/react/runtime/tests/cxx/ReactInstanceTest.cpp @@ -294,12 +294,12 @@ TEST_F(ReactInstanceTest, testSetImmediateWithInvalidArgs) { EXPECT_EQ( getErrorMessage("setImmediate();"), "setImmediate must be called with at least one argument (a function to call)"); - EXPECT_EQ( - getErrorMessage("setImmediate('invalid');"), - "The first argument to setImmediate must be a function."); - EXPECT_EQ( - getErrorMessage("setImmediate({});"), - "The first argument to setImmediate must be a function."); + + eval("setImmediate('invalid');"); + expectNoError(); + + eval("setImmediate({});"); + expectNoError(); } TEST_F(ReactInstanceTest, testClearImmediate) { @@ -420,12 +420,12 @@ TEST_F(ReactInstanceTest, testSetTimeoutWithInvalidArgs) { EXPECT_EQ( getErrorMessage("setTimeout();"), "setTimeout must be called with at least one argument (the function to call)."); - EXPECT_EQ( - getErrorMessage("setTimeout('invalid');"), - "The first argument to setTimeout must be a function."); - EXPECT_EQ( - getErrorMessage("setTimeout(() => {}, 'invalid');"), - "The second argument to setTimeout must be a number or undefined."); + + eval("setTimeout('invalid');"); + expectNoError(); + + eval("setTimeout(() => {}, 'invalid');"); + expectNoError(); } TEST_F(ReactInstanceTest, testClearTimeout) { diff --git a/packages/rn-tester/js/examples/Timer/TimerExample.js b/packages/rn-tester/js/examples/Timer/TimerExample.js index 4e003845eb7e..ca49c80671f0 100644 --- a/packages/rn-tester/js/examples/Timer/TimerExample.js +++ b/packages/rn-tester/js/examples/Timer/TimerExample.js @@ -144,7 +144,7 @@ class RequestIdleCallbackTester extends React.Component< } type TimerTesterProps = $ReadOnly<{| - dt?: number, + dt?: any, type: string, |}>; @@ -328,6 +328,8 @@ exports.examples = [ + 200}} /> + ); },