Skip to content

Commit 9f8d250

Browse files
javachemeta-codesync[bot]
authored andcommitted
Fix callback and promise teardown races (#59670)
Summary: Pull Request resolved: #59670 Keep JavaScript callback and promise values rooted while native owners retain weak lifetime handles. Release inline on the JavaScript thread and dispatch only off-thread cleanup, preventing native invocation or destruction from retaining these values across runtime teardown. Apply the same ownership policy to asynchronous callbacks and promises. This builds on the investigation and reproducer contributed by HyunWoo Lee in #59579. Changelog: [General][Fixed] - Fix TurboModule callback and async promise cleanup racing runtime teardown [General][Deprecated] - Deprecate CallbackWrapper in favour of LongLivedObjectHandle Differential Revision: D128581452
1 parent 687e2ed commit 9f8d250

18 files changed

Lines changed: 1315 additions & 140 deletions

‎packages/react-native/ReactCommon/react/bridging/CallbackWrapper.h‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,10 @@ namespace facebook::react {
1919

2020
class CallInvoker;
2121

22-
// Helper for passing jsi::Function arg to other methods.
22+
/**
23+
* @deprecated Use LongLivedObjectHandle<jsi::Function> instead.
24+
* Access and release this legacy wrapper only on the JavaScript thread.
25+
*/
2326
class CallbackWrapper : public LongLivedObject {
2427
private:
2528
CallbackWrapper(jsi::Function &&callback, jsi::Runtime &runtime, std::shared_ptr<CallInvoker> jsInvoker)

‎packages/react-native/ReactCommon/react/bridging/Function.h‎

Lines changed: 60 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
#include <react/cxxstableapi/UmbrellaGuard.h>
1111

1212
#include <react/bridging/Base.h>
13-
#include <react/bridging/CallbackWrapper.h>
13+
#include <react/bridging/CallbackWrapper.h> // Retain the legacy transitive include.
1414

1515
#include <ReactCommon/CallInvoker.h>
1616
#include <ReactCommon/SchedulerPriority.h>
@@ -61,16 +61,18 @@ class AsyncCallback {
6161

6262
void callWithArgs(std::optional<SchedulerPriority> priority, Args... args) const noexcept
6363
{
64-
if (auto wrapper = callback_->wrapper_.lock()) {
65-
auto fn = [callback = callback_,
64+
auto callback = callback_;
65+
if (auto jsInvoker = callback->function_.lockInvoker()) {
66+
// The queued function owns the tuple; its lifetime is opaque to the analyzer.
67+
// NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks)
68+
auto fn = [callback = std::move(callback),
6669
argsPtr = std::make_shared<std::tuple<Args...>>(std::make_tuple(std::forward<Args>(args)...))](
67-
jsi::Runtime &) { callback->apply(std::move(*argsPtr)); };
70+
jsi::Runtime &rt) { callback->apply(rt, std::move(*argsPtr)); };
6871

69-
auto &jsInvoker = wrapper->jsInvoker();
7072
if (priority) {
71-
jsInvoker.invokeAsync(*priority, std::move(fn));
73+
jsInvoker->invokeAsync(*priority, std::move(fn));
7274
} else {
73-
jsInvoker.invokeAsync(std::move(fn));
75+
jsInvoker->invokeAsync(std::move(fn));
7476
}
7577
}
7678
}
@@ -79,80 +81,77 @@ class AsyncCallback {
7981
std::optional<SchedulerPriority> priority,
8082
std::function<void(jsi::Runtime &, jsi::Function &)> &&callImpl) const noexcept
8183
{
82-
if (auto wrapper = callback_->wrapper_.lock()) {
83-
// Capture callback_ and not wrapper_. If callback_ is deallocated or the
84-
// JSVM is shutdown before the async task is scheduled, the underlying
85-
// function will have been deallocated.
86-
auto fn = [callback = callback_, callImpl = std::move(callImpl)](jsi::Runtime &rt) {
87-
if (auto wrapper2 = callback->wrapper_.lock()) {
88-
callImpl(rt, wrapper2->callback());
84+
auto callback = callback_;
85+
if (auto jsInvoker = callback->function_.lockInvoker()) {
86+
// Retain native ownership until this task is destroyed, without retaining
87+
// a JavaScript value while the task is queued or discarded.
88+
auto fn = [callback = std::move(callback), callImpl = std::move(callImpl)](jsi::Runtime &rt) {
89+
if (auto function = callback->function_.lockOnJS(rt)) {
90+
callImpl(rt, function->value);
8991
}
9092
};
9193

92-
auto &jsInvoker = wrapper->jsInvoker();
9394
if (priority) {
94-
jsInvoker.invokeAsync(*priority, std::move(fn));
95+
jsInvoker->invokeAsync(*priority, std::move(fn));
9596
} else {
96-
jsInvoker.invokeAsync(std::move(fn));
97+
jsInvoker->invokeAsync(std::move(fn));
9798
}
9899
}
99100
}
100101
};
101102

102-
// You must ensure that when invoking this you're located on the JS thread, or
103-
// have exclusive control of the JS VM context. If you cannot ensure this, use
104-
// AsyncCallback instead.
103+
// Invoke only on the JavaScript thread. Use AsyncCallback from other threads.
105104
template <typename R, typename... Args>
106105
class SyncCallback<R(Args...)> {
107106
public:
108107
SyncCallback(jsi::Runtime &rt, jsi::Function function, std::shared_ptr<CallInvoker> jsInvoker)
109-
: wrapper_(CallbackWrapper::createWeak(std::move(function), rt, std::move(jsInvoker)))
108+
: runtime_(&rt), function_(rt, std::move(function), std::move(jsInvoker))
110109
{
111110
}
112111

113-
// Disallow copying, as we can no longer safely destroy the callback
114-
// from the destructor if there's multiple copies
115112
SyncCallback(const SyncCallback &) = delete;
116113
SyncCallback &operator=(const SyncCallback &) = delete;
114+
SyncCallback(SyncCallback &&) noexcept = default;
115+
SyncCallback &operator=(SyncCallback &&) noexcept = default;
116+
~SyncCallback() = default;
117117

118-
// Allow move
119-
SyncCallback(SyncCallback &&other) noexcept : wrapper_(std::move(other.wrapper_)) {}
120-
121-
SyncCallback &operator=(SyncCallback &&other) noexcept
118+
R operator()(Args... args) const
122119
{
123-
wrapper_ = std::move(other.wrapper_);
124-
return *this;
120+
return call(std::forward<Args>(args)...);
125121
}
126122

127-
~SyncCallback()
123+
R call(Args... args) const
128124
{
129-
if (auto wrapper = wrapper_.lock()) {
130-
wrapper->destroy();
125+
if (function_.expired()) {
126+
return invalidated();
131127
}
128+
return callWithRuntime(*runtime_, std::forward<Args>(args)...);
132129
}
133130

134-
R operator()(Args... args) const
131+
private:
132+
friend AsyncCallback<Args...>;
133+
friend Bridging<SyncCallback>;
134+
135+
static R invalidated()
135136
{
136-
return call(std::forward<Args>(args)...);
137+
if constexpr (std::is_void_v<R>) {
138+
return;
139+
} else {
140+
throw std::runtime_error("Failed to call invalidated sync callback");
141+
}
137142
}
138143

139-
R call(Args... args) const
144+
R callWithRuntime(jsi::Runtime &rt, Args... args) const
140145
{
141-
auto wrapper = wrapper_.lock();
142-
143-
// If the wrapper has been deallocated, we can no longer provide a return
144-
// value consistently, so our only option is to throw
145-
if (!wrapper) {
146-
if constexpr (std::is_void_v<R>) {
147-
return;
148-
} else {
149-
throw std::runtime_error("Failed to call invalidated sync callback");
150-
}
146+
auto function = function_.lockOnJS(rt);
147+
if (!function) {
148+
return invalidated();
151149
}
152150

153-
auto &callback = wrapper->callback();
154-
auto &rt = wrapper->runtime();
155-
auto jsInvoker = wrapper->jsInvokerPtr();
151+
// JavaScript can destroy this native callback during the call; use only
152+
// the retained value and local runtime/invoker references afterward.
153+
auto &callback = function->value;
154+
auto jsInvoker = function->jsInvoker;
156155

157156
if constexpr (std::is_void_v<R>) {
158157
callback.call(rt, bridging::toJs(rt, std::forward<Args>(args), jsInvoker)...);
@@ -162,23 +161,21 @@ class SyncCallback<R(Args...)> {
162161
}
163162
}
164163

165-
private:
166-
friend AsyncCallback<Args...>;
167-
friend Bridging<SyncCallback>;
168-
169-
R apply(std::tuple<Args...> &&args) const
164+
R apply(jsi::Runtime &rt, std::tuple<Args...> &&args) const
170165
{
171-
return apply(std::move(args), std::index_sequence_for<Args...>{});
166+
return apply(rt, std::move(args), std::index_sequence_for<Args...>{});
172167
}
173168

174169
template <size_t... Index>
175-
R apply(std::tuple<Args...> &&args, std::index_sequence<Index...> /*unused*/) const
170+
R apply(jsi::Runtime &rt, std::tuple<Args...> &&args, std::index_sequence<Index...> /*unused*/) const
176171
{
177-
return call(std::move(std::get<Index>(args))...);
172+
return callWithRuntime(rt, std::move(std::get<Index>(args))...);
178173
}
179174

180-
// Held weakly so lifetime is managed by LongLivedObjectCollection.
181-
std::weak_ptr<CallbackWrapper> wrapper_;
175+
// Only the legacy synchronous call needs to recover its construction runtime.
176+
// Async calls use the runtime supplied by the executor instead.
177+
jsi::Runtime *runtime_;
178+
LongLivedObjectHandle<jsi::Function> function_;
182179
};
183180

184181
template <typename... Args>
@@ -191,7 +188,7 @@ struct Bridging<AsyncCallback<Args...>> {
191188

192189
static jsi::Function toJs(jsi::Runtime &rt, const AsyncCallback<Args...> &value)
193190
{
194-
return value.callback_->function_.getFunction(rt);
191+
return Bridging<SyncCallback<void(Args...)>>::toJs(rt, *value.callback_);
195192
}
196193
};
197194

@@ -205,7 +202,10 @@ struct Bridging<SyncCallback<R(Args...)>> {
205202

206203
static jsi::Function toJs(jsi::Runtime &rt, const SyncCallback<R(Args...)> &value)
207204
{
208-
return value.function_.getFunction(rt);
205+
if (auto function = value.function_.lockOnJS(rt)) {
206+
return function->value.getFunction(rt);
207+
}
208+
throw jsi::JSError(rt, "Failed to get invalidated callback");
209209
}
210210
};
211211

‎packages/react-native/ReactCommon/react/bridging/LongLivedObject.cpp‎

Lines changed: 40 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
*/
77

88
#include "LongLivedObject.h"
9+
10+
#include <ReactCommon/CallInvoker.h>
911
#include <unordered_map>
1012

1113
namespace facebook::react {
@@ -35,22 +37,31 @@ void LongLivedObjectCollection::add(std::shared_ptr<LongLivedObject> so) {
3537
}
3638

3739
void LongLivedObjectCollection::remove(const LongLivedObject* o) {
38-
std::scoped_lock lock(collectionMutex_);
39-
// Empty owner: lookup-only alias, with no ownership or control block.
40-
const std::shared_ptr<LongLivedObject> lookupKey{
41-
std::shared_ptr<LongLivedObject>{}, const_cast<LongLivedObject*>(o)};
40+
decltype(collection_)::node_type retired;
41+
{
42+
std::scoped_lock lock(collectionMutex_);
43+
// Empty owner: lookup-only alias, with no ownership or control block.
44+
const std::shared_ptr<LongLivedObject> lookupKey{
45+
std::shared_ptr<LongLivedObject>{},
46+
// Lookup never dereferences or mutates the object.
47+
// NOLINTNEXTLINE(cppcoreguidelines-pro-type-const-cast)
48+
const_cast<LongLivedObject*>(o)};
4249

43-
// A shared_ptr hashes and compares by the pointer it stores, so a
44-
// non-owning shared_ptr to `o` finds its entry without walking the set.
45-
auto entry = collection_.find(lookupKey);
46-
if (entry != collection_.end()) {
47-
collection_.erase(entry);
50+
// A shared_ptr hashes and compares by the pointer it stores, so a
51+
// non-owning shared_ptr to `o` finds its entry without walking the set.
52+
auto entry = collection_.find(lookupKey);
53+
if (entry != collection_.end()) {
54+
retired = collection_.extract(entry);
55+
}
4856
}
4957
}
5058

5159
void LongLivedObjectCollection::clear() {
52-
std::scoped_lock lock(collectionMutex_);
53-
collection_.clear();
60+
decltype(collection_) retired;
61+
{
62+
std::scoped_lock lock(collectionMutex_);
63+
retired.swap(collection_);
64+
}
5465
}
5566

5667
size_t LongLivedObjectCollection::size() const {
@@ -64,4 +75,22 @@ void LongLivedObject::allowRelease() {
6475
LongLivedObjectCollection::get(runtime_).remove(this);
6576
}
6677

78+
void detail::releaseLongLivedObjectOnJS(
79+
std::weak_ptr<LongLivedObject> object,
80+
std::weak_ptr<CallInvoker> jsInvoker) noexcept {
81+
if (object.expired()) {
82+
return;
83+
}
84+
if (auto invoker = jsInvoker.lock()) {
85+
invoker->invokeAsync([object = std::move(object)](jsi::Runtime&) {
86+
if (auto locked = object.lock()) {
87+
locked->allowRelease();
88+
}
89+
});
90+
}
91+
// If dispatch is dropped, runtime cleanup releases the collection. Never
92+
// fall back to acquiring or releasing a strong reference on the caller
93+
// thread.
94+
}
95+
6796
} // namespace facebook::react

0 commit comments

Comments
 (0)