Skip to content

fix: align timer functions' error handling with web standards #45092

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 8 commits into from
Closed

fix: align timer functions' error handling with web standards #45092

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
25b682e
fix: align timer functions' error handling with web standards
thejamesgore Jun 20, 2024
3767c33
Revert "Log a SoftException on SurfaceMountingManager.addRootView (#3…
thejamesgore Jun 21, 2024
01af5d5
Merge branch 'fix/timer-error-handling-45085' of github.com:thejamesg…
thejamesgore Jun 21, 2024
7cb4eb4
revert: Undo changes made to JSTimers.js and JSTimers-test.js based o…
thejamesgore Jun 21, 2024
b714330
revert: Undo linting error
thejamesgore Jun 21, 2024
6551a77
fix: Align timer functions' error handling with web standards in Time…
thejamesgore Jun 21, 2024
0612d38
fix: Align timer functions' error handling with web standards in Time…
thejamesgore Jun 21, 2024
f2241c2
Merge branch 'main' into fix/timer-error-handling-45085
thejamesgore Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 26 additions & 50 deletions packages/react-native/ReactCommon/react/runtime/TimerManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,49 +258,36 @@ void TimerManager::attachGlobals(jsi::Runtime& runtime) {
}

runtime.global().setProperty(
runtime,
"setTimeout",
jsi::Function::createFromHostFunction(
runtime,
jsi::PropNameID::forAscii(runtime, "setTimeout"),
3, // Function, delay, ...args
[this](
jsi::Runtime& rt,
const jsi::Value& thisVal,
const jsi::Value* args,
size_t count) {
runtime,
"setTimeout",
jsi::Function::createFromHostFunction(
runtime,
jsi::PropNameID::forAscii(runtime, "setTimeout"),
3, // Function, delay, ...args
[this](jsi::Runtime& rt, const jsi::Value& thisVal, const jsi::Value* args, size_t count) {
if (count == 0) {
throw jsi::JSError(
rt,
"setTimeout must be called with at least one argument (the function to call).");
throw jsi::JSError(rt, "setTimeout must be called with at least one argument (the function to call).");
}

if (!args[0].isObject() || !args[0].asObject(rt).isFunction(rt)) {
throw jsi::JSError(
rt, "The first argument to setTimeout must be a function.");
}
auto callback = args[0].getObject(rt).getFunction(rt);
auto callback = args[0].isObject() && args[0].asObject(rt).isFunction(rt)
? args[0].getObject(rt).getFunction(rt)
: jsi::Function::createFromHostFunction(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a host function for invalid calls seems wasteful and I don't think it's necessary. I saw that Chrome returns 0 for some types of calls to setTimeout (e.g.: setTimeout('')) so we could do that same here (just return jsi::Value(0).

You need to make another change to make that work though. In TimerManager.h we initialize the value with 0 (as the initial timer handle) and we should change that to 1, so the first timer doesn't conflict with an "invalid" one. When you do that, might be worth renaming timerIndex_ as nextTimerHandle_ for clarity.

This is another aspect where we'll align with the spec: https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#timer-initialisation-steps (the handle must be a value greater than 0, but we can use 0 for the invalid ones).

rt,
jsi::PropNameID::forAscii(rt, "dummyFunction"),
0,
[](jsi::Runtime& rt, const jsi::Value& thisVal, const jsi::Value* args, size_t count) {
return jsi::Value::undefined();
});

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 delay = (count > 1 && args[1].isNumber()) ? args[1].getNumber() : 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also check for delay < 0 and assign 0 in that case as well. You can use std::max(args[1].getNumber(), 0) in this same expression :)


// Package up the remaining argument values into one place.
std::vector<jsi::Value> moreArgs;
for (size_t extraArgNum = 2; extraArgNum < count; extraArgNum++) {
moreArgs.emplace_back(rt, args[extraArgNum]);
moreArgs.emplace_back(rt, args[extraArgNum]);
}

return createTimer(
std::move(callback),
std::move(moreArgs),
delay,
TimerSource::SetTimeout);
}));
return createTimer(std::move(callback), std::move(moreArgs), delay);
}));

runtime.global().setProperty(
runtime,
Expand Down Expand Up @@ -333,26 +320,15 @@ void TimerManager::attachGlobals(jsi::Runtime& runtime) {
const jsi::Value& thisVal,
const jsi::Value* args,
size_t count) {
if (count == 0) {
throw jsi::JSError(
rt,
"setInterval must be called with at least one argument (the function to call).");
}

if (!args[0].isObject() || !args[0].asObject(rt).isFunction(rt)) {
if (count == 0 || !args[0].isObject() ||
!args[0].asObject(rt).isFunction(rt)) {
throw jsi::JSError(
rt, "The first argument to setInterval must be a function.");
rt, "setInterval must be called with a function.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setInterval should behave the same way as setTimeout. We shouldn't fail if the value isn't a function but just return early with a 0 handle.

}
auto callback = args[0].getObject(rt).getFunction(rt);
auto delay =
count > 1 && args[1].isNumber() ? args[1].getNumber() : 0;

// Package up the remaining argument values into one place.
std::vector<jsi::Value> moreArgs;
for (size_t extraArgNum = 2; extraArgNum < count; extraArgNum++) {
moreArgs.emplace_back(rt, args[extraArgNum]);
}

(count > 1 && args[1].isNumber()) ? args[1].getNumber() : 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same about delay being greater or equal to zero.

std::vector<jsi::Value> moreArgs(args + 2, args + count);
return createRecurringTimer(
std::move(callback),
std::move(moreArgs),
Expand Down