Skip to content

Commit fb19d4e

Browse files
Pull the transaction that stops a surface in the pull model (#59030)
Summary: With `enableMountingCoordinatorPullModelAndroid`, the UI thread pulls a surface's transaction through `FabricUIManagerBinding`'s surface registry. `stopSurfaceWithSurfaceHandler` removes the surface from the registry before `SurfaceHandler::stop()` commits the empty tree, so the transaction that deletes the surface's views is never pulled. Mounting override delegates such as LayoutAnimations and C++ Animated never see those Deletes. With `enableLayoutAnimationsOnAndroid`, a layout animation that is running when its surface stops stays in flight. `FabricUIManager` then calls `driveCxxAnimations` on every frame until another surface pulls a transaction. This PR: - Pulls and executes a commit on the commit thread in `schedulerShouldRenderTransactions` when its surface is not registered, as push does. - Makes `stopSurface(int)` unregister the surface before stopping it, like `stopSurfaceWithSurfaceHandler`. - Registers a bridgeless surface before `SurfaceHandler::start()`, like `startSurface` and `startSurfaceWithConstraints`. Otherwise a commit made during the start would also take the commit-thread path and mount after the next pull. Only a stopping surface now commits while unregistered. - Skips a registered surface that is not running in `pullAndExecuteTransaction` and `reportMount`. A restarted surface keeps its id and is now registered before it starts, so a pull left over from its previous run could reach it before it has a shadow tree. The legacy `setConstraints` commits while it holds the registry's shared lock, so the new registry check nests a shared lock there. `pullAndExecuteTransaction` already does the same on main. ## Changelog: [ANDROID] [FIXED] - Pull the transaction that stops a surface with `enableMountingCoordinatorPullModelAndroid` Test Plan: New `FabricMountingManagerInstrumentationTest` cases drive a real `FabricUIManagerBinding`. The test helper commits to the surface's shadow tree directly, without JavaScript. 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_stopSurfaceWithSurfaceHandler_pushModel_pullsDeletes` | passes | passes | | `native_startSurfaceWithSurfaceHandler_pullModel_mountsCommitDuringStartInOrder` | passes | passes | | `native_registeredSurfaceThatIsNotRunning_pullModel_skipsPullAndMountReport` | crashes in `SurfaceHandler::getMountingCoordinator()` | passes | The start test passes on main, where nothing takes the commit-thread path. It guards the registration order: with the fallback in place but the surface registered after `start()`, it fails with the views mounted as `[42, 43]` instead of `[43, 42]`. 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 ``` With the fix, the whole class passes. I also used RNTester with a temporary hook: the Playground starts a 60 s `LayoutAnimation`, then `ReactDelegate.unloadApp()` stops the surface while the activity stays resumed. Props 2.0 and `enableLayoutAnimationsOnAndroid` were on: | | Before | After | | --- | --- | --- | | Pull | no `onAllAnimationsComplete`; `driveCxxAnimations` still runs every frame 100 s after the stop | `onAllAnimationsComplete` right after the stop, then no more frames | | Push | `onAllAnimationsComplete` right after the stop | same | Differential Revision: D124966432 Pulled By: bartlomiejbloniarz
1 parent 8d61635 commit fb19d4e

6 files changed

Lines changed: 433 additions & 53 deletions

File tree

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

Lines changed: 65 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,33 @@
3535

3636
namespace facebook::react {
3737

38+
namespace {
39+
40+
std::shared_ptr<const MountingCoordinator> getRunningMountingCoordinator(
41+
const std::variant<
42+
SurfaceHandler,
43+
jni::weak_ref<SurfaceHandlerBinding::jhybridobject>>& registryEntry) {
44+
const auto* surfaceHandler = std::get_if<SurfaceHandler>(&registryEntry);
45+
jni::local_ref<SurfaceHandlerBinding::jhybridobject> javaSurfaceHandler;
46+
if (surfaceHandler == nullptr) {
47+
javaSurfaceHandler =
48+
std::get<jni::weak_ref<SurfaceHandlerBinding::jhybridobject>>(
49+
registryEntry)
50+
.lockLocal();
51+
if (!javaSurfaceHandler) {
52+
return nullptr;
53+
}
54+
surfaceHandler = &javaSurfaceHandler->cthis()->getSurfaceHandler();
55+
}
56+
// Callers hold the registry lock, and a surface leaves it before it stops.
57+
if (surfaceHandler->getStatus() != SurfaceHandler::Status::Running) {
58+
return nullptr;
59+
}
60+
return surfaceHandler->getMountingCoordinator();
61+
}
62+
63+
} // namespace
64+
3865
void FabricUIManagerBinding::initHybrid(jni::alias_ref<jhybridobject> jobj) {
3966
setCxxInstance(jobj);
4067
}
@@ -89,22 +116,10 @@ void FabricUIManagerBinding::reportMount(SurfaceId surfaceId) {
89116
std::shared_lock lock(surfaceHandlerRegistryMutex_);
90117
auto iterator = surfaceHandlerRegistry_.find(surfaceId);
91118
if (iterator != surfaceHandlerRegistry_.end()) {
92-
const auto* surfaceHandler =
93-
std::get_if<SurfaceHandler>(&iterator->second);
94-
if (surfaceHandler == nullptr) {
95-
auto javaSurfaceHandler =
96-
std::get<jni::weak_ref<SurfaceHandlerBinding::jhybridobject>>(
97-
iterator->second)
98-
.lockLocal();
99-
if (javaSurfaceHandler) {
100-
surfaceHandler = &javaSurfaceHandler->cthis()->getSurfaceHandler();
101-
}
102-
}
103-
if (surfaceHandler != nullptr) {
104-
auto mountingCoordinator = surfaceHandler->getMountingCoordinator();
105-
if (mountingCoordinator != nullptr) {
106-
mountingCoordinator->didPerformAsyncTransactions();
107-
}
119+
auto mountingCoordinator =
120+
getRunningMountingCoordinator(iterator->second);
121+
if (mountingCoordinator != nullptr) {
122+
mountingCoordinator->didPerformAsyncTransactions();
108123
}
109124
} else {
110125
LOG(ERROR) << "FabricUIManagerBinding::reportMount: Surface with id "
@@ -130,20 +145,7 @@ void FabricUIManagerBinding::pullAndExecuteTransaction(SurfaceId surfaceId) {
130145
if (iterator == surfaceHandlerRegistry_.end()) {
131146
return;
132147
}
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-
}
148+
mountingCoordinator = getRunningMountingCoordinator(iterator->second);
147149
}
148150

149151
if (mountingCoordinator == nullptr) {
@@ -169,6 +171,11 @@ void FabricUIManagerBinding::pullAndExecuteTransaction(SurfaceId surfaceId) {
169171
}
170172
}
171173

174+
bool FabricUIManagerBinding::isSurfaceRegistered(SurfaceId surfaceId) {
175+
std::shared_lock lock(surfaceHandlerRegistryMutex_);
176+
return surfaceHandlerRegistry_.contains(surfaceId);
177+
}
178+
172179
#pragma mark - Surface management
173180

174181
// Used by bridgeless
@@ -195,6 +202,13 @@ void FabricUIManagerBinding::startSurfaceWithSurfaceHandler(
195202
<< "FabricUIManagerBinding::startSurfaceWithSurfaceHandler: scheduler disappeared";
196203
return;
197204
}
205+
206+
{
207+
std::unique_lock lock(surfaceHandlerRegistryMutex_);
208+
surfaceHandlerRegistry_.emplace(
209+
surfaceId, jni::make_weak(surfaceHandlerBinding));
210+
}
211+
198212
scheduler->registerSurface(surfaceHandler);
199213

200214
auto mountingManager = getMountingManager("startSurfaceWithSurfaceHandler");
@@ -208,12 +222,6 @@ void FabricUIManagerBinding::startSurfaceWithSurfaceHandler(
208222
surfaceHandler.getMountingCoordinator()->setMountingOverrideDelegate(
209223
animationDriver_);
210224
}
211-
212-
{
213-
std::unique_lock lock(surfaceHandlerRegistryMutex_);
214-
surfaceHandlerRegistry_.emplace(
215-
surfaceId, jni::make_weak(surfaceHandlerBinding));
216-
}
217225
}
218226

219227
// Used by non-bridgeless+Fabric
@@ -459,23 +467,25 @@ void FabricUIManagerBinding::stopSurface(jint surfaceId) {
459467
return;
460468
}
461469

470+
// The surface leaves the registry before it stops: the stop commit reads the
471+
// registry on this thread.
472+
decltype(surfaceHandlerRegistry_)::node_type registryEntry;
462473
{
463474
std::unique_lock lock(surfaceHandlerRegistryMutex_);
464-
auto iterator = surfaceHandlerRegistry_.find(surfaceId);
465-
if (iterator == surfaceHandlerRegistry_.end()) {
466-
LOG(ERROR)
467-
<< "FabricUIManagerBinding::stopSurface: Surface with given id is not found";
468-
return;
469-
}
475+
registryEntry = surfaceHandlerRegistry_.extract(surfaceId);
476+
}
477+
if (registryEntry.empty()) {
478+
LOG(ERROR)
479+
<< "FabricUIManagerBinding::stopSurface: Surface with given id is not found";
480+
return;
481+
}
470482

471-
auto* surfaceHandler = std::get_if<SurfaceHandler>(&iterator->second);
472-
if (surfaceHandler != nullptr) {
473-
surfaceHandler->stop();
474-
scheduler->unregisterSurface(*surfaceHandler);
475-
} else {
476-
LOG(ERROR) << "Java-owned SurfaceHandler found in stopSurface";
477-
}
478-
surfaceHandlerRegistry_.erase(iterator);
483+
auto* surfaceHandler = std::get_if<SurfaceHandler>(&registryEntry.mapped());
484+
if (surfaceHandler != nullptr) {
485+
surfaceHandler->stop();
486+
scheduler->unregisterSurface(*surfaceHandler);
487+
} else {
488+
LOG(ERROR) << "Java-owned SurfaceHandler found in stopSurface";
479489
}
480490

481491
auto mountingManager = getMountingManager("stopSurface");
@@ -727,7 +737,11 @@ void FabricUIManagerBinding::schedulerShouldRenderTransactions(
727737
}
728738
}
729739

730-
if (ReactNativeFeatureFlags::enableMountingCoordinatorPullModelAndroid()) {
740+
// The UI thread pulls only a registered surface, so a commit of a surface
741+
// outside the registry, such as the commit that stops it, is pulled below,
742+
// on the commit thread.
743+
if (ReactNativeFeatureFlags::enableMountingCoordinatorPullModelAndroid() &&
744+
isSurfaceRegistered(mountingCoordinator->getSurfaceId())) {
731745
// Pull model: do NOT pull the transaction or build the batch here (on the
732746
// commit thread). Just notify Java that a transaction is available; the UI
733747
// thread calls back into `pullAndExecuteTransaction` at its next mount

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,8 @@ class FabricUIManagerBinding : public jni::HybridClass<FabricUIManagerBinding>,
137137

138138
void pullAndExecuteTransaction(SurfaceId surfaceId);
139139

140+
bool isSurfaceRegistered(SurfaceId surfaceId);
141+
140142
jint findNextFocusableElement(jint parentTag, jint focusedTag, jint direction);
141143

142144
jintArray getRelativeAncestorList(jint rootTag, jint childTag);

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

Lines changed: 168 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,56 @@
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>
20+
21+
#include <thread>
1522

1623
namespace facebook::react {
1724

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

126+
jni::local_ref<JRuntimeExecutor::jhybridobject>
127+
FabricMountingManagerTestHelper::createRuntimeExecutor() {
128+
return JRuntimeExecutor::newObjectCxxArgs(runNoJavaScript);
129+
}
130+
131+
jni::local_ref<JRuntimeScheduler::jhybridobject>
132+
FabricMountingManagerTestHelper::createRuntimeScheduler() {
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+
commitViewTags(surfaceId, toTags(tags));
157+
}
158+
159+
void FabricMountingManagerTestHelper::commitViewsOnSurfaceStart(
160+
jni::alias_ref<jni::JArrayInt> tags) {
161+
javaUIManager_->getBinding()
162+
->getScheduler()
163+
->getUIManager()
164+
->addOnSurfaceStartCallback(
165+
[this, tags = toTags(tags)](const ShadowTree& shadowTree) {
166+
std::thread([&] {
167+
jni::ThreadScope::WithClassLoader(
168+
[&] { commitViewTags(shadowTree.getSurfaceId(), tags); });
169+
}).join();
170+
});
171+
}
172+
173+
void FabricMountingManagerTestHelper::stopSurfaceHandler(
174+
jni::alias_ref<SurfaceHandlerBinding::jhybridobject>
175+
surfaceHandlerBinding) {
176+
surfaceHandlerBinding->cthis()->getSurfaceHandler().stop();
177+
}
178+
179+
void FabricMountingManagerTestHelper::commitViewTags(
180+
SurfaceId surfaceId,
181+
const std::vector<Tag>& tags) {
182+
auto children =
183+
std::make_shared<std::vector<std::shared_ptr<const ShadowNode>>>();
184+
for (auto tag : tags) {
185+
auto props = std::make_shared<ViewProps>();
186+
props->collapsable = false;
187+
children->push_back(viewComponentDescriptor_->createShadowNode(
188+
{.props = props},
189+
viewComponentDescriptor_->createFamily(
190+
{.tag = tag, .surfaceId = surfaceId, .instanceHandle = nullptr})));
191+
}
192+
javaUIManager_->getBinding()
193+
->getScheduler()
194+
->getUIManager()
195+
->getShadowTreeRegistry()
196+
.visit(surfaceId, [&](const ShadowTree& shadowTree) {
197+
shadowTree.commit(
198+
[&](const RootShadowNode& oldRootShadowNode) {
199+
return std::make_shared<RootShadowNode>(
200+
oldRootShadowNode,
201+
ShadowNodeFragment{
202+
.props = ShadowNodeFragment::propsPlaceholder(),
203+
.children = children});
204+
},
205+
{});
206+
});
207+
}
208+
209+
void FabricMountingManagerTestHelper::recordPulledDeletes(jint surfaceId) {
210+
deleteRecorder_ = std::make_shared<DeleteRecorder>();
211+
javaUIManager_->getBinding()
212+
->getScheduler()
213+
->getUIManager()
214+
->getShadowTreeRegistry()
215+
.visit(surfaceId, [&](const ShadowTree& shadowTree) {
216+
shadowTree.getMountingCoordinator()->setMountingOverrideDelegate(
217+
deleteRecorder_);
218+
});
219+
}
220+
221+
jni::local_ref<jni::JArrayInt>
222+
FabricMountingManagerTestHelper::pulledDeleteTags() {
223+
const auto& tags = deleteRecorder_->deletedTags;
224+
auto array = jni::JArrayInt::newArray(tags.size());
225+
array->setRegion(0, static_cast<jsize>(tags.size()), tags.data());
226+
return array;
227+
}
228+
84229
void FabricMountingManagerTestHelper::registerNatives() {
85230
registerHybrid({
86231
makeNativeMethod(
@@ -99,6 +244,29 @@ void FabricMountingManagerTestHelper::registerNatives() {
99244
makeNativeMethod(
100245
"synchronouslyUpdateAnimatedProps",
101246
FabricMountingManagerTestHelper::synchronouslyUpdateAnimatedProps),
247+
makeNativeMethod(
248+
"createRuntimeExecutor",
249+
FabricMountingManagerTestHelper::createRuntimeExecutor),
250+
makeNativeMethod(
251+
"createRuntimeScheduler",
252+
FabricMountingManagerTestHelper::createRuntimeScheduler),
253+
makeNativeMethod(
254+
"registerViewComponent",
255+
FabricMountingManagerTestHelper::registerViewComponent),
256+
makeNativeMethod(
257+
"commitViews", FabricMountingManagerTestHelper::commitViews),
258+
makeNativeMethod(
259+
"commitViewsOnSurfaceStart",
260+
FabricMountingManagerTestHelper::commitViewsOnSurfaceStart),
261+
makeNativeMethod(
262+
"stopSurfaceHandler",
263+
FabricMountingManagerTestHelper::stopSurfaceHandler),
264+
makeNativeMethod(
265+
"recordPulledDeletes",
266+
FabricMountingManagerTestHelper::recordPulledDeletes),
267+
makeNativeMethod(
268+
"pulledDeleteTags",
269+
FabricMountingManagerTestHelper::pulledDeleteTags),
102270
});
103271
}
104272

0 commit comments

Comments
 (0)