From 3fbf5c72f3464bb089993ec1787af04949d6e4aa Mon Sep 17 00:00:00 2001 From: Pieter De Baets Date: Thu, 26 Sep 2024 11:32:00 -0700 Subject: [PATCH] Fix ReactSurfaceView-backed roots not reporting the end of pending transactions correctly (#46676) 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 --- .../src/main/jni/react/fabric/Binding.cpp | 62 ++++++++++++++----- .../src/main/jni/react/fabric/Binding.h | 18 ++++-- 2 files changed, 61 insertions(+), 19 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/Binding.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/Binding.cpp index 6cb64b8049a..d87d4dcfd32 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/Binding.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/Binding.cpp @@ -13,7 +13,6 @@ #include "EventEmitterWrapper.h" #include "FabricMountingManager.h" #include "ReactNativeConfigHolder.h" -#include "SurfaceHandlerBinding.h" #include #include @@ -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(&iterator->second); + if (surfaceHandler == nullptr) { + auto javaSurfaceHandler = + std::get>( + 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(&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) { + 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) { + 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(&iterator->second); + if (surfaceHandler != nullptr) { + surfaceHandler->constraintLayout(constraints, context); + } } } diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/Binding.h b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/Binding.h index 17c09e46d9b..22ad8ae3a03 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/Binding.h +++ b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/Binding.h @@ -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 { constexpr static auto kJavaDescriptor = "Lcom/facebook/react/fabric/Binding;"; @@ -95,9 +95,11 @@ class Binding : public jni::HybridClass, void stopSurface(jint surfaceId); - void registerSurface(SurfaceHandlerBinding* surfaceHandler); + void registerSurface( + jni::alias_ref surfaceHandler); - void unregisterSurface(SurfaceHandlerBinding* surfaceHandler); + void unregisterSurface( + jni::alias_ref surfaceHandler); void schedulerDidFinishTransaction( const MountingCoordinator::Shared& mountingCoordinator) override; @@ -148,7 +150,15 @@ class Binding : public jni::HybridClass, BackgroundExecutor backgroundExecutor_; - std::unordered_map 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>> + surfaceHandlerRegistry_{}; std::shared_mutex surfaceHandlerRegistryMutex_; // Protects `surfaceHandlerRegistry_`.