Skip to content

Commit f851a9f

Browse files
Destroy preallocated views that the pull model never mounted (#58890)
Summary: With `enableMountingCoordinatorPullModelAndroid`, the UI thread pulls mount transactions instead of the JS thread pushing every commit. When one commit mounts a subtree and the next commit unmounts it before the UI thread pulls, the pulled transaction contains neither the Create nor the Delete for it. The views preallocated for that subtree stay in `SurfaceMountingManager`, and their tags stay in `FabricMountingManager`'s allocated view registry. The cleanup that destroys never-mounted preallocated views does not run either, because `ShadowNodeFamily` already marked the family as mounted at commit time. In a test app that repeatedly mounts and unmounts 1200 views, nearly all of them leaked as Java views and registry entries. This PR changes how preallocated views are cleaned up when the pull model is enabled: - The allocated view registry records whether each view is only preallocated, or was created by a mount (a Create or the surface root). - `ShadowNodeFamily` gets `onFamilyDestroyed`, whose callback runs for every destroyed family, and a `hasBeenMounted()` getter. `onUnmountedFamilyDestroyed` is deprecated. It keeps its behaviour by wrapping `onFamilyDestroyed` and skipping mounted families. - `destroyUnmountedShadowNode` destroys only views that are still preallocated-only. A view created by a mount is left to its Delete, which can be pulled after the family is destroyed. - Each queued preallocation holds a weak reference to its family. The drain skips views whose family is already destroyed, so it does not preallocate a view that nothing would clean up. `FabricMountingManager::preallocateShadowView` is removed, because the drain now registers and preallocates its views directly and no production code called it. ## Changelog: [ANDROID] [FIXED] - Fix preallocated views leaking when `enableMountingCoordinatorPullModelAndroid` is enabled Test Plan: - `ShadowNodeFamilyTest`: `onFamilyDestroyed` runs for both mounted and unmounted families and reports `hasBeenMounted()` correctly, and the deprecated `onUnmountedFamilyDestroyed` still skips mounted families. There is no open-source target for these gtests, so I built them with the NDK and ran them on an Android emulator. - `FabricMountingManagerInstrumentationTest` has new cases for the following. It has no Gradle target in the open-source repository, so I ran it on an Android emulator through a local androidTest source set that is not part of this PR. - the drain skipping a view whose family was destroyed (pull model); - a committed family destroying its preallocated view (pull model); - a committed family keeping a view that a mount created (pull model). With the native changes reverted, the cases for the destroyed committed family and the drain skip fail. - Release build of a test app on an Android emulator. The app mounts and unmounts a subtree of 1200 views in a loop, then idles. View counts at idle: | Configuration | Java views before → after | Registry entries before → after | | --- | --- | --- | | Pull model | 1197 → 3 | 1197 → 3 | | Pull model + Props 2.0 | 1203 → 3 | 1203 → 3 | There were no crashes or new soft exceptions. - `yarn cxx-api-validate`, `yarn format-check-cpp` and `yarn format-check-kotlin` pass. Differential Revision: D123648108 Pulled By: bartlomiejbloniarz
1 parent fdec9e6 commit f851a9f

23 files changed

Lines changed: 405 additions & 78 deletions

‎packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.cpp‎

Lines changed: 71 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,11 @@ FabricMountingManager::~FabricMountingManager() {
4747
void FabricMountingManager::onSurfaceStart(SurfaceId surfaceId) {
4848
std::lock_guard lock(allocatedViewsMutex_);
4949
allocatedViewRegistry_.emplace(
50-
surfaceId, std::unordered_map<Tag, Props::Shared>{{surfaceId, nullptr}});
50+
surfaceId,
51+
std::unordered_map<Tag, AllocatedView>{
52+
{surfaceId,
53+
{.preallocatedProps = nullptr,
54+
.state = AllocatedViewState::Mounted}}});
5155
}
5256

5357
void FabricMountingManager::onSurfaceStop(SurfaceId surfaceId) {
@@ -617,9 +621,9 @@ void FabricMountingManager::executeMount(
617621
std::lock_guard allocatedViewsLock(allocatedViewsMutex_);
618622

619623
auto allocatedViewsIterator = allocatedViewRegistry_.find(surfaceId);
620-
auto defaultAllocatedViews = std::unordered_map<Tag, Props::Shared>{};
624+
auto defaultAllocatedViews = std::unordered_map<Tag, AllocatedView>{};
621625
// Do not remove `defaultAllocatedViews` or initialize
622-
// `std::unordered_map<Tag, Props::Shared>{}` inline in below ternary
626+
// `std::unordered_map<Tag, AllocatedView>{}` inline in below ternary
623627
// expression - if falsy operand is a value type, the compiler will decide
624628
// the expression to be a value type, an unnecessary (sometimes expensive)
625629
// copy will happen as a result.
@@ -642,13 +646,18 @@ void FabricMountingManager::executeMount(
642646
bool isVirtual = mutation.mutatedViewIsVirtual();
643647
switch (mutationType) {
644648
case ShadowViewMutation::Create: {
645-
bool shouldCreateView =
646-
!allocatedViewTags.contains(newChildShadowView.tag);
649+
auto [allocatedView, shouldCreateView] =
650+
allocatedViewTags.try_emplace(
651+
newChildShadowView.tag,
652+
AllocatedView{
653+
.preallocatedProps = nullptr,
654+
.state = AllocatedViewState::Mounted});
647655

648656
if (shouldCreateView) {
649657
cppCommonMountItems.push_back(
650658
CppMountItem::CreateMountItem(newChildShadowView));
651-
allocatedViewTags.emplace(newChildShadowView.tag, nullptr);
659+
} else {
660+
allocatedView->second.state = AllocatedViewState::Mounted;
652661
}
653662
break;
654663
}
@@ -767,7 +776,8 @@ void FabricMountingManager::executeMount(
767776
.push_back(
768777
CppMountItem::UpdatePropsMountItem(
769778
{}, newChildShadowView));
770-
} else if (auto& preallocatedProps = allocatedView->second;
779+
} else if (auto& preallocatedProps =
780+
allocatedView->second.preallocatedProps;
771781
preallocatedProps != nullptr) {
772782
if (preallocatedProps != newChildShadowView.props) {
773783
auto preallocatedShadowView = newChildShadowView;
@@ -1068,22 +1078,55 @@ void FabricMountingManager::executeMount(
10681078
}
10691079

10701080
void FabricMountingManager::drainPreallocateViewsQueue() {
1081+
std::vector<QueuedView> queuedViews;
10711082
std::vector<ShadowView> shadowViews;
10721083

10731084
{
10741085
std::lock_guard lock(preallocateMutex_);
1075-
std::swap(shadowViews, preallocatedViewsQueue_);
1086+
std::swap(queuedViews, preallocatedViewsQueue_);
1087+
}
1088+
1089+
bool pullModel =
1090+
ReactNativeFeatureFlags::enableMountingCoordinatorPullModelAndroid();
1091+
{
1092+
std::lock_guard allocatedViewsLock(allocatedViewsMutex_);
1093+
for (auto& [shadowView, family] : queuedViews) {
1094+
if (pullModel && family.expired()) {
1095+
continue;
1096+
}
1097+
auto allocatedViewsIterator =
1098+
allocatedViewRegistry_.find(shadowView.surfaceId);
1099+
if (allocatedViewsIterator != allocatedViewRegistry_.end() &&
1100+
allocatedViewsIterator->second
1101+
.emplace(
1102+
shadowView.tag,
1103+
AllocatedView{
1104+
.preallocatedProps =
1105+
shouldDiffInsertPropsAgainstPreallocatedProps()
1106+
? shadowView.props
1107+
: nullptr,
1108+
.state = AllocatedViewState::Preallocated})
1109+
.second) {
1110+
shadowViews.push_back(std::move(shadowView));
1111+
}
1112+
}
10761113
}
10771114

10781115
for (const auto& shadowView : shadowViews) {
1079-
preallocateShadowView(shadowView);
1116+
preallocateRegisteredView(shadowView);
10801117
}
10811118
}
10821119

10831120
void FabricMountingManager::destroyUnmountedShadowNode(
10841121
const ShadowNodeFamily& family) {
10851122
auto tag = family.getTag();
10861123
auto surfaceId = family.getSurfaceId();
1124+
bool pullModel =
1125+
ReactNativeFeatureFlags::enableMountingCoordinatorPullModelAndroid();
1126+
1127+
if (!pullModel && family.hasBeenMounted()) {
1128+
return;
1129+
}
10871130

10881131
// Remove from allocatedViewRegistry so that executeMount does not skip
10891132
// the Create mount item for this tag. Without this, if the view was
@@ -1094,7 +1137,20 @@ void FabricMountingManager::destroyUnmountedShadowNode(
10941137
{
10951138
std::lock_guard allocatedViewsLock(allocatedViewsMutex_);
10961139
auto allocatedViewsIterator = allocatedViewRegistry_.find(surfaceId);
1097-
if (allocatedViewsIterator != allocatedViewRegistry_.end()) {
1140+
if (pullModel) {
1141+
if (allocatedViewsIterator == allocatedViewRegistry_.end()) {
1142+
return;
1143+
}
1144+
auto allocatedView = allocatedViewsIterator->second.find(tag);
1145+
// In the pull model, a committed family can have a preallocated view that
1146+
// no mount created (its Create and Delete were never pulled), and a
1147+
// mounted view's family can be destroyed before its Delete is pulled.
1148+
if (allocatedView == allocatedViewsIterator->second.end() ||
1149+
allocatedView->second.state != AllocatedViewState::Preallocated) {
1150+
return;
1151+
}
1152+
allocatedViewsIterator->second.erase(allocatedView);
1153+
} else if (allocatedViewsIterator != allocatedViewRegistry_.end()) {
10981154
allocatedViewsIterator->second.erase(tag);
10991155
}
11001156
}
@@ -1125,29 +1181,15 @@ void FabricMountingManager::maybePreallocateShadowNode(
11251181

11261182
{
11271183
std::lock_guard lock(preallocateMutex_);
1128-
preallocatedViewsQueue_.push_back(std::move(shadowView));
1184+
preallocatedViewsQueue_.push_back(
1185+
{.shadowView = std::move(shadowView),
1186+
.family = shadowNode.getFamilyShared()});
11291187
}
11301188
}
11311189

1132-
void FabricMountingManager::preallocateShadowView(
1190+
void FabricMountingManager::preallocateRegisteredView(
11331191
const ShadowView& shadowView) {
1134-
TraceSection section("FabricMountingManager::preallocateShadowView");
1135-
1136-
{
1137-
std::lock_guard lock(allocatedViewsMutex_);
1138-
auto allocatedViewsIterator =
1139-
allocatedViewRegistry_.find(shadowView.surfaceId);
1140-
if (allocatedViewsIterator == allocatedViewRegistry_.end()) {
1141-
return;
1142-
}
1143-
const auto [_, inserted] = allocatedViewsIterator->second.emplace(
1144-
shadowView.tag,
1145-
shouldDiffInsertPropsAgainstPreallocatedProps() ? shadowView.props
1146-
: nullptr);
1147-
if (!inserted) {
1148-
return;
1149-
}
1150-
}
1192+
TraceSection section("FabricMountingManager::preallocateRegisteredView");
11511193

11521194
bool isLayoutableShadowNode = shadowView.layoutMetrics != EmptyLayoutMetrics;
11531195

‎packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.h‎

Lines changed: 26 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -7,19 +7,20 @@
77

88
#pragma once
99

10+
#include <memory>
1011
#include <mutex>
1112
#include <unordered_map>
1213

1314
#include <React/RendererCore.h>
1415
#include <fbjni/fbjni.h>
1516
#include <react/fabric/JFabricUIManager.h>
17+
#include <react/renderer/mounting/ShadowView.h>
1618

1719
namespace facebook::react {
1820

1921
struct AnimatedProps;
2022

2123
class MountingTransaction;
22-
struct ShadowView;
2324

2425
class FabricMountingManager final {
2526
public:
@@ -36,18 +37,13 @@ class FabricMountingManager final {
3637
void destroyUnmountedShadowNode(const ShadowNodeFamily &family);
3738

3839
/*
39-
* Drains preallocatedViewsQueue_ by calling preallocateShadowView on each
40-
* item in the queue. Can be called by any thread.
40+
* Drains preallocatedViewsQueue_: preallocates each queued view on the Java
41+
* side and registers its tag in allocatedViewRegistry_ so that executeMount
42+
* skips the redundant Create mount item for this tag. In the pull model, it
43+
* skips the views whose family was destroyed. Can be called by any thread.
4144
*/
4245
void drainPreallocateViewsQueue();
4346

44-
/*
45-
* Preallocates a view on the Java side and registers the tag in
46-
* allocatedViewRegistry_ so that executeMount skips the redundant Create
47-
* mount item for this tag.
48-
*/
49-
void preallocateShadowView(const ShadowView &shadowView);
50-
5147
/*
5248
* Returns true if the given tag is registered in allocatedViewRegistry_
5349
* for the given surface. A registered tag means executeMount will skip
@@ -98,6 +94,8 @@ class FabricMountingManager final {
9894
private:
9995
bool isOnMainThread();
10096

97+
void preallocateRegisteredView(const ShadowView &shadowView);
98+
10199
jni::global_ref<JFabricUIManager::javaobject> javaUIManager_;
102100

103101
std::recursive_mutex commitMutex_;
@@ -107,17 +105,28 @@ class FabricMountingManager final {
107105
*/
108106
std::mutex preallocateMutex_;
109107

108+
struct QueuedView {
109+
ShadowView shadowView;
110+
std::weak_ptr<const ShadowNodeFamily> family;
111+
};
112+
110113
/*
111114
* A queue of views to be preallocated on the Java side.
112115
*/
113-
std::vector<ShadowView> preallocatedViewsQueue_{};
116+
std::vector<QueuedView> preallocatedViewsQueue_{};
114117

115-
/*
116-
* Allocated tags per surface. With enablePreallocatedPropsDiffOnInsertAndroid
117-
* a preallocated tag maps to the props it was preallocated with until its
118-
* first Insert; every other tag maps to nullptr.
119-
*/
120-
std::unordered_map<SurfaceId, std::unordered_map<Tag, Props::Shared>> allocatedViewRegistry_{};
118+
enum class AllocatedViewState { Preallocated, Mounted };
119+
120+
struct AllocatedView {
121+
/*
122+
* With enablePreallocatedPropsDiffOnInsertAndroid, the props the view was
123+
* preallocated with, until its first Insert. nullptr otherwise.
124+
*/
125+
Props::Shared preallocatedProps;
126+
AllocatedViewState state;
127+
};
128+
129+
std::unordered_map<SurfaceId, std::unordered_map<Tag, AllocatedView>> allocatedViewRegistry_{};
121130
std::recursive_mutex allocatedViewsMutex_;
122131
};
123132

‎packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.cpp‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -807,11 +807,11 @@ void FabricUIManagerBinding::schedulerDidRequestPreliminaryViewAllocation(
807807
return;
808808
}
809809
mountingManager->maybePreallocateShadowNode(shadowNode);
810-
// Only the Views of ShadowNode that were pre-allocated (forms views) needs
811-
// to be destroyed if the ShadowNode is destroyed but it was never mounted
812-
// on the screen.
810+
// Only the Views of ShadowNode that were pre-allocated (forms views) need
811+
// to be destroyed if the ShadowNode is destroyed and no mount created its
812+
// View.
813813
if (shadowNode.getTraits().check(ShadowNodeTraits::Trait::FormsView)) {
814-
shadowNode.getFamilyShared()->onUnmountedFamilyDestroyed(
814+
shadowNode.getFamilyShared()->onFamilyDestroyed(
815815
[weakMountingManager =
816816
std::weak_ptr(mountingManager)](const ShadowNodeFamily& family) {
817817
if (auto mountingManager = weakMountingManager.lock()) {

‎packages/react-native/ReactAndroid/src/main/jni/react/fabric/test/FabricMountingManagerTest.cpp‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -60,10 +60,9 @@ class FabricMountingManagerTest : public ::testing::Test {
6060
* and to short-circuit a redundant Create mount item for the root view.
6161
*
6262
* Bug this catches: if a refactor stops registering the root surface tag
63-
* (e.g. switches to `unordered_set<Tag>{}` instead of
64-
* `unordered_set<Tag>({surfaceId})`), the root view would be re-created on
65-
* every commit. Without this assertion, that regression would only surface
66-
* via end-to-end Java tests.
63+
* (e.g. starts each surface with an empty registry), the root view would be
64+
* re-created on every commit. Without this assertion, that regression would
65+
* only surface via end-to-end Java tests.
6766
*/
6867
TEST_F(
6968
FabricMountingManagerTest,
@@ -79,7 +78,7 @@ TEST_F(
7978
/*
8079
* Verifies that `isViewAllocated` returns false for a surface that has not
8180
* been started. The contract is "false when the surface is unknown", not
82-
* "throw" or "abort"; `executeMount` and `preallocateShadowView` both rely
81+
* "throw" or "abort"; `executeMount` and `drainPreallocateViewsQueue` both rely
8382
* on this to gracefully no-op when a surface has been stopped concurrently.
8483
*/
8584
TEST_F(
@@ -98,7 +97,7 @@ TEST_F(
9897
* must return false for *any* tag on that surface, including the surface
9998
* tag itself (which `onSurfaceStart` had inserted).
10099
*
101-
* Bug this catches: if `onSurfaceStop` only `.clear()`-ed the inner set
100+
* Bug this catches: if `onSurfaceStop` only `.clear()`-ed the inner map
102101
* instead of erasing the outer map entry, `executeMount`'s "surface was
103102
* stopped" log would never fire, masking commit-after-stop bugs.
104103
*/
@@ -122,7 +121,7 @@ TEST_F(
122121
* the same process.
123122
*
124123
* Bug this catches: if a future change accidentally shared the
125-
* `unordered_set<Tag>` across surfaces (e.g. via a misplaced static), tags
124+
* per-surface registry across surfaces (e.g. via a misplaced static), tags
126125
* from one surface would falsely appear as allocated on another, and
127126
* stopping any surface would purge unrelated surfaces' registries.
128127
*/

0 commit comments

Comments
 (0)