Skip to content

Commit 35e5f80

Browse files
rubennortefacebook-github-bot
authored andcommitted
[skip ci] Improve thread safety of MountingCoordinator (facebook#43503)
Summary: 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. Differential Revision: D54945100
1 parent 14a7202 commit 35e5f80

2 files changed

Lines changed: 8 additions & 2 deletions

File tree

packages/react-native/ReactCommon/react/renderer/mounting/MountingCoordinator.cpp

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,10 +68,12 @@ bool MountingCoordinator::waitForTransaction(
6868

6969
void MountingCoordinator::updateBaseRevision(
7070
const ShadowTreeRevision& baseRevision) const {
71+
std::scoped_lock lock(mutex_);
7172
baseRevision_ = baseRevision;
7273
}
7374

7475
void MountingCoordinator::resetLatestRevision() const {
76+
std::scoped_lock lock(mutex_);
7577
lastRevision_.reset();
7678
}
7779

@@ -184,14 +186,16 @@ std::optional<MountingTransaction> MountingCoordinator::pullTransaction()
184186
}
185187

186188
bool MountingCoordinator::hasPendingTransactions() const {
189+
std::scoped_lock lock(mutex_);
187190
return lastRevision_.has_value();
188191
}
189192

190193
const TelemetryController& MountingCoordinator::getTelemetryController() const {
191194
return telemetryController_;
192195
}
193196

194-
const ShadowTreeRevision& MountingCoordinator::getBaseRevision() const {
197+
ShadowTreeRevision MountingCoordinator::getBaseRevision() const {
198+
std::scoped_lock lock(mutex_);
195199
return baseRevision_;
196200
}
197201

packages/react-native/ReactCommon/react/renderer/mounting/MountingCoordinator.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ class MountingCoordinator final {
7979

8080
const TelemetryController& getTelemetryController() const;
8181

82-
const ShadowTreeRevision& getBaseRevision() const;
82+
ShadowTreeRevision getBaseRevision() const;
8383

8484
/*
8585
* Methods from this section are meant to be used by
@@ -112,6 +112,8 @@ class MountingCoordinator final {
112112
private:
113113
const SurfaceId surfaceId_;
114114

115+
// Protects access to `baseRevision_`, `lastRevision_` and
116+
// `mountingOverrideDelegate_`.
115117
mutable std::mutex mutex_;
116118
mutable ShadowTreeRevision baseRevision_;
117119
mutable std::optional<ShadowTreeRevision> lastRevision_{};

0 commit comments

Comments
 (0)