Fix: Move destroy field to shared instance object (#26561)

This fixes the "double free" bug illustrated by the regression test
added in the previous commit.

The underlying issue is that `effect.destroy` field is a mutable field
but we read it during render. This is a concurrency bug — if we had a
borrow checker, it would not allow this.

It's rare in practice today because the field is updated during the
commit phase, which takes a lock on the fiber tree until all the effects
have fired. But it's still theoretically wrong because you can have
multiple Fiber copies each with their own reference to a single destroy
function, and indeed we discovered in production a scenario where this
happens via our current APIs.

In the future these types of scenarios will be much more common because
we will introduce features where effects may run concurrently with the
render phase — i.e. an imperative `hide` method that synchronously hides
a React tree and unmounts all its effects without entering the render
phase, and without interrupting a render phase that's already in
progress.

A future version of React may also be able to run the entire commit
phase concurrently with a subsequent render phase. We can't do this now
because our data structures are not fully thread safe (see: the Fiber
alternate model) but we should be able to do this in the future.

The fix I've introduced in this commit is to move the `destroy` field to
a separate object. The effect "instance" is a shared object that remains
the same for the entire lifetime of an effect. In Rust terms, a RefCell.
The field is `undefined` if the effect is unmounted, or if the effect
ran but is not stateful. We don't explicitly track whether the effect is
mounted or unmounted because that can be inferred by the hiddenness of
the fiber in the tree, i.e. whether there is a hidden Offscreen fiber
above it.

It's unfortunate that this is stored on a separate object, because it
adds more memory per effect instance, but it's conceptually sound. I
think there's likely a better data structure we could use for effects;
perhaps just one array of effect instances per fiber. But I think this
is OK for now despite the additional memory and we can follow up with
performance optimizations later.

---------

Co-authored-by: Dan Abramov <dan.abramov@gmail.com>
Co-authored-by: Rick Hanlon <rickhanlonii@gmail.com>
Co-authored-by: Jan Kassens <jan@kassens.net>

DiffTrain build for commit 85bb7b6.

Author: acdlite

compiled-rn/facebook-fbsource/xplat/js/RKJSModules/vendor/react-test-renderer/cjs/ReactTestRenderer-dev.js

@@ -6005,7 +6005,21 @@ var didWarnAboutUseWrappedInTryCatch;
 {
   didWarnAboutMismatchedHooksForComponent = new Set();
   didWarnAboutUseWrappedInTryCatch = new Set();
-} // These are set right before calling the component.
+} // The effect "instance" is a shared object that remains the same for the entire
+// lifetime of an effect. In Rust terms, a RefCell. We use it to store the
+// "destroy" function that is returned from an effect, because that is stateful.
+// The field is `undefined` if the effect is unmounted, or if the effect ran
+// but is not stateful. We don't explicitly track whether the effect is mounted
+// or unmounted because that can be inferred by the hiddenness of the fiber in
+// the tree, i.e. whether there is a hidden Offscreen fiber above it.
+//
+// It's unfortunate that this is stored on a separate object, because it adds
+// more memory per effect instance, but it's conceptually sound. I think there's
+// likely a better data structure we could use for effects; perhaps just one
+// array of effect instances per fiber. But I think this is OK for now despite
+// the additional memory and we can follow up with performance
+// optimizations later.
+// These are set right before calling the component.
 
 var renderLanes$1 = NoLanes; // The work-in-progress fiber. I've named it differently to distinguish it from
 // the work-in-progress hook.
@@ -6970,7 +6984,7 @@ function mountSyncExternalStore(subscribe, getSnapshot, getServerSnapshot) {
   pushEffect(
     HasEffect | Passive,
     updateStoreInstance.bind(null, fiber, inst, nextSnapshot, getSnapshot),
-    undefined,
+    createEffectInstance(),
     null
   );
   return nextSnapshot;
@@ -7025,7 +7039,7 @@ function updateSyncExternalStore(subscribe, getSnapshot, getServerSnapshot) {
     pushEffect(
       HasEffect | Passive,
       updateStoreInstance.bind(null, fiber, inst, nextSnapshot, getSnapshot),
-      undefined,
+      createEffectInstance(),
       null
     ); // Unless we're rendering a blocking lane, schedule a consistency check.
     // Right before committing, we will walk the tree and check if any of the
@@ -7150,11 +7164,11 @@ function rerenderState(initialState) {
   return rerenderReducer(basicStateReducer);
 }
 
-function pushEffect(tag, create, destroy, deps) {
+function pushEffect(tag, create, inst, deps) {
   var effect = {
     tag: tag,
     create: create,
-    destroy: destroy,
+    inst: inst,
     deps: deps,
     // Circular
     next: null
@@ -7181,6 +7195,12 @@ function pushEffect(tag, create, destroy, deps) {
   return effect;
 }
 
+function createEffectInstance() {
+  return {
+    destroy: undefined
+  };
+}
+
 function mountRef(initialValue) {
   var hook = mountWorkInProgressHook();
 
@@ -7205,25 +7225,24 @@ function mountEffectImpl(fiberFlags, hookFlags, create, deps) {
   hook.memoizedState = pushEffect(
     HasEffect | hookFlags,
     create,
-    undefined,
+    createEffectInstance(),
     nextDeps
   );
 }
 
 function updateEffectImpl(fiberFlags, hookFlags, create, deps) {
   var hook = updateWorkInProgressHook();
   var nextDeps = deps === undefined ? null : deps;
-  var destroy = undefined; // currentHook is null when rerendering after a render phase state update.
+  var effect = hook.memoizedState;
+  var inst = effect.inst; // currentHook is null when rerendering after a render phase state update.
 
   if (currentHook !== null) {
-    var prevEffect = currentHook.memoizedState;
-    destroy = prevEffect.destroy;
-
     if (nextDeps !== null) {
+      var prevEffect = currentHook.memoizedState;
       var prevDeps = prevEffect.deps;
 
       if (areHookInputsEqual(nextDeps, prevDeps)) {
-        hook.memoizedState = pushEffect(hookFlags, create, destroy, nextDeps);
+        hook.memoizedState = pushEffect(hookFlags, create, inst, nextDeps);
         return;
       }
     }
@@ -7233,7 +7252,7 @@ function updateEffectImpl(fiberFlags, hookFlags, create, deps) {
   hook.memoizedState = pushEffect(
     HasEffect | hookFlags,
     create,
-    destroy,
+    inst,
     nextDeps
   );
 }
@@ -16254,10 +16273,12 @@ function commitHookEffectListUnmount(
     do {
       if ((effect.tag & flags) === flags) {
         // Unmount
-        var destroy = effect.destroy;
-        effect.destroy = undefined;
+        var inst = effect.inst;
+        var destroy = inst.destroy;
 
         if (destroy !== undefined) {
+          inst.destroy = undefined;
+
           {
             if ((flags & Insertion) !== NoFlags) {
               setIsRunningInsertionEffect(true);
@@ -16297,7 +16318,9 @@ function commitHookEffectListMount(flags, finishedWork) {
           }
         }
 
-        effect.destroy = create();
+        var inst = effect.inst;
+        var destroy = create();
+        inst.destroy = destroy;
 
         {
           if ((flags & Insertion) !== NoFlags) {
@@ -16306,8 +16329,6 @@ function commitHookEffectListMount(flags, finishedWork) {
         }
 
         {
-          var destroy = effect.destroy;
-
           if (destroy !== undefined && typeof destroy !== "function") {
             var hookName = void 0;
 
@@ -17391,12 +17412,13 @@ function commitDeletionEffectsOnFiber(
             var effect = firstEffect;
 
             do {
-              var _effect = effect,
-                destroy = _effect.destroy,
-                tag = _effect.tag;
+              var tag = effect.tag;
+              var inst = effect.inst;
+              var destroy = inst.destroy;
 
               if (destroy !== undefined) {
                 if ((tag & Insertion) !== NoFlags) {
+                  inst.destroy = undefined;
                   safelyCallDestroy(
                     deletedFiber,
                     nearestMountedAncestor,
@@ -17405,13 +17427,15 @@ function commitDeletionEffectsOnFiber(
                 } else if ((tag & Layout) !== NoFlags) {
                   if (shouldProfile(deletedFiber)) {
                     startLayoutEffectTimer();
+                    inst.destroy = undefined;
                     safelyCallDestroy(
                       deletedFiber,
                       nearestMountedAncestor,
                       destroy
                     );
                     recordLayoutEffectDuration(deletedFiber);
                   } else {
+                    inst.destroy = undefined;
                     safelyCallDestroy(
                       deletedFiber,
                       nearestMountedAncestor,
@@ -23864,7 +23888,7 @@ function createFiberRoot(
   return root;
 }
 
-var ReactVersion = "18.3.0-next-60cfeeebe-20230406";
+var ReactVersion = "18.3.0-next-85bb7b685-20230406";
 
 // Might add PROFILE later.
 

compiled-rn/facebook-fbsource/xplat/js/RKJSModules/vendor/react-test-renderer/cjs/ReactTestRenderer-prod.js

@@ -2272,7 +2272,7 @@ function updateSyncExternalStore(subscribe, getSnapshot) {
     pushEffect(
       9,
       updateStoreInstance.bind(null, fiber, hook, nextSnapshot, getSnapshot),
-      void 0,
+      { destroy: void 0 },
       null
     );
     subscribe = workInProgressRoot;
@@ -2341,18 +2341,18 @@ function mountState(initialState) {
   );
   return [hook.memoizedState, initialState];
 }
-function pushEffect(tag, create, destroy, deps) {
-  tag = { tag: tag, create: create, destroy: destroy, deps: deps, next: null };
+function pushEffect(tag, create, inst, deps) {
+  tag = { tag: tag, create: create, inst: inst, deps: deps, next: null };
   create = currentlyRenderingFiber$1.updateQueue;
   null === create
     ? ((create = createFunctionComponentUpdateQueue()),
       (currentlyRenderingFiber$1.updateQueue = create),
       (create.lastEffect = tag.next = tag))
-    : ((destroy = create.lastEffect),
-      null === destroy
+    : ((inst = create.lastEffect),
+      null === inst
         ? (create.lastEffect = tag.next = tag)
-        : ((deps = destroy.next),
-          (destroy.next = tag),
+        : ((deps = inst.next),
+          (inst.next = tag),
           (tag.next = deps),
           (create.lastEffect = tag)));
   return tag;
@@ -2366,24 +2366,20 @@ function mountEffectImpl(fiberFlags, hookFlags, create, deps) {
   hook.memoizedState = pushEffect(
     1 | hookFlags,
     create,
-    void 0,
+    { destroy: void 0 },
     void 0 === deps ? null : deps
   );
 }
 function updateEffectImpl(fiberFlags, hookFlags, create, deps) {
   var hook = updateWorkInProgressHook();
   deps = void 0 === deps ? null : deps;
-  var destroy = void 0;
-  if (null !== currentHook) {
-    var prevEffect = currentHook.memoizedState;
-    destroy = prevEffect.destroy;
-    if (null !== deps && areHookInputsEqual(deps, prevEffect.deps)) {
-      hook.memoizedState = pushEffect(hookFlags, create, destroy, deps);
-      return;
-    }
-  }
-  currentlyRenderingFiber$1.flags |= fiberFlags;
-  hook.memoizedState = pushEffect(1 | hookFlags, create, destroy, deps);
+  var inst = hook.memoizedState.inst;
+  null !== currentHook &&
+  null !== deps &&
+  areHookInputsEqual(deps, currentHook.memoizedState.deps)
+    ? (hook.memoizedState = pushEffect(hookFlags, create, inst, deps))
+    : ((currentlyRenderingFiber$1.flags |= fiberFlags),
+      (hook.memoizedState = pushEffect(1 | hookFlags, create, inst, deps)));
 }
 function mountEffect(create, deps) {
   mountEffectImpl(8390656, 8, create, deps);
@@ -2690,7 +2686,7 @@ var HooksDispatcherOnMount = {
     pushEffect(
       9,
       updateStoreInstance.bind(null, fiber, root, nextSnapshot, getSnapshot),
-      void 0,
+      { destroy: void 0 },
       null
     );
     return nextSnapshot;
@@ -4883,10 +4879,11 @@ function commitHookEffectListUnmount(
     var effect = (updateQueue = updateQueue.next);
     do {
       if ((effect.tag & flags) === flags) {
-        var destroy = effect.destroy;
-        effect.destroy = void 0;
+        var inst = effect.inst,
+          destroy = inst.destroy;
         void 0 !== destroy &&
-          safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy);
+          ((inst.destroy = void 0),
+          safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy));
       }
       effect = effect.next;
     } while (effect !== updateQueue);
@@ -4899,8 +4896,10 @@ function commitHookEffectListMount(flags, finishedWork) {
     var effect = (finishedWork = finishedWork.next);
     do {
       if ((effect.tag & flags) === flags) {
-        var create$81 = effect.create;
-        effect.destroy = create$81();
+        var create$81 = effect.create,
+          inst = effect.inst;
+        create$81 = create$81();
+        inst.destroy = create$81;
       }
       effect = effect.next;
     } while (effect !== finishedWork);
@@ -5162,18 +5161,24 @@ function commitDeletionEffectsOnFiber(
       ) {
         var effect = (prevHostParent = prevHostParent.next);
         do {
-          var _effect = effect,
-            destroy = _effect.destroy;
-          _effect = _effect.tag;
+          var tag = effect.tag,
+            inst = effect.inst,
+            destroy = inst.destroy;
           void 0 !== destroy &&
-            (0 !== (_effect & 2)
-              ? safelyCallDestroy(deletedFiber, nearestMountedAncestor, destroy)
-              : 0 !== (_effect & 4) &&
+            (0 !== (tag & 2)
+              ? ((inst.destroy = void 0),
+                safelyCallDestroy(
+                  deletedFiber,
+                  nearestMountedAncestor,
+                  destroy
+                ))
+              : 0 !== (tag & 4) &&
+                ((inst.destroy = void 0),
                 safelyCallDestroy(
                   deletedFiber,
                   nearestMountedAncestor,
                   destroy
-                ));
+                )));
           effect = effect.next;
         } while (effect !== prevHostParent);
       }
@@ -8683,7 +8688,7 @@ var devToolsConfig$jscomp$inline_1028 = {
     throw Error("TestRenderer does not support findFiberByHostInstance()");
   },
   bundleType: 0,
-  version: "18.3.0-next-60cfeeebe-20230406",
+  version: "18.3.0-next-85bb7b685-20230406",
   rendererPackageName: "react-test-renderer"
 };
 var internals$jscomp$inline_1220 = {
@@ -8714,7 +8719,7 @@ var internals$jscomp$inline_1220 = {
   scheduleRoot: null,
   setRefreshHandler: null,
   getCurrentFiber: null,
-  reconcilerVersion: "18.3.0-next-60cfeeebe-20230406"
+  reconcilerVersion: "18.3.0-next-85bb7b685-20230406"
 };
 if ("undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__) {
   var hook$jscomp$inline_1221 = __REACT_DEVTOOLS_GLOBAL_HOOK__;