Facebook/react-native
1
0
Fork 0
mirror of https://github.com/facebook/react-native.git synced 2025-11-01 09:14:26 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix ReactSurfaceView-backed roots not reporting the end of pending transactions correctly (#46676)

Browse Source 
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/46676

Changelog: [internal]

## Context

We recently "fixed" a problem in `MountingCoordinator` on Android where it would report that it doesn't have any pending transactions when, in fact, it does. The fix introduces a new method in that class to delay marking transactions as done until a mount hook is invoked for that surface.

That fixed the issue... by always reporting that there were pending transactions accidentally.

The reason for this bug is that the mount hook doesn't have access to the mounting coordinator of the surface if the surface is registered through some of the methods in `Binding.cpp` that don't add the surface to a registry. In that case, we can never mark the transactions as done and the mounting coordinator for those surfaces always report pending transactions incorrectly.

NOTE: this bug only affects apps that have the `fixMountingCoordinatorReportedPendingTransactionsOnAndroid` feature flag enabled.

## Changes

This fixes the issue by making sure that surfaces are always registered in the registry and that we can access their mounting coordinators in the mount hook to report the transactions as done.

Reviewed By: rubennorte

Differential Revision: D63466672

fbshipit-source-id: a621a12cda89a3ab7331d3c6a16c6cdfa9341821
This commit is contained in:
Pieter De Baets
2024-09-26 11:32:00 -07:00
committed by Facebook GitHub Bot
parent 04da3d784b
commit 3fbf5c72f3
2 changed files with 61 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/react-native/ReactAndroid/src/main/jni/react/fabric/Binding.cpp
+47 -15
View File
@@ -13,7 +13,6 @@
#include "EventEmitterWrapper.h"
#include "FabricMountingManager.h"
#include "ReactNativeConfigHolder.h"
#include "SurfaceHandlerBinding.h"
#include <cxxreact/SystraceSection.h>
#include <fbjni/fbjni.h>
@@ -111,8 +110,20 @@ void Binding::reportMount(SurfaceId surfaceId) {
std::shared_lock lock(surfaceHandlerRegistryMutex_);
auto iterator = surfaceHandlerRegistry_.find(surfaceId);
if (iterator != surfaceHandlerRegistry_.end()) {
auto& surfaceHandler = iterator->second;
surfaceHandler.getMountingCoordinator()->didPerformAsyncTransactions();
const auto* surfaceHandler =
std::get_if<SurfaceHandler>(&iterator->second);
if (surfaceHandler == nullptr) {
auto javaSurfaceHandler =
std::get<jni::weak_ref<SurfaceHandlerBinding::jhybridobject>>(
iterator->second)
.lockLocal();
if (javaSurfaceHandler) {
surfaceHandler = &javaSurfaceHandler->cthis()->getSurfaceHandler();
}
}
if (surfaceHandler != nullptr) {
surfaceHandler->getMountingCoordinator()->didPerformAsyncTransactions();
}
} else {
LOG(ERROR) << "Binding::reportMount: Surface with id " << surfaceId
<< " is not found";
@@ -258,16 +269,19 @@ void Binding::stopSurface(jint surfaceId) {
std::unique_lock lock(surfaceHandlerRegistryMutex_);
auto iterator = surfaceHandlerRegistry_.find(surfaceId);
if (iterator == surfaceHandlerRegistry_.end()) {
LOG(ERROR) << "Binding::stopSurface: Surface with given id is not found";
return;
}
auto surfaceHandler = std::move(iterator->second);
auto* surfaceHandler = std::get_if<SurfaceHandler>(&iterator->second);
if (surfaceHandler != nullptr) {
surfaceHandler->stop();
scheduler->unregisterSurface(*surfaceHandler);
} else {
LOG(ERROR) << "Java-owned SurfaceHandler found in stopSurface";
}
surfaceHandlerRegistry_.erase(iterator);
surfaceHandler.stop();
scheduler->unregisterSurface(surfaceHandler);
}
auto mountingManager = getMountingManager("stopSurface");
@@ -277,8 +291,12 @@ void Binding::stopSurface(jint surfaceId) {
mountingManager->onSurfaceStop(surfaceId);
}
void Binding::registerSurface(SurfaceHandlerBinding* surfaceHandlerBinding) {
const auto& surfaceHandler = surfaceHandlerBinding->getSurfaceHandler();
void Binding::registerSurface(
jni::alias_ref<SurfaceHandlerBinding::jhybridobject>
surfaceHandlerBinding) {
const auto& surfaceHandler =
surfaceHandlerBinding->cthis()->getSurfaceHandler();
auto scheduler = getScheduler();
if (!scheduler) {
LOG(ERROR) << "Binding::registerSurface: scheduler disappeared";
@@ -286,6 +304,12 @@ void Binding::registerSurface(SurfaceHandlerBinding* surfaceHandlerBinding) {
}
scheduler->registerSurface(surfaceHandler);
{
std::unique_lock lock(surfaceHandlerRegistryMutex_);
surfaceHandlerRegistry_.emplace(
surfaceHandler.getSurfaceId(), jni::make_weak(surfaceHandlerBinding));
}
auto mountingManager = getMountingManager("registerSurface");
if (!mountingManager) {
return;
@@ -293,8 +317,11 @@ void Binding::registerSurface(SurfaceHandlerBinding* surfaceHandlerBinding) {
mountingManager->onSurfaceStart(surfaceHandler.getSurfaceId());
}
void Binding::unregisterSurface(SurfaceHandlerBinding* surfaceHandlerBinding) {
const auto& surfaceHandler = surfaceHandlerBinding->getSurfaceHandler();
void Binding::unregisterSurface(
jni::alias_ref<SurfaceHandlerBinding::jhybridobject>
surfaceHandlerBinding) {
const auto& surfaceHandler =
surfaceHandlerBinding->cthis()->getSurfaceHandler();
auto scheduler = getScheduler();
if (!scheduler) {
LOG(ERROR) << "Binding::unregisterSurface: scheduler disappeared";
@@ -302,6 +329,11 @@ void Binding::unregisterSurface(SurfaceHandlerBinding* surfaceHandlerBinding) {
}
scheduler->unregisterSurface(surfaceHandler);
{
std::unique_lock lock(surfaceHandlerRegistryMutex_);
surfaceHandlerRegistry_.erase(surfaceHandler.getSurfaceId());
}
auto mountingManager = getMountingManager("unregisterSurface");
if (!mountingManager) {
return;
@@ -347,15 +379,15 @@ void Binding::setConstraints(
std::shared_lock lock(surfaceHandlerRegistryMutex_);
auto iterator = surfaceHandlerRegistry_.find(surfaceId);
if (iterator == surfaceHandlerRegistry_.end()) {
LOG(ERROR)
<< "Binding::setConstraints: Surface with given id is not found";
return;
}
auto& surfaceHandler = iterator->second;
surfaceHandler.constraintLayout(constraints, context);
auto* surfaceHandler = std::get_if<SurfaceHandler>(&iterator->second);
if (surfaceHandler != nullptr) {
surfaceHandler->constraintLayout(constraints, context);
}
}
}
packages/react-native/ReactAndroid/src/main/jni/react/fabric/Binding.h
+14 -4
View File
@@ -23,6 +23,7 @@
#include "EventEmitterWrapper.h"
#include "JFabricUIManager.h"
#include "SurfaceHandlerBinding.h"
namespace facebook::react {
@@ -33,7 +34,6 @@ class Instance;
class LayoutAnimationDriver;
class ReactNativeConfig;
class Scheduler;
class SurfaceHandlerBinding;
struct JBinding : public jni::JavaClass<JBinding> {
constexpr static auto kJavaDescriptor = "Lcom/facebook/react/fabric/Binding;";
@@ -95,9 +95,11 @@ class Binding : public jni::HybridClass<Binding, JBinding>,
void stopSurface(jint surfaceId);
void registerSurface(SurfaceHandlerBinding* surfaceHandler);
void registerSurface(
jni::alias_ref<SurfaceHandlerBinding::jhybridobject> surfaceHandler);
void unregisterSurface(SurfaceHandlerBinding* surfaceHandler);
void unregisterSurface(
jni::alias_ref<SurfaceHandlerBinding::jhybridobject> surfaceHandler);
void schedulerDidFinishTransaction(
const MountingCoordinator::Shared& mountingCoordinator) override;
@@ -148,7 +150,15 @@ class Binding : public jni::HybridClass<Binding, JBinding>,
BackgroundExecutor backgroundExecutor_;
std::unordered_map<SurfaceId, SurfaceHandler> surfaceHandlerRegistry_{};
// Roots not created through ReactSurface (non-bridgeless) will store their
// SurfaceHandler here, for other roots we keep a weak reference to the Java
// owner
std::unordered_map<
SurfaceId,
std::variant<
SurfaceHandler,
jni::weak_ref<SurfaceHandlerBinding::jhybridobject>>>
surfaceHandlerRegistry_{};
std::shared_mutex
surfaceHandlerRegistryMutex_; // Protects `surfaceHandlerRegistry_`.