Skip to content

Fix callback and promise teardown races (#59670) #59670

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

Open
javache wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix callback and promise teardown races (#59670) #59670

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 4 additions & 1 deletion packages/react-native/ReactCommon/react/bridging/CallbackWrapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@ namespace facebook::react {

class CallInvoker;

// Helper for passing jsi::Function arg to other methods.
/**
* @deprecated Use LongLivedObjectHandle<jsi::Function> instead.
* Access and release this legacy wrapper only on the JavaScript thread.
*/
class CallbackWrapper : public LongLivedObject {
private:
CallbackWrapper(jsi::Function &&callback, jsi::Runtime &runtime, std::shared_ptr<CallInvoker> jsInvoker)
Expand Down
90 changes: 41 additions & 49 deletions packages/react-native/ReactCommon/react/bridging/Function.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#include <react/cxxstableapi/UmbrellaGuard.h>

#include <react/bridging/Base.h>
#include <react/bridging/CallbackWrapper.h>
#include <react/bridging/CallbackWrapper.h> // Retain the legacy transitive include.

#include <ReactCommon/CallInvoker.h>
#include <ReactCommon/SchedulerPriority.h>
Expand Down Expand Up @@ -61,16 +61,18 @@ class AsyncCallback {

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

auto &jsInvoker = wrapper->jsInvoker();
if (priority) {
jsInvoker.invokeAsync(*priority, std::move(fn));
jsInvoker->invokeAsync(*priority, std::move(fn));
} else {
jsInvoker.invokeAsync(std::move(fn));
jsInvoker->invokeAsync(std::move(fn));
}
}
}
Expand All @@ -79,57 +81,42 @@ class AsyncCallback {
std::optional<SchedulerPriority> priority,
std::function<void(jsi::Runtime &, jsi::Function &)> &&callImpl) const noexcept
{
if (auto wrapper = callback_->wrapper_.lock()) {
// Capture callback_ and not wrapper_. If callback_ is deallocated or the
// JSVM is shutdown before the async task is scheduled, the underlying
// function will have been deallocated.
auto fn = [callback = callback_, callImpl = std::move(callImpl)](jsi::Runtime &rt) {
if (auto wrapper2 = callback->wrapper_.lock()) {
callImpl(rt, wrapper2->callback());
auto callback = callback_;
if (auto jsInvoker = callback->function_.lockInvoker()) {
// Retain native ownership until this task is destroyed, without retaining
// a JavaScript value while the task is queued or discarded.
auto fn = [callback = std::move(callback), callImpl = std::move(callImpl)](jsi::Runtime &) {
if (!callback->function_.expired()) {
auto &rt = *callback->runtime_;
if (auto function = callback->function_.lockJSValue(rt)) {
callImpl(rt, function->value);
}
}
};

auto &jsInvoker = wrapper->jsInvoker();
if (priority) {
jsInvoker.invokeAsync(*priority, std::move(fn));
jsInvoker->invokeAsync(*priority, std::move(fn));
} else {
jsInvoker.invokeAsync(std::move(fn));
jsInvoker->invokeAsync(std::move(fn));
}
}
}
};

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

// Disallow copying, as we can no longer safely destroy the callback
// from the destructor if there's multiple copies
SyncCallback(const SyncCallback &) = delete;
SyncCallback &operator=(const SyncCallback &) = delete;

// Allow move
SyncCallback(SyncCallback &&other) noexcept : wrapper_(std::move(other.wrapper_)) {}

SyncCallback &operator=(SyncCallback &&other) noexcept
{
wrapper_ = std::move(other.wrapper_);
return *this;
}

~SyncCallback()
{
if (auto wrapper = wrapper_.lock()) {
wrapper->destroy();
}
}
SyncCallback(SyncCallback &&) noexcept = default;
SyncCallback &operator=(SyncCallback &&) noexcept = default;
~SyncCallback() = default;

R operator()(Args... args) const
{
Expand All @@ -138,21 +125,23 @@ class SyncCallback<R(Args...)> {

R call(Args... args) const
{
auto wrapper = wrapper_.lock();
auto jsInvoker = function_.lockInvoker();
auto function = jsInvoker ? function_.lockJSValue(*runtime_) : nullptr;

// If the wrapper has been deallocated, we can no longer provide a return
// value consistently, so our only option is to throw
if (!wrapper) {
// If the function has been deallocated, we can no longer provide a return
// value consistently, so our only option is to throw.
if (!function) {
if constexpr (std::is_void_v<R>) {
return;
} else {
throw std::runtime_error("Failed to call invalidated sync callback");
}
}

auto &callback = wrapper->callback();
auto &rt = wrapper->runtime();
auto jsInvoker = wrapper->jsInvokerPtr();
// JavaScript can destroy this native callback during the call; use only
// the retained value and local runtime/invoker references afterward.
auto &callback = function->value;
auto &rt = *runtime_;

if constexpr (std::is_void_v<R>) {
callback.call(rt, bridging::toJs(rt, std::forward<Args>(args), jsInvoker)...);
Expand All @@ -177,8 +166,8 @@ class SyncCallback<R(Args...)> {
return call(std::move(std::get<Index>(args))...);
}

// Held weakly so lifetime is managed by LongLivedObjectCollection.
std::weak_ptr<CallbackWrapper> wrapper_;
jsi::Runtime *runtime_;
LongLivedObjectHandle<jsi::Function> function_;
};

template <typename... Args>
Expand All @@ -191,7 +180,7 @@ struct Bridging<AsyncCallback<Args...>> {

static jsi::Function toJs(jsi::Runtime &rt, const AsyncCallback<Args...> &value)
{
return value.callback_->function_.getFunction(rt);
return Bridging<SyncCallback<void(Args...)>>::toJs(rt, *value.callback_);
}
};

Expand All @@ -205,7 +194,10 @@ struct Bridging<SyncCallback<R(Args...)>> {

static jsi::Function toJs(jsi::Runtime &rt, const SyncCallback<R(Args...)> &value)
{
return value.function_.getFunction(rt);
if (auto function = value.function_.lockJSValue(rt)) {
return function->value.getFunction(rt);
}
throw jsi::JSError(rt, "Failed to get invalidated callback");
}
};

Expand Down
51 changes: 40 additions & 11 deletions packages/react-native/ReactCommon/react/bridging/LongLivedObject.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
*/

#include "LongLivedObject.h"

#include <ReactCommon/CallInvoker.h>
#include <unordered_map>

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

void LongLivedObjectCollection::remove(const LongLivedObject* o) {
std::scoped_lock lock(collectionMutex_);
// Empty owner: lookup-only alias, with no ownership or control block.
const std::shared_ptr<LongLivedObject> lookupKey{
std::shared_ptr<LongLivedObject>{}, const_cast<LongLivedObject*>(o)};
decltype(collection_)::node_type retired;
{
std::scoped_lock lock(collectionMutex_);
// Empty owner: lookup-only alias, with no ownership or control block.
const std::shared_ptr<LongLivedObject> lookupKey{
std::shared_ptr<LongLivedObject>{},
// Lookup never dereferences or mutates the object.
// NOLINTNEXTLINE(cppcoreguidelines-pro-type-const-cast)
const_cast<LongLivedObject*>(o)};

// A shared_ptr hashes and compares by the pointer it stores, so a
// non-owning shared_ptr to `o` finds its entry without walking the set.
auto entry = collection_.find(lookupKey);
if (entry != collection_.end()) {
collection_.erase(entry);
// A shared_ptr hashes and compares by the pointer it stores, so a
// non-owning shared_ptr to `o` finds its entry without walking the set.
auto entry = collection_.find(lookupKey);
if (entry != collection_.end()) {
retired = collection_.extract(entry);
}
}
}

void LongLivedObjectCollection::clear() {
std::scoped_lock lock(collectionMutex_);
collection_.clear();
decltype(collection_) retired;
{
std::scoped_lock lock(collectionMutex_);
retired.swap(collection_);
}
}

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

void detail::releaseLongLivedObjectOnJS(
std::weak_ptr<LongLivedObject> object,
std::weak_ptr<CallInvoker> jsInvoker) noexcept {
if (object.expired()) {
return;
}
if (auto invoker = jsInvoker.lock()) {
invoker->invokeAsync([object = std::move(object)](jsi::Runtime&) {
if (auto locked = object.lock()) {
locked->allowRelease();
}
});
}
// If dispatch is dropped, runtime cleanup releases the collection. Never
// fall back to acquiring or releasing a strong reference on the caller
// thread.
}

} // namespace facebook::react
108 changes: 107 additions & 1 deletion packages/react-native/ReactCommon/react/bridging/LongLivedObject.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,23 @@
#include <jsi/jsi.h>
#include <memory>
#include <mutex>
#include <thread>
#include <unordered_set>
#include <utility>

namespace facebook::react {

class CallInvoker;

/**
* A simple wrapper class that can be registered to a collection that keep it
* alive for extended period of time. This object can be removed from the
* collection when needed.
*
* The subclass of this class must be created using std::make_shared<T>().
* After creation, add it to the `LongLivedObjectCollection`. When done with the
* object, call `allowRelease()` to reclaim its memory.
* object, call `allowRelease()` to reclaim its memory. Prefer
* `LongLivedObjectHandle` for automatic JavaScript-value ownership and release.
*
* When using LongLivedObject to keep JS values alive, ensure you only hold weak
* references to the object outside the JS thread to avoid accessing deallocated
Expand All @@ -41,6 +46,8 @@ class LongLivedObject {

/**
* A singleton, thread-safe, write-only collection for the `LongLivedObject`s.
* Clear with exclusive access to the still-live runtime during teardown, even
* when native handles outlive it or queued release tasks cannot execute.
*/
class LongLivedObjectCollection {
public:
Expand All @@ -60,4 +67,103 @@ class LongLivedObjectCollection {
mutable std::mutex collectionMutex_;
};

namespace detail {
void releaseLongLivedObjectOnJS(std::weak_ptr<LongLivedObject> object, std::weak_ptr<CallInvoker> jsInvoker) noexcept;

// A derived weak pointer can transiently acquire strong ownership when converted.
template <typename T>
void releaseLongLivedObjectOnJS(std::weak_ptr<T>, std::weak_ptr<CallInvoker>) = delete;
} // namespace detail

/**
* Owns a collection registration without retaining its JavaScript value on a
* native thread. Construct and lock on the JavaScript thread; move and destroy
* on any thread. The runtime host owns the call invoker; this handle retains it
* weakly. The collection must be cleared before its runtime is destroyed.
*/
template <typename T>
class LongLivedObjectHandle {
public:
// All strong references to this holder, including copies, must stay on the JavaScript thread.
struct Holder : LongLivedObject {
Holder(jsi::Runtime &runtime, T value) : LongLivedObject(runtime), value(std::move(value)) {}

T value;
};

LongLivedObjectHandle() = default;

LongLivedObjectHandle(jsi::Runtime &runtime, T value, std::shared_ptr<CallInvoker> jsInvoker)
{
if (!jsInvoker) {
throw jsi::JSError(runtime, "LongLivedObjectHandle requires a call invoker");
}
jsThread_ = std::this_thread::get_id();
jsInvoker_ = jsInvoker;
auto holder = std::make_shared<Holder>(runtime, std::move(value));
LongLivedObjectCollection::get(runtime).add(holder);
holder_ = holder;
}

LongLivedObjectHandle(const LongLivedObjectHandle &) = delete;
LongLivedObjectHandle &operator=(const LongLivedObjectHandle &) = delete;
LongLivedObjectHandle(LongLivedObjectHandle &&) noexcept = default;

LongLivedObjectHandle &operator=(LongLivedObjectHandle &&other) noexcept
{
if (this != &other) {
reset();
holder_ = std::move(other.holder_);
jsInvoker_ = std::move(other.jsInvoker_);
jsThread_ = std::exchange(other.jsThread_, {});
}
return *this;
}

~LongLivedObjectHandle()
{
reset();
}

bool expired() const noexcept
{
return holder_.expired();
}

std::shared_ptr<CallInvoker> lockInvoker() const noexcept
{
return holder_.expired() ? nullptr : jsInvoker_.lock();
}

// Require this handle's runtime as proof that the caller has access to it;
// runtime identity is not checked. Keep the returned strong reference on the
// JavaScript thread, including its destruction.
std::shared_ptr<Holder> lockJSValue(jsi::Runtime & /*runtime*/) const noexcept
{
return std::static_pointer_cast<Holder>(holder_.lock());
}

void reset() noexcept
{
auto holder = std::exchange(holder_, {});
auto jsInvoker = std::exchange(jsInvoker_, {});
auto jsThread = std::exchange(jsThread_, {});
if (std::this_thread::get_id() == jsThread) {
if (auto locked = holder.lock()) {
locked->allowRelease();
}
} else {
detail::releaseLongLivedObjectOnJS(std::move(holder), std::move(jsInvoker));
}
}

private:
// Converting weak pointers can promote the object; keep the helper's base type
// so off-thread release only moves weak references.
std::weak_ptr<LongLivedObject> holder_;
std::weak_ptr<CallInvoker> jsInvoker_;
// Cache affinity without promoting the JavaScript-value holder on a native thread.
std::thread::id jsThread_;
};

} // namespace facebook::react
Loading
Loading