Skip to content

Commit b703a10

Browse files
Pull model: mount the commits of starting and stopping surfaces (#59030)
Summary: With `enableMountingCoordinatorPullModelAndroid`, a commit tells Java only its surface id. At the next mount dispatch, `FabricUIManagerBinding::pullAndExecuteTransaction` looks the surface's `MountingCoordinator` up in the binding's surface registry. A surface is not in the registry while it starts or stops: - The transaction that stops a surface is never pulled, because `stopSurfaceWithSurfaceHandler` removes the surface from the registry before `SurfaceHandler::stop()` commits the empty tree. Mounting override delegates such as LayoutAnimations and C++ Animated never see those Deletes. With `enableLayoutAnimationsOnAndroid`, a layout animation that runs when its surface stops stays in flight, and `FabricUIManager` calls `driveCxxAnimations` on every frame until another surface pulls a transaction. - A commit that a bridgeless surface makes before `startSurfaceWithSurfaceHandler` registers it is not mounted until the surface commits again. - The legacy `stopSurface(int)` holds the registry lock across `stop()`. On the UI thread, the stop commit pulls inline and blocks on the same lock, so the app freezes. iOS does not look the surface up. `RCTMountingManager` captures the `MountingCoordinator` that reported the commit in the block it dispatches to the main queue, and pulls from it. This PR does the same. `schedulerShouldRenderTransactions` keeps the reporting `MountingCoordinator` until the UI thread pulls, and `pullAndExecuteTransaction` pulls from it. Java still receives only the surface id. The surface registry, the start and stop functions, `reportMount` and the push model are unchanged. As on iOS, a surface stopped on the UI thread has its stop transaction pulled during the stop commit. When it stops on another thread, its shadow tree is destroyed and its coordinator revoked before the UI thread pulls, so that transaction is not mounted. A pull of a stopped surface can log "Executing commit after surface N was stopped!" when a mounting override delegate still returns a transaction; iOS pulls these the same way. Only one coordinator per surface id is kept. Stopping and starting the same `ReactSurface` on the same host reuses its id. If the stopped surface's commit has not been pulled yet, the restarted surface's coordinator replaces it and a warning is logged. The replaced coordinator is revoked and would pull nothing. Java does not mount such a restarted surface anyway: `MountingManager.startSurface` logs "Called startSurface more than once" and keeps the stopped surface's manager. ## Changelog: [ANDROID] [FIXED] - Pull model: mount the commits of starting and stopping surfaces, and fix a freeze when the legacy `stopSurface(int)` runs on the UI thread Test Plan: New `FabricMountingManagerInstrumentationTest` cases drive a real `FabricUIManagerBinding`. The test helper runs no JavaScript and commits views to the surface's shadow tree itself. The pull-model cases run with the Props 2.0 flags and `enablePreallocatedPropsDiffOnInsertAndroid`. | Test | Without the fix | With the fix | | --- | --- | --- | | `native_stopSurfaceWithSurfaceHandler_pullModel_pullsDeletes` | fails: no Deletes are pulled | passes | | `native_stopSurface_pullModel_pullsDeletes` | hangs | passes | | `native_stopSurfaceWithSurfaceHandler_pushModel_pullsDeletes` | passes | passes | The push case is a control. The hang stops the run, so the bridgeless stop test was also run without the legacy one. With the fix, the whole class passes. A one-off check, not committed, covered the start: a bridgeless surface started on a background thread commits a view, and the start is held before `startSurfaceWithSurfaceHandler` registers the surface. On #59029 alone the view is not mounted while the start is held. With this PR it is mounted by the next frame. The test class has no Gradle target in the open-source repository, so I ran it on an Android emulator through a local androidTest source set: ``` ./gradlew :packages:react-native:ReactAndroid:assembleReleaseAndroidTest -PreactNativeArchitectures=arm64-v8a adb install -r -t packages/react-native/ReactAndroid/build/outputs/apk/androidTest/release/ReactAndroid-release-androidTest.apk adb shell am instrument -w -e class com.facebook.react.fabric.FabricMountingManagerInstrumentationTest \ com.facebook.react.tests.gradle/androidx.test.runner.AndroidJUnitRunner ``` I also checked the effect in RNTester with a temporary hook, not part of this PR. The Playground starts a 60 s `LayoutAnimation`, then `ReactDelegate.unloadApp()` stops the surface on the UI thread while the activity stays resumed. Props 2.0 and `enableLayoutAnimationsOnAndroid` were on: | Run | After the stop | | --- | --- | | Pull, without this PR | no `onAllAnimationsComplete`; `driveCxxAnimations` keeps running on every frame | | Pull, with this PR | `onAllAnimationsComplete` right after the stop, then no more driven frames | | Push, with this PR | `onAllAnimationsComplete` right after the stop, as before | Pulled By: bartlomiejbloniarz bartlomiejbloniarz Differential Revision: D124966432
1 parent 4490aa8 commit b703a10

6 files changed

Lines changed: 357 additions & 26 deletions

File tree

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

Lines changed: 17 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -125,29 +125,12 @@ void FabricUIManagerBinding::pullAndExecuteTransaction(SurfaceId surfaceId) {
125125

126126
std::shared_ptr<const MountingCoordinator> mountingCoordinator;
127127
{
128-
std::shared_lock lock(surfaceHandlerRegistryMutex_);
129-
auto iterator = surfaceHandlerRegistry_.find(surfaceId);
130-
if (iterator == surfaceHandlerRegistry_.end()) {
128+
std::scoped_lock lock(pullableMountingCoordinatorsMutex_);
129+
auto pullable = pullableMountingCoordinators_.extract(surfaceId);
130+
if (pullable.empty()) {
131131
return;
132132
}
133-
const auto* surfaceHandler = std::get_if<SurfaceHandler>(&iterator->second);
134-
jni::local_ref<SurfaceHandlerBinding::jhybridobject> javaSurfaceHandler;
135-
if (surfaceHandler == nullptr) {
136-
javaSurfaceHandler =
137-
std::get<jni::weak_ref<SurfaceHandlerBinding::jhybridobject>>(
138-
iterator->second)
139-
.lockLocal();
140-
if (javaSurfaceHandler) {
141-
surfaceHandler = &javaSurfaceHandler->cthis()->getSurfaceHandler();
142-
}
143-
}
144-
if (surfaceHandler != nullptr) {
145-
mountingCoordinator = surfaceHandler->getMountingCoordinator();
146-
}
147-
}
148-
149-
if (mountingCoordinator == nullptr) {
150-
return;
133+
mountingCoordinator = std::move(pullable.mapped());
151134
}
152135

153136
auto mountingManager = getMountingManager("pullAndExecuteTransaction");
@@ -732,8 +715,19 @@ void FabricUIManagerBinding::schedulerShouldRenderTransactions(
732715
// commit thread). Just notify Java that a transaction is available; the UI
733716
// thread calls back into `pullAndExecuteTransaction` at its next mount
734717
// dispatch.
735-
mountingManager->onTransactionAvailable(
736-
mountingCoordinator->getSurfaceId());
718+
auto surfaceId = mountingCoordinator->getSurfaceId();
719+
{
720+
std::scoped_lock lock(pullableMountingCoordinatorsMutex_);
721+
auto& pullable = pullableMountingCoordinators_[surfaceId];
722+
if (pullable != nullptr && pullable != mountingCoordinator) {
723+
LOG(WARNING)
724+
<< "FabricUIManagerBinding::schedulerShouldRenderTransactions: "
725+
<< "surface " << surfaceId
726+
<< " was restarted while the previous surface's commit waits to be pulled";
727+
}
728+
pullable = mountingCoordinator;
729+
}
730+
mountingManager->onTransactionAvailable(surfaceId);
737731
} else if (ReactNativeFeatureFlags::
738732
enableAccumulatedUpdatesInRawPropsAndroid()) {
739733
auto mountingTransaction = mountingCoordinator->pullTransaction(

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,12 @@ class FabricUIManagerBinding : public jni::HybridClass<FabricUIManagerBinding>,
167167
std::mutex pendingTransactionsMutex_;
168168
std::vector<MountingTransaction> pendingTransactions_;
169169

170+
// Pull model: keeps the coordinator that reported a surface's commit alive
171+
// until the UI thread handles the pull, as iOS does by capturing it in the
172+
// block it dispatches to the main queue. Java only gets the surface id.
173+
std::mutex pullableMountingCoordinatorsMutex_;
174+
std::unordered_map<SurfaceId, std::shared_ptr<const MountingCoordinator>> pullableMountingCoordinators_;
175+
170176
float pointScaleFactor_ = 1;
171177

172178
bool enableFabricLogs_{false};

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

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,54 @@
77

88
#include "FabricMountingManagerTestHelper.h"
99

10+
#include <react/fabric/FabricUIManagerBinding.h>
1011
#include <react/renderer/animationbackend/AnimatedProps.h>
12+
#include <react/renderer/componentregistry/ComponentDescriptorProviderRegistry.h>
13+
#include <react/renderer/components/root/RootShadowNode.h>
1114
#include <react/renderer/components/view/ViewComponentDescriptor.h>
1215
#include <react/renderer/components/view/ViewProps.h>
1316
#include <react/renderer/core/ComponentDescriptor.h>
17+
#include <react/renderer/mounting/MountingOverrideDelegate.h>
1418
#include <react/renderer/mounting/ShadowView.h>
19+
#include <react/renderer/scheduler/Scheduler.h>
1520

1621
namespace facebook::react {
1722

23+
class FabricMountingManagerTestHelper::DeleteRecorder
24+
: public MountingOverrideDelegate {
25+
public:
26+
bool shouldOverridePullTransaction() const override {
27+
return true;
28+
}
29+
30+
std::optional<MountingTransaction> pullTransaction(
31+
SurfaceId surfaceId,
32+
MountingTransaction::Number number,
33+
const TransactionTelemetry& telemetry,
34+
ShadowViewMutationList mutations) const override {
35+
for (const auto& mutation : mutations) {
36+
if (mutation.type == ShadowViewMutation::Delete) {
37+
deletedTags.push_back(mutation.oldChildShadowView.tag);
38+
}
39+
}
40+
return MountingTransaction{
41+
surfaceId, number, std::move(mutations), telemetry};
42+
}
43+
44+
mutable std::vector<Tag> deletedTags;
45+
};
46+
47+
namespace {
48+
49+
void runNoJavaScript(std::function<void(jsi::Runtime& runtime)>&& /*unused*/) {}
50+
51+
std::vector<Tag> toTags(jni::alias_ref<jni::JArrayInt> tags) {
52+
auto region = tags->getRegion(0, static_cast<jsize>(tags->size()));
53+
return {region.get(), region.get() + tags->size()};
54+
}
55+
56+
} // namespace
57+
1858
FabricMountingManagerTestHelper::FabricMountingManagerTestHelper(
1959
jni::alias_ref<JFabricUIManager::javaobject> jFabricUIManager)
2060
: javaUIManager_(jni::make_global(jFabricUIManager)) {
@@ -81,6 +121,85 @@ void FabricMountingManagerTestHelper::synchronouslyUpdateAnimatedProps(
81121
mountingManager_->synchronouslyUpdateAnimatedProps(updates);
82122
}
83123

124+
jni::local_ref<JRuntimeExecutor::jhybridobject>
125+
FabricMountingManagerTestHelper::createRuntimeExecutor() {
126+
return JRuntimeExecutor::newObjectCxxArgs(runNoJavaScript);
127+
}
128+
129+
jni::local_ref<JRuntimeScheduler::jhybridobject>
130+
FabricMountingManagerTestHelper::createRuntimeScheduler() {
131+
// Work scheduled here never runs and stays queued. A queued task holds the
132+
// UIManager, which holds this scheduler, so both outlive the test.
133+
runtimeScheduler_ = std::make_shared<RuntimeScheduler>(runNoJavaScript);
134+
return JRuntimeScheduler::newObjectCxxArgs(runtimeScheduler_);
135+
}
136+
137+
void FabricMountingManagerTestHelper::registerViewComponent(
138+
ComponentFactory* componentFactory) {
139+
componentFactory->buildRegistryFunction =
140+
[](const EventDispatcher::Weak& eventDispatcher,
141+
const std::shared_ptr<const ContextContainer>& contextContainer) {
142+
auto providerRegistry =
143+
std::make_shared<ComponentDescriptorProviderRegistry>();
144+
providerRegistry->add(
145+
concreteComponentDescriptorProvider<ViewComponentDescriptor>());
146+
return providerRegistry->createComponentDescriptorRegistry(
147+
{.eventDispatcher = eventDispatcher,
148+
.contextContainer = contextContainer,
149+
.flavor = nullptr});
150+
};
151+
}
152+
153+
void FabricMountingManagerTestHelper::commitViews(
154+
jint surfaceId,
155+
jni::alias_ref<jni::JArrayInt> tags) {
156+
auto children =
157+
std::make_shared<std::vector<std::shared_ptr<const ShadowNode>>>();
158+
for (auto tag : toTags(tags)) {
159+
auto props = std::make_shared<ViewProps>();
160+
props->collapsable = false;
161+
children->push_back(viewComponentDescriptor_->createShadowNode(
162+
{.props = props},
163+
viewComponentDescriptor_->createFamily(
164+
{.tag = tag, .surfaceId = surfaceId, .instanceHandle = nullptr})));
165+
}
166+
javaUIManager_->getBinding()
167+
->getScheduler()
168+
->getUIManager()
169+
->getShadowTreeRegistry()
170+
.visit(surfaceId, [&](const ShadowTree& shadowTree) {
171+
shadowTree.commit(
172+
[&](const RootShadowNode& oldRootShadowNode) {
173+
return std::make_shared<RootShadowNode>(
174+
oldRootShadowNode,
175+
ShadowNodeFragment{
176+
.props = ShadowNodeFragment::propsPlaceholder(),
177+
.children = children});
178+
},
179+
{});
180+
});
181+
}
182+
183+
void FabricMountingManagerTestHelper::recordPulledDeletes(jint surfaceId) {
184+
deleteRecorder_ = std::make_shared<DeleteRecorder>();
185+
javaUIManager_->getBinding()
186+
->getScheduler()
187+
->getUIManager()
188+
->getShadowTreeRegistry()
189+
.visit(surfaceId, [&](const ShadowTree& shadowTree) {
190+
shadowTree.getMountingCoordinator()->setMountingOverrideDelegate(
191+
deleteRecorder_);
192+
});
193+
}
194+
195+
jni::local_ref<jni::JArrayInt>
196+
FabricMountingManagerTestHelper::pulledDeleteTags() {
197+
const auto& tags = deleteRecorder_->deletedTags;
198+
auto array = jni::JArrayInt::newArray(tags.size());
199+
array->setRegion(0, static_cast<jsize>(tags.size()), tags.data());
200+
return array;
201+
}
202+
84203
void FabricMountingManagerTestHelper::registerNatives() {
85204
registerHybrid({
86205
makeNativeMethod(
@@ -99,6 +218,23 @@ void FabricMountingManagerTestHelper::registerNatives() {
99218
makeNativeMethod(
100219
"synchronouslyUpdateAnimatedProps",
101220
FabricMountingManagerTestHelper::synchronouslyUpdateAnimatedProps),
221+
makeNativeMethod(
222+
"createRuntimeExecutor",
223+
FabricMountingManagerTestHelper::createRuntimeExecutor),
224+
makeNativeMethod(
225+
"createRuntimeScheduler",
226+
FabricMountingManagerTestHelper::createRuntimeScheduler),
227+
makeNativeMethod(
228+
"registerViewComponent",
229+
FabricMountingManagerTestHelper::registerViewComponent),
230+
makeNativeMethod(
231+
"commitViews", FabricMountingManagerTestHelper::commitViews),
232+
makeNativeMethod(
233+
"recordPulledDeletes",
234+
FabricMountingManagerTestHelper::recordPulledDeletes),
235+
makeNativeMethod(
236+
"pulledDeleteTags",
237+
FabricMountingManagerTestHelper::pulledDeleteTags),
102238
});
103239
}
104240

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,12 @@
88
#pragma once
99

1010
#include <fbjni/fbjni.h>
11+
#include <react/jni/JRuntimeExecutor.h>
12+
#include <react/jni/JRuntimeScheduler.h>
1113
#include <react/jni/NativeMap.h>
1214
#include <react/renderer/components/view/ViewComponentDescriptor.h>
1315

16+
#include <react/fabric/ComponentFactory.h>
1417
#include <react/fabric/FabricMountingManager.h>
1518

1619
namespace facebook::react {
@@ -31,6 +34,17 @@ class FabricMountingManagerTestHelper : public jni::HybridClass<FabricMountingMa
3134
jni::alias_ref<jni::JArrayInt> tags,
3235
jni::alias_ref<jni::JArrayClass<NativeMap::javaobject>> props);
3336

37+
// A FabricUIManagerBinding registered on the helper's FabricUIManager with
38+
// these never runs JavaScript: its surfaces mount only what the helper
39+
// commits.
40+
jni::local_ref<JRuntimeExecutor::jhybridobject> createRuntimeExecutor();
41+
jni::local_ref<JRuntimeScheduler::jhybridobject> createRuntimeScheduler();
42+
void registerViewComponent(ComponentFactory *componentFactory);
43+
44+
void commitViews(jint surfaceId, jni::alias_ref<jni::JArrayInt> tags);
45+
void recordPulledDeletes(jint surfaceId);
46+
jni::local_ref<jni::JArrayInt> pulledDeleteTags();
47+
3448
static void registerNatives();
3549

3650
private:
@@ -45,6 +59,9 @@ class FabricMountingManagerTestHelper : public jni::HybridClass<FabricMountingMa
4559
jni::global_ref<JFabricUIManager::javaobject> javaUIManager_;
4660
std::shared_ptr<FabricMountingManager> mountingManager_;
4761
std::unique_ptr<ViewComponentDescriptor> viewComponentDescriptor_;
62+
std::shared_ptr<RuntimeScheduler> runtimeScheduler_;
63+
class DeleteRecorder;
64+
std::shared_ptr<DeleteRecorder> deleteRecorder_;
4865
};
4966

5067
} // namespace facebook::react

0 commit comments

Comments
 (0)