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

This reverts commit d41e5c2.

Author: kassens

packages/react-reconciler/src/ReactFiberCommitWork.js

@@ -592,9 +592,10 @@ function commitHookEffectListUnmount(
     do {
       if ((effect.tag & flags) === flags) {
         // Unmount
-        const destroy = effect.destroy;
-        effect.destroy = undefined;
+        const inst = effect.inst;
+        const destroy = inst.destroy;
         if (destroy !== undefined) {
+          inst.destroy = undefined;
           if (enableSchedulingProfiler) {
             if ((flags & HookPassive) !== NoHookEffect) {
               markComponentPassiveEffectUnmountStarted(finishedWork);
@@ -653,7 +654,9 @@ function commitHookEffectListMount(flags: HookFlags, finishedWork: Fiber) {
             setIsRunningInsertionEffect(true);
           }
         }
-        effect.destroy = create();
+        const inst = effect.inst;
+        const destroy = create();
+        inst.destroy = destroy;
         if (__DEV__) {
           if ((flags & HookInsertion) !== NoHookEffect) {
             setIsRunningInsertionEffect(false);
@@ -669,7 +672,6 @@ function commitHookEffectListMount(flags: HookFlags, finishedWork: Fiber) {
         }
 
         if (__DEV__) {
-          const destroy = effect.destroy;
           if (destroy !== undefined && typeof destroy !== 'function') {
             let hookName;
             if ((effect.tag & HookLayout) !== NoFlags) {
@@ -2190,9 +2192,12 @@ function commitDeletionEffectsOnFiber(
 
             let effect = firstEffect;
             do {
-              const {destroy, tag} = effect;
+              const tag = effect.tag;
+              const inst = effect.inst;
+              const destroy = inst.destroy;
               if (destroy !== undefined) {
                 if ((tag & HookInsertion) !== NoHookEffect) {
+                  inst.destroy = undefined;
                   safelyCallDestroy(
                     deletedFiber,
                     nearestMountedAncestor,
@@ -2205,13 +2210,15 @@ function commitDeletionEffectsOnFiber(
 
                   if (shouldProfile(deletedFiber)) {
                     startLayoutEffectTimer();
+                    inst.destroy = undefined;
                     safelyCallDestroy(
                       deletedFiber,
                       nearestMountedAncestor,
                       destroy,
                     );
                     recordLayoutEffectDuration(deletedFiber);
                   } else {
+                    inst.destroy = undefined;
                     safelyCallDestroy(
                       deletedFiber,
                       nearestMountedAncestor,

packages/react-reconciler/src/ReactFiberHooks.js

@@ -180,11 +180,29 @@ export type Hook = {
   next: Hook | null,
 };
 
+// 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.
+type EffectInstance = {
+  destroy: void | (() => void),
+};
+
 export type Effect = {
   tag: HookFlags,
   create: () => (() => void) | void,
-  destroy: (() => void) | void,
-  deps: Array<mixed> | void | null,
+  inst: EffectInstance,
+  deps: Array<mixed> | null,
   next: Effect,
 };
 
@@ -1662,7 +1680,7 @@ function mountSyncExternalStore<T>(
   pushEffect(
     HookHasEffect | HookPassive,
     updateStoreInstance.bind(null, fiber, inst, nextSnapshot, getSnapshot),
-    undefined,
+    createEffectInstance(),
     null,
   );
 
@@ -1719,7 +1737,7 @@ function updateSyncExternalStore<T>(
     pushEffect(
       HookHasEffect | HookPassive,
       updateStoreInstance.bind(null, fiber, inst, nextSnapshot, getSnapshot),
-      undefined,
+      createEffectInstance(),
       null,
     );
 
@@ -1860,13 +1878,13 @@ function rerenderState<S>(
 function pushEffect(
   tag: HookFlags,
   create: () => (() => void) | void,
-  destroy: (() => void) | void,
-  deps: Array<mixed> | void | null,
+  inst: EffectInstance,
+  deps: Array<mixed> | null,
 ): Effect {
   const effect: Effect = {
     tag,
     create,
-    destroy,
+    inst,
     deps,
     // Circular
     next: (null: any),
@@ -1891,6 +1909,10 @@ function pushEffect(
   return effect;
 }
 
+function createEffectInstance(): EffectInstance {
+  return {destroy: undefined};
+}
+
 let stackContainsErrorMessage: boolean | null = null;
 
 function getCallerStackFrame(): string {
@@ -1994,7 +2016,7 @@ function mountEffectImpl(
   hook.memoizedState = pushEffect(
     HookHasEffect | hookFlags,
     create,
-    undefined,
+    createEffectInstance(),
     nextDeps,
   );
 }
@@ -2007,16 +2029,16 @@ function updateEffectImpl(
 ): void {
   const hook = updateWorkInProgressHook();
   const nextDeps = deps === undefined ? null : deps;
-  let destroy = undefined;
+  const effect: Effect = hook.memoizedState;
+  const inst = effect.inst;
 
   // currentHook is null when rerendering after a render phase state update.
   if (currentHook !== null) {
-    const prevEffect = currentHook.memoizedState;
-    destroy = prevEffect.destroy;
     if (nextDeps !== null) {
+      const prevEffect: Effect = currentHook.memoizedState;
       const prevDeps = prevEffect.deps;
       if (areHookInputsEqual(nextDeps, prevDeps)) {
-        hook.memoizedState = pushEffect(hookFlags, create, destroy, nextDeps);
+        hook.memoizedState = pushEffect(hookFlags, create, inst, nextDeps);
         return;
       }
     }
@@ -2027,7 +2049,7 @@ function updateEffectImpl(
   hook.memoizedState = pushEffect(
     HookHasEffect | hookFlags,
     create,
-    destroy,
+    inst,
     nextDeps,
   );
 }

packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js

@@ -496,4 +496,82 @@ describe('ReactSuspenseEffectsSemanticsDOM', () => {
     ReactDOM.render(null, container);
     assertLog(['Unmount']);
   });
+
+  it('does not call cleanup effects twice after a bailout', async () => {
+    const never = new Promise(resolve => {});
+    function Never() {
+      throw never;
+    }
+
+    let setSuspended;
+    let setLetter;
+
+    function App() {
+      const [suspended, _setSuspended] = React.useState(false);
+      setSuspended = _setSuspended;
+      const [letter, _setLetter] = React.useState('A');
+      setLetter = _setLetter;
+
+      return (
+        <React.Suspense fallback="Loading...">
+          <Child letter={letter} />
+          {suspended && <Never />}
+        </React.Suspense>
+      );
+    }
+
+    let nextId = 0;
+    const freed = new Set();
+    let setStep;
+
+    function Child({letter}) {
+      const [, _setStep] = React.useState(0);
+      setStep = _setStep;
+
+      React.useLayoutEffect(() => {
+        const localId = nextId++;
+        Scheduler.log('Did mount: ' + letter + localId);
+        return () => {
+          if (freed.has(localId)) {
+            throw Error('Double free: ' + letter + localId);
+          }
+          freed.add(localId);
+          Scheduler.log('Will unmount: ' + letter + localId);
+        };
+      }, [letter]);
+    }
+
+    const root = ReactDOMClient.createRoot(container);
+    await act(() => {
+      root.render(<App />);
+    });
+    assertLog(['Did mount: A0']);
+
+    await act(() => {
+      setStep(1);
+      setSuspended(false);
+    });
+    assertLog([]);
+
+    await act(() => {
+      setStep(1);
+    });
+    assertLog([]);
+
+    await act(() => {
+      setSuspended(true);
+    });
+    assertLog(['Will unmount: A0']);
+
+    await act(() => {
+      setSuspended(false);
+      setLetter('B');
+    });
+    assertLog(['Did mount: B1']);
+
+    await act(() => {
+      root.unmount();
+    });
+    assertLog(['Will unmount: B1']);
+  });
 });