Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,37 @@ void EventQueueProcessor::flushEvents(
}
}

// RAII guard for the matching release() pass. If event dispatch throws (a JS
// exception from eventPipe_, or eventPipeConclusion_), the previous code
// skipped the release loop entirely, leaking JSI strong references. The
// guard destructor runs on every exit path. No DispatchMutex needed for
// release: we hold a strong pointer to each EventTarget via `events`, and
// release() only touches runtime-thread-confined state.
class EventTargetReleaseGuard {
public:
EventTargetReleaseGuard(
jsi::Runtime& runtime,
const std::vector<RawEvent>& events)
: runtime_(runtime), events_(events) {}

EventTargetReleaseGuard(const EventTargetReleaseGuard&) = delete;
EventTargetReleaseGuard(EventTargetReleaseGuard&&) = delete;
EventTargetReleaseGuard& operator=(const EventTargetReleaseGuard&) = delete;
EventTargetReleaseGuard& operator=(EventTargetReleaseGuard&&) = delete;

~EventTargetReleaseGuard() {
for (const auto& event : events_) {
if (event.eventTarget) {
event.eventTarget->release(runtime_);
}
}
}

private:
jsi::Runtime& runtime_;
const std::vector<RawEvent>& events_;
} releaseGuard{runtime, events};

for (const auto& event : events) {
auto reactPriority = ReactEventPriority::Default;

Expand Down Expand Up @@ -111,15 +142,7 @@ void EventQueueProcessor::flushEvents(
// We only run the "Conclusion" once per event group when batched.
eventPipeConclusion_(runtime);

// No need to lock `EventEmitter::DispatchMutex()` here.
// The mutex protects from a situation when the `instanceHandle` can be
// deallocated during accessing, but that's impossible at this point because
// we have a strong pointer to it.
for (const auto& event : events) {
if (event.eventTarget) {
event.eventTarget->release(runtime);
}
}
// EventTarget release happens in ~EventTargetReleaseGuard above.
}

void EventQueueProcessor::flushStateUpdates(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,12 @@ void EventTarget::release(jsi::Runtime& /*runtime*/) {
// It takes it only to ensure thread-safety (if the caller has the reference,
// we are on a proper thread).

if (--retainCount_ == 0) {
if (retainCount_ > 0) {
--retainCount_;
}
if (retainCount_ == 0) {
strongInstanceHandle_ = jsi::Value::null();
}

react_native_assert(retainCount_ >= 0);
}

jsi::Value EventTarget::getInstanceHandle(jsi::Runtime& runtime) const {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,13 @@
#include <react/renderer/core/EventPipe.h>
#include <react/renderer/core/EventQueueProcessor.h>
#include <react/renderer/core/EventTarget.h>
#include <react/renderer/core/InstanceHandle.h>
#include <react/renderer/core/ShadowNodeFamily.h>
#include <react/renderer/core/StatePipe.h>
#include <react/renderer/core/ValueFactoryEventPayload.h>

#include <memory>
#include <stdexcept>
#include <string_view>

namespace facebook::react {
Expand Down Expand Up @@ -157,4 +159,50 @@ TEST_F(EventQueueProcessorTest, alwaysDiscreteEvent) {
EXPECT_EQ(eventPriorities_[0], ReactEventPriority::Discrete);
}

TEST_F(EventQueueProcessorTest, releasesEventTargetsWhenDispatchThrows) {
// Set up an enabled EventTarget so that flushEvents() will retain a strong
// JSI reference to its instance handle.
auto object = jsi::Object(*runtime_);
auto instanceHandle = std::make_shared<InstanceHandle>(
*runtime_, jsi::Value(*runtime_, object), 1);
auto eventTarget =
std::make_shared<EventTarget>(std::move(instanceHandle), 41);
eventTarget->setEnabled(true);

// An event pipe that throws, simulating a JS exception during dispatch.
auto throwingEventPipe = [](jsi::Runtime& /*runtime*/,
const EventTarget* /*eventTarget*/,
const std::string& /*type*/,
ReactEventPriority /*priority*/,
const EventPayload& /*payload*/,
HighResTimeStamp /*eventTimestamp*/) {
throw std::runtime_error("dispatch failed");
};
auto dummyEventPipeConclusion = [](jsi::Runtime& /*runtime*/) {};
auto dummyStatePipe = [](const StateUpdate& /*stateUpdate*/) {};
auto mockEventLogger = std::make_shared<MockEventLogger>();

auto processor = EventQueueProcessor(
throwingEventPipe,
dummyEventPipeConclusion,
dummyStatePipe,
mockEventLogger);

EXPECT_THROW(
processor.flushEvents(
*runtime_,
{RawEvent(
"onThrow",
std::make_shared<ValueFactoryEventPayload>(dummyValueFactory_),
eventTarget,
{},
RawEvent::Category::Discrete)}),
std::runtime_error);

// The strong JSI reference acquired by retain() must have been released even
// though dispatch threw, so the instance handle is no longer reachable
// through the EventTarget.
EXPECT_TRUE(eventTarget->getInstanceHandle(*runtime_).isNull());
}

} // namespace facebook::react
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,29 @@ TEST(EventTargetTests, getInstanceHandle) {

EXPECT_TRUE(eventTarget.getInstanceHandle(*runtime).isNull());
}

TEST(EventTargetTests, releaseWhileDisabledDoesNotCorruptRetainCount) {
// retain() is a no-op while the target is disabled, but the matching
// release() still runs. release() must tolerate a zero retain count without
// underflowing, so that a later enable + retain still populates the strong
// instance handle.
auto runtime = facebook::hermes::makeHermesRuntime();
auto object = jsi::Object(*runtime);
auto instanceHandle = std::make_shared<InstanceHandle>(
*runtime, jsi::Value(*runtime, object), 1);

auto eventTarget = EventTarget(std::move(instanceHandle), 41);

// Disabled by default: retain is a no-op, release sees a zero count.
eventTarget.retain(*runtime);
eventTarget.release(*runtime);

// After enabling, a fresh retain must succeed in acquiring the handle.
eventTarget.setEnabled(true);
eventTarget.retain(*runtime);
EXPECT_FALSE(eventTarget.getInstanceHandle(*runtime).isNull());

// And the matching release must drop it again.
eventTarget.release(*runtime);
EXPECT_TRUE(eventTarget.getInstanceHandle(*runtime).isNull());
}
Loading