Improve thread safety of MountingCoordinator (#43503)

Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/43503

Changelog: [internal]

## Context

In the experiments to enable mount hooks on Android we saw some crashes that we couldn't reproduce or pinpoint accurately.

We ran another experiment excluding part of the code in one of the variants, and we found that the problem was in this block (only crashes when `skipMountHookNotifications` is false):
https://github.com/facebook/react-native/blob/121b26184acbb77ff4f2360647cb322ff560b145/packages/react-native/ReactCommon/react/renderer/uimanager/UIManager.cpp#L726-L734

Looking more closely at the mounting coordinator code, I realized that some of the methods are not thread-safe, which is likely causing these issues.

~~We're probably only seeing these issues on Android because we have a push model there (we call `pullTransaction` from whatever thread we're committing to, JS thread or Fabric background thread) whereas in the rest of platforms we have a pull model and we always access call `pullTransaction` from the main thread, as we do to report mounts.~~

This is probably fine because both cases are protected by a mutex when accessing through `ShadowTreeRegistry::visit`. But there's a case that doesn't go through it that could be causing the issues: prerendering:
https://github.com/facebook/react-native/blob/121b26184acbb77ff4f2360647cb322ff560b145/packages/react-native/ReactCommon/react/renderer/scheduler/SurfaceHandler.cpp#L289

## Changes

1) Make `getBaseRevision` return a copy of the revision rather than a reference.
2) Make all methods that access `baseRevision_` and `lastRevision_` thread-safe.

Reviewed By: javache

Differential Revision: D54945100

fbshipit-source-id: d8b211137d0eac02a5814cc6c376c22733290eab
This commit is contained in:
Rubén Norte
2024-03-18 06:15:52 -07:00
committed by Facebook GitHub Bot
parent 87b73a0bad
commit ebe26c198e
2 changed files with 8 additions and 2 deletions
@@ -68,10 +68,12 @@ bool MountingCoordinator::waitForTransaction(
void MountingCoordinator::updateBaseRevision(
const ShadowTreeRevision& baseRevision) const {
std::scoped_lock lock(mutex_);
baseRevision_ = baseRevision;
}
void MountingCoordinator::resetLatestRevision() const {
std::scoped_lock lock(mutex_);
lastRevision_.reset();
}
@@ -184,6 +186,7 @@ std::optional<MountingTransaction> MountingCoordinator::pullTransaction()
}
bool MountingCoordinator::hasPendingTransactions() const {
std::scoped_lock lock(mutex_);
return lastRevision_.has_value();
}
@@ -191,7 +194,8 @@ const TelemetryController& MountingCoordinator::getTelemetryController() const {
return telemetryController_;
}
const ShadowTreeRevision& MountingCoordinator::getBaseRevision() const {
ShadowTreeRevision MountingCoordinator::getBaseRevision() const {
std::scoped_lock lock(mutex_);
return baseRevision_;
}
@@ -79,7 +79,7 @@ class MountingCoordinator final {
const TelemetryController& getTelemetryController() const;
const ShadowTreeRevision& getBaseRevision() const;
ShadowTreeRevision getBaseRevision() const;
/*
* Methods from this section are meant to be used by
@@ -112,6 +112,8 @@ class MountingCoordinator final {
private:
const SurfaceId surfaceId_;
// Protects access to `baseRevision_`, `lastRevision_` and
// `mountingOverrideDelegate_`.
mutable std::mutex mutex_;
mutable ShadowTreeRevision baseRevision_;
mutable std::optional<ShadowTreeRevision> lastRevision_{};