Skip to content

Commit

Permalink
Prevent race conditions in TimerManager (facebook#48519)
Browse files Browse the repository at this point in the history 
Summary:

When working on RN instance reload in D67818969 I noticed that we can crash in TimerManager when the RN instance is reloaded.

{F1974211648}

This change aims to fix it.

NOTE: `TimerManager` is a shared_ptr

React Native Granite

https://www.internalfb.com/code/fbsource/[46cacd0162d251cfcd08f069f20eaec600f59c7f]/xplat/ReactNative/react-native-cxx/react/runtime/ReactHost.cpp?lines=119-120

React Native iOS / Android

https://www.internalfb.com/code/search?q=-filepath%3ATimerManager.h%7CTimerManager.cpp%20filepath%3Areact-native-github%20case%3Ayes%20repo%3Afbsource%20TimerManager&lang_filter=cpp%2Cobjective-cpp

Differential Revision: D67888312
  • Loading branch information
@christophpurrer @facebook-github-bot
christophpurrer authored and facebook-github-bot committed Jan 7, 2025
1 parent ec72af4 commit f04db61
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 46 deletions.
120 changes: 75 additions & 45 deletions packages/react-native/ReactCommon/react/runtime/TimerManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ namespace facebook::react {

namespace {

constexpr int INVALID_TIMER_ID = -1;

double coerceNumberTimeout(jsi::Runtime& rt, const jsi::Value& timeout) {
double delay = 0.0;

Expand Down Expand Up @@ -195,29 +197,34 @@ void TimerManager::deleteRecurringTimer(
}

void TimerManager::callTimer(TimerHandle timerHandle) {
runtimeExecutor_([this, timerHandle](jsi::Runtime& runtime) {
auto it = timers_.find(timerHandle);
if (it != timers_.end()) {
auto& timerCallback = it->second;
bool repeats = timerCallback.repeat;

{
TraceSection s(
"TimerManager::callTimer",
"id",
timerHandle,
"type",
getTimerSourceName(timerCallback.source));
timerCallback.invoke(runtime);
}

if (!repeats) {
// Invoking a timer has the potential to delete it. Do not re-use the
// existing iterator to erase it from the map.
timers_.erase(timerHandle);
}
}
});
runtimeExecutor_(
[weakThis = weak_from_this(), timerHandle](jsi::Runtime& runtime) {
auto strongThis = weakThis.lock();
if (!strongThis) {
return;
}
if (auto it = strongThis->timers_.find(timerHandle);
it != strongThis->timers_.end()) {
auto& timerCallback = it->second;
bool repeats = timerCallback.repeat;

{
TraceSection s(
"TimerManager::callTimer",
"id",
timerHandle,
"type",
getTimerSourceName(timerCallback.source));
timerCallback.invoke(runtime);
}

if (!repeats) {
// Invoking a timer has the potential to delete it. Do not re-use
// the existing iterator to erase it from the map.
strongThis->timers_.erase(timerHandle);
}
}
});
}

void TimerManager::attachGlobals(jsi::Runtime& runtime) {
Expand All @@ -234,11 +241,16 @@ void TimerManager::attachGlobals(jsi::Runtime& runtime) {
runtime,
jsi::PropNameID::forAscii(runtime, "setImmediate"),
2, // Function, ...args
[this](
[weakThis = weak_from_this()](
jsi::Runtime& rt,
const jsi::Value& thisVal,
const jsi::Value* args,
size_t count) {
auto strongThis = weakThis.lock();
if (!strongThis) {
return INVALID_TIMER_ID;
}

if (count == 0) {
throw jsi::JSError(
rt,
Expand All @@ -247,7 +259,7 @@ 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_++;
return strongThis->timerIndex_++;
}
auto callback = args[0].getObject(rt).getFunction(rt);

Expand All @@ -257,7 +269,7 @@ void TimerManager::attachGlobals(jsi::Runtime& runtime) {
moreArgs.emplace_back(rt, args[extraArgNum]);
}

return createReactNativeMicrotask(
return strongThis->createReactNativeMicrotask(
std::move(callback), std::move(moreArgs));
}));

Expand All @@ -268,14 +280,15 @@ void TimerManager::attachGlobals(jsi::Runtime& runtime) {
runtime,
jsi::PropNameID::forAscii(runtime, "clearImmediate"),
1, // handle
[this](
[weakThis = weak_from_this()](
jsi::Runtime& rt,
const jsi::Value& thisVal,
const jsi::Value* args,
size_t count) {
if (count > 0 && args[0].isNumber()) {
if (auto strongThis = weakThis.lock();
strongThis != nullptr && count > 0 && args[0].isNumber()) {
auto handle = (TimerHandle)args[0].asNumber();
deleteReactNativeMicrotask(rt, handle);
strongThis->deleteReactNativeMicrotask(rt, handle);
}
return jsi::Value::undefined();
}));
Expand All @@ -288,11 +301,15 @@ void TimerManager::attachGlobals(jsi::Runtime& runtime) {
runtime,
jsi::PropNameID::forAscii(runtime, "setTimeout"),
3, // Function, delay, ...args
[this](
[weakThis = weak_from_this()](
jsi::Runtime& rt,
const jsi::Value& thisVal,
const jsi::Value* args,
size_t count) {
auto strongThis = weakThis.lock();
if (!strongThis) {
return INVALID_TIMER_ID;
}
if (count == 0) {
throw jsi::JSError(
rt,
Expand All @@ -301,7 +318,7 @@ 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_++;
return strongThis->timerIndex_++;
}

auto callback = args[0].getObject(rt).getFunction(rt);
Expand All @@ -313,7 +330,7 @@ void TimerManager::attachGlobals(jsi::Runtime& runtime) {
moreArgs.emplace_back(rt, args[extraArgNum]);
}

return createTimer(
return strongThis->createTimer(
std::move(callback),
std::move(moreArgs),
delay,
Expand All @@ -327,14 +344,15 @@ void TimerManager::attachGlobals(jsi::Runtime& runtime) {
runtime,
jsi::PropNameID::forAscii(runtime, "clearTimeout"),
1, // timerID
[this](
[weakThis = weak_from_this()](
jsi::Runtime& rt,
const jsi::Value& thisVal,
const jsi::Value* args,
size_t count) {
if (count > 0 && args[0].isNumber()) {
if (auto strongThis = weakThis.lock();
strongThis != nullptr && count > 0 && args[0].isNumber()) {
auto handle = (TimerHandle)args[0].asNumber();
deleteTimer(rt, handle);
strongThis->deleteTimer(rt, handle);
}
return jsi::Value::undefined();
}));
Expand All @@ -346,11 +364,16 @@ void TimerManager::attachGlobals(jsi::Runtime& runtime) {
runtime,
jsi::PropNameID::forAscii(runtime, "setInterval"),
3, // Function, delay, ...args
[this](
[weakThis = weak_from_this()](
jsi::Runtime& rt,
const jsi::Value& thisVal,
const jsi::Value* args,
size_t count) {
auto strongThis = weakThis.lock();
if (!strongThis) {
return INVALID_TIMER_ID;
}

if (count == 0) {
throw jsi::JSError(
rt,
Expand All @@ -372,7 +395,7 @@ void TimerManager::attachGlobals(jsi::Runtime& runtime) {
moreArgs.emplace_back(rt, args[extraArgNum]);
}

return createRecurringTimer(
return strongThis->createRecurringTimer(
std::move(callback),
std::move(moreArgs),
delay,
Expand All @@ -386,14 +409,15 @@ void TimerManager::attachGlobals(jsi::Runtime& runtime) {
runtime,
jsi::PropNameID::forAscii(runtime, "clearInterval"),
1, // timerID
[this](
[weakThis = weak_from_this()](
jsi::Runtime& rt,
const jsi::Value& thisVal,
const jsi::Value* args,
size_t count) {
if (count > 0 && args[0].isNumber()) {
if (auto strongThis = weakThis.lock();
strongThis != nullptr && count > 0 && args[0].isNumber()) {
auto handle = (TimerHandle)args[0].asNumber();
deleteRecurringTimer(rt, handle);
strongThis->deleteRecurringTimer(rt, handle);
}
return jsi::Value::undefined();
}));
Expand All @@ -405,11 +429,16 @@ void TimerManager::attachGlobals(jsi::Runtime& runtime) {
runtime,
jsi::PropNameID::forAscii(runtime, "requestAnimationFrame"),
1, // callback
[this](
[weakThis = weak_from_this()](
jsi::Runtime& rt,
const jsi::Value& thisVal,
const jsi::Value* args,
size_t count) {
auto strongThis = weakThis.lock();
if (!strongThis) {
return INVALID_TIMER_ID;
}

if (count == 0) {
throw jsi::JSError(
rt,
Expand Down Expand Up @@ -442,7 +471,7 @@ void TimerManager::attachGlobals(jsi::Runtime& runtime) {
// The current implementation of requestAnimationFrame is the same
// as setTimeout(0). This isn't exactly how requestAnimationFrame
// is supposed to work on web, and may change in the future.
return createTimer(
return strongThis->createTimer(
std::move(callback),
std::vector<jsi::Value>(),
/* delay */ 0,
Expand All @@ -456,14 +485,15 @@ void TimerManager::attachGlobals(jsi::Runtime& runtime) {
runtime,
jsi::PropNameID::forAscii(runtime, "cancelAnimationFrame"),
1, // timerID
[this](
[weakThis = weak_from_this()](
jsi::Runtime& rt,
const jsi::Value& thisVal,
const jsi::Value* args,
size_t count) {
if (count > 0 && args[0].isNumber()) {
if (auto strongThis = weakThis.lock();
strongThis != nullptr && count > 0 && args[0].isNumber()) {
auto handle = (TimerHandle)args[0].asNumber();
deleteTimer(rt, handle);
strongThis->deleteTimer(rt, handle);
}
return jsi::Value::undefined();
}));
Expand Down
2 changes: 1 addition & 1 deletion packages/react-native/ReactCommon/react/runtime/TimerManager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ struct TimerCallback {
TimerSource source;
};

class TimerManager {
class TimerManager : public std::enable_shared_from_this<TimerManager> {
public:
explicit TimerManager(
std::unique_ptr<PlatformTimerRegistry> platformTimerRegistry) noexcept;
Expand Down

0 comments on commit f04db61

Please sign in to comment.