From c3dbebff5a272be8168517f435ad0f03cfe3eadc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Wed, 31 Jul 2024 07:02:16 -0700 Subject: [PATCH] Force accesses to ReactFabric to always go through proxy (#45822) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/45822 Changelog: [internal] ## Context `react-native/Libraries/Renderer/shims/ReactFabric` is the default module to load the RN renderer and loading it has side-effects (among other things it sets `global.RN$stopSurface`. We introduced a proxy module (`RendererProxy`) so we could use dependency injection to overwrite the renderer module with a custom implementation (the original goal was to be able to use a renderer version that didn't pull paper and only used Fabric). Unfortunately, using both the proxy and the module directly in some places leads to race conditions setting `global.RN$stopSurface`, which causes some screens to be rendered with one renderer and unmounted/disposed with a different one (because we accessed `ReactFabric` later and set `RN$stopSurface` from a different renderer implementation). When this happens, the unmount request in the other renderer is a no-op because no surface was renderer in it. This leads to surfaces not being disposed. ## Changes This modifies the proxy to add additional functions and modifies all other modules in the package to make sure that all the accesses to the renderer go through the proxy. Reviewed By: sammy-SC Differential Revision: D60452544 fbshipit-source-id: 1b17a95ed9b1c529718f22983dde1f00f1b2adae --- .../ReactFabricHostComponent.js | 2 +- .../ReactFabricPublicInstance.js | 10 ++++---- .../ReactNative/RendererImplementation.js | 24 ++++++++++++++++++- .../__snapshots__/public-api-test.js.snap | 6 +++++ .../private/webapis/dom/nodes/ReadOnlyNode.js | 10 ++++---- 5 files changed, 41 insertions(+), 11 deletions(-) diff --git a/packages/react-native/Libraries/ReactNative/ReactFabricPublicInstance/ReactFabricHostComponent.js b/packages/react-native/Libraries/ReactNative/ReactFabricPublicInstance/ReactFabricHostComponent.js index 7ddad94e8ec..b7a06c89ed7 100644 --- a/packages/react-native/Libraries/ReactNative/ReactFabricPublicInstance/ReactFabricHostComponent.js +++ b/packages/react-native/Libraries/ReactNative/ReactFabricPublicInstance/ReactFabricHostComponent.js @@ -20,7 +20,7 @@ import type { import type {ElementRef} from 'react'; import TextInputState from '../../Components/TextInput/TextInputState'; -import {getNodeFromInternalInstanceHandle} from '../../Renderer/shims/ReactFabric'; +import {getNodeFromInternalInstanceHandle} from '../../ReactNative/RendererProxy'; import {getFabricUIManager} from '../FabricUIManager'; import {create} from './ReactNativeAttributePayload'; import warnForStyleProps from './warnForStyleProps'; diff --git a/packages/react-native/Libraries/ReactNative/ReactFabricPublicInstance/ReactFabricPublicInstance.js b/packages/react-native/Libraries/ReactNative/ReactFabricPublicInstance/ReactFabricPublicInstance.js index ff9687732db..fe71cec166e 100644 --- a/packages/react-native/Libraries/ReactNative/ReactFabricPublicInstance/ReactFabricPublicInstance.js +++ b/packages/react-native/Libraries/ReactNative/ReactFabricPublicInstance/ReactFabricPublicInstance.js @@ -15,7 +15,7 @@ import type ReactNativeElement from '../../../src/private/webapis/dom/nodes/ReactNativeElement'; import type ReadOnlyText from '../../../src/private/webapis/dom/nodes/ReadOnlyText'; -import typeof ReactFabricType from '../../Renderer/shims/ReactFabric'; +import typeof * as RendererProxyT from '../../ReactNative/RendererProxy'; import type { InternalInstanceHandle, Node, @@ -32,7 +32,7 @@ let PublicInstanceClass: let ReadOnlyTextClass: Class; // Lazy loaded to avoid evaluating the module when using the legacy renderer. -let ReactFabric: ReactFabricType; +let RendererProxy: RendererProxyT; export function createPublicInstance( tag: number, @@ -78,10 +78,10 @@ export function getNodeFromPublicInstance( return null; } - if (ReactFabric == null) { - ReactFabric = require('../../Renderer/shims/ReactFabric'); + if (RendererProxy == null) { + RendererProxy = require('../../ReactNative/RendererProxy'); } - return ReactFabric.getNodeFromInternalInstanceHandle( + return RendererProxy.getNodeFromInternalInstanceHandle( publicInstance.__internalInstanceHandle, ); } diff --git a/packages/react-native/Libraries/ReactNative/RendererImplementation.js b/packages/react-native/Libraries/ReactNative/RendererImplementation.js index 2ba6ef04c83..7a9faac5ac7 100644 --- a/packages/react-native/Libraries/ReactNative/RendererImplementation.js +++ b/packages/react-native/Libraries/ReactNative/RendererImplementation.js @@ -8,7 +8,11 @@ * @flow strict-local */ -import type {HostComponent} from '../Renderer/shims/ReactNativeTypes'; +import type { + HostComponent, + InternalInstanceHandle, + Node, +} from '../Renderer/shims/ReactNativeTypes'; import type ReactFabricHostComponent from './ReactFabricPublicInstance/ReactFabricHostComponent'; import type {Element, ElementRef, ElementType} from 'react'; @@ -139,3 +143,21 @@ export function isChildPublicInstance( childInstance, ); } + +export function getNodeFromInternalInstanceHandle( + internalInstanceHandle: InternalInstanceHandle, +): ?Node { + // This is only available in Fabric + return require('../Renderer/shims/ReactFabric').getNodeFromInternalInstanceHandle( + internalInstanceHandle, + ); +} + +export function getPublicInstanceFromInternalInstanceHandle( + internalInstanceHandle: InternalInstanceHandle, +): mixed /*PublicInstance | PublicTextInstance | null*/ { + // This is only available in Fabric + return require('../Renderer/shims/ReactFabric').getPublicInstanceFromInternalInstanceHandle( + internalInstanceHandle, + ); +} diff --git a/packages/react-native/Libraries/__tests__/__snapshots__/public-api-test.js.snap b/packages/react-native/Libraries/__tests__/__snapshots__/public-api-test.js.snap index a4bff2c9f5c..e92361bd8cd 100644 --- a/packages/react-native/Libraries/__tests__/__snapshots__/public-api-test.js.snap +++ b/packages/react-native/Libraries/__tests__/__snapshots__/public-api-test.js.snap @@ -7548,6 +7548,12 @@ declare export function isChildPublicInstance( parentInstance: ReactFabricHostComponent | HostComponent, childInstance: ReactFabricHostComponent | HostComponent ): boolean; +declare export function getNodeFromInternalInstanceHandle( + internalInstanceHandle: InternalInstanceHandle +): ?Node; +declare export function getPublicInstanceFromInternalInstanceHandle( + internalInstanceHandle: InternalInstanceHandle +): mixed; " `; diff --git a/packages/react-native/src/private/webapis/dom/nodes/ReadOnlyNode.js b/packages/react-native/src/private/webapis/dom/nodes/ReadOnlyNode.js index e86862d6c6b..e368332a252 100644 --- a/packages/react-native/src/private/webapis/dom/nodes/ReadOnlyNode.js +++ b/packages/react-native/src/private/webapis/dom/nodes/ReadOnlyNode.js @@ -304,8 +304,10 @@ function setInstanceHandle( export function getShadowNode(node: ReadOnlyNode): ?ShadowNode { // Lazy import Fabric here to avoid DOM Node APIs classes from having side-effects. // With a static import we can't use these classes for Paper-only variants. - const ReactFabric = require('../../../../../Libraries/Renderer/shims/ReactFabric'); - return ReactFabric.getNodeFromInternalInstanceHandle(getInstanceHandle(node)); + const RendererProxy = require('../../../../../Libraries/ReactNative/RendererProxy'); + return RendererProxy.getNodeFromInternalInstanceHandle( + getInstanceHandle(node), + ); } export function getChildNodes( @@ -349,9 +351,9 @@ export function getPublicInstanceFromInternalInstanceHandle( ): ?ReadOnlyNode { // Lazy import Fabric here to avoid DOM Node APIs classes from having side-effects. // With a static import we can't use these classes for Paper-only variants. - const ReactFabric = require('../../../../../Libraries/Renderer/shims/ReactFabric'); + const RendererProxy = require('../../../../../Libraries/ReactNative/RendererProxy'); const mixedPublicInstance = - ReactFabric.getPublicInstanceFromInternalInstanceHandle(instanceHandle); + RendererProxy.getPublicInstanceFromInternalInstanceHandle(instanceHandle); // $FlowExpectedError[incompatible-return] React defines public instances as "mixed" because it can't access the definition from React Native. return mixedPublicInstance; }