Flush useEffect clean up functions in the passive effects phase (#17925)

* Flush useEffect clean up functions in the passive effects phase

This is a change in behavior that may cause broken product code, so it has been added behind a killswitch (deferPassiveEffectCleanupDuringUnmount)

* Avoid scheduling unnecessary callbacks for cleanup effects

Updated enqueuePendingPassiveEffectDestroyFn() to check rootDoesHavePassiveEffects before scheduling a new callback. This way we'll only schedule (at most) one.

* Updated newly added test for added clarity.

* Cleaned up hooks effect tags

We previously used separate Mount* and Unmount* tags to track hooks work for each phase (snapshot, mutation, layout, and passive). This was somewhat complicated to trace through and there were man tag types we never even used (e.g. UnmountLayout, MountMutation, UnmountSnapshot). In addition to this, it left passive and layout hooks looking the same after renders without changed dependencies, which meant we were unable to reliably defer passive effect destroy functions until after the commit phase.

This commit reduces the effect tag types to only include Layout and Passive and differentiates between work and no-work with an HasEffect flag.

* Disabled deferred passive effects flushing in OSS builds for now

* Split up unmount and mount effects list traversal

Author: Brian Vaughn

packages/react-reconciler/src/ReactFiberCommitWork.js

@@ -26,6 +26,7 @@ import type {ReactPriorityLevel} from './SchedulerWithReactIntegration';
 
 import {unstable_wrap as Schedule_tracing_wrap} from 'scheduler/tracing';
 import {
+  deferPassiveEffectCleanupDuringUnmount,
   enableSchedulerTracing,
   enableProfilerTimer,
   enableSuspenseServerRenderer,
@@ -109,16 +110,13 @@ import {
   captureCommitPhaseError,
   resolveRetryThenable,
   markCommitTimeOfFallback,
+  enqueuePendingPassiveEffectDestroyFn,
 } from './ReactFiberWorkLoop';
 import {
   NoEffect as NoHookEffect,
-  UnmountSnapshot,
-  UnmountMutation,
-  MountMutation,
-  UnmountLayout,
-  MountLayout,
-  UnmountPassive,
-  MountPassive,
+  HasEffect as HookHasEffect,
+  Layout as HookLayout,
+  Passive as HookPassive,
 } from './ReactHookEffectTags';
 import {didWarnAboutReassigningProps} from './ReactFiberBeginWork';
 import {runWithPriority, NormalPriority} from './SchedulerWithReactIntegration';
@@ -250,7 +248,6 @@ function commitBeforeMutationLifeCycles(
     case ForwardRef:
     case SimpleMemoComponent:
     case Chunk: {
-      commitHookEffectList(UnmountSnapshot, NoHookEffect, finishedWork);
       return;
     }
     case ClassComponent: {
@@ -328,26 +325,34 @@ function commitBeforeMutationLifeCycles(
   );
 }
 
-function commitHookEffectList(
-  unmountTag: number,
-  mountTag: number,
-  finishedWork: Fiber,
-) {
+function commitHookEffectListUnmount(tag: number, finishedWork: Fiber) {
   const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any);
   let lastEffect = updateQueue !== null ? updateQueue.lastEffect : null;
   if (lastEffect !== null) {
     const firstEffect = lastEffect.next;
     let effect = firstEffect;
     do {
-      if ((effect.tag & unmountTag) !== NoHookEffect) {
+      if ((effect.tag & tag) === tag) {
         // Unmount
         const destroy = effect.destroy;
         effect.destroy = undefined;
         if (destroy !== undefined) {
           destroy();
         }
       }
-      if ((effect.tag & mountTag) !== NoHookEffect) {
+      effect = effect.next;
+    } while (effect !== firstEffect);
+  }
+}
+
+function commitHookEffectListMount(tag: number, finishedWork: Fiber) {
+  const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any);
+  let lastEffect = updateQueue !== null ? updateQueue.lastEffect : null;
+  if (lastEffect !== null) {
+    const firstEffect = lastEffect.next;
+    let effect = firstEffect;
+    do {
+      if ((effect.tag & tag) === tag) {
         // Mount
         const create = effect.create;
         effect.destroy = create();
@@ -398,8 +403,11 @@ export function commitPassiveHookEffects(finishedWork: Fiber): void {
       case ForwardRef:
       case SimpleMemoComponent:
       case Chunk: {
-        commitHookEffectList(UnmountPassive, NoHookEffect, finishedWork);
-        commitHookEffectList(NoHookEffect, MountPassive, finishedWork);
+        // TODO (#17945) We should call all passive destroy functions (for all fibers)
+        // before calling any create functions. The current approach only serializes
+        // these for a single fiber.
+        commitHookEffectListUnmount(HookPassive | HookHasEffect, finishedWork);
+        commitHookEffectListMount(HookPassive | HookHasEffect, finishedWork);
         break;
       }
       default:
@@ -419,7 +427,11 @@ function commitLifeCycles(
     case ForwardRef:
     case SimpleMemoComponent:
     case Chunk: {
-      commitHookEffectList(UnmountLayout, MountLayout, finishedWork);
+      // At this point layout effects have already been destroyed (during mutation phase).
+      // This is done to prevent sibling component effects from interfering with each other,
+      // e.g. a destroy function in one component should never override a ref set
+      // by a create function in another component during the same commit.
+      commitHookEffectListMount(HookLayout | HookHasEffect, finishedWork);
       return;
     }
     case ClassComponent: {
@@ -756,32 +768,47 @@ function commitUnmount(
         if (lastEffect !== null) {
           const firstEffect = lastEffect.next;
 
-          // When the owner fiber is deleted, the destroy function of a passive
-          // effect hook is called during the synchronous commit phase. This is
-          // a concession to implementation complexity. Calling it in the
-          // passive effect phase (like they usually are, when dependencies
-          // change during an update) would require either traversing the
-          // children of the deleted fiber again, or including unmount effects
-          // as part of the fiber effect list.
-          //
-          // Because this is during the sync commit phase, we need to change
-          // the priority.
-          //
-          // TODO: Reconsider this implementation trade off.
-          const priorityLevel =
-            renderPriorityLevel > NormalPriority
-              ? NormalPriority
-              : renderPriorityLevel;
-          runWithPriority(priorityLevel, () => {
+          if (deferPassiveEffectCleanupDuringUnmount) {
             let effect = firstEffect;
             do {
-              const destroy = effect.destroy;
+              const {destroy, tag} = effect;
               if (destroy !== undefined) {
-                safelyCallDestroy(current, destroy);
+                if ((tag & HookPassive) !== NoHookEffect) {
+                  enqueuePendingPassiveEffectDestroyFn(destroy);
+                } else {
+                  safelyCallDestroy(current, destroy);
+                }
               }
               effect = effect.next;
             } while (effect !== firstEffect);
-          });
+          } else {
+            // When the owner fiber is deleted, the destroy function of a passive
+            // effect hook is called during the synchronous commit phase. This is
+            // a concession to implementation complexity. Calling it in the
+            // passive effect phase (like they usually are, when dependencies
+            // change during an update) would require either traversing the
+            // children of the deleted fiber again, or including unmount effects
+            // as part of the fiber effect list.
+            //
+            // Because this is during the sync commit phase, we need to change
+            // the priority.
+            //
+            // TODO: Reconsider this implementation trade off.
+            const priorityLevel =
+              renderPriorityLevel > NormalPriority
+                ? NormalPriority
+                : renderPriorityLevel;
+            runWithPriority(priorityLevel, () => {
+              let effect = firstEffect;
+              do {
+                const destroy = effect.destroy;
+                if (destroy !== undefined) {
+                  safelyCallDestroy(current, destroy);
+                }
+                effect = effect.next;
+              } while (effect !== firstEffect);
+            });
+          }
         }
       }
       return;
@@ -1285,9 +1312,12 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
       case MemoComponent:
       case SimpleMemoComponent:
       case Chunk: {
-        // Note: We currently never use MountMutation, but useLayout uses
-        // UnmountMutation.
-        commitHookEffectList(UnmountMutation, MountMutation, finishedWork);
+        // Layout effects are destroyed during the mutation phase so that all
+        // destroy functions for all fibers are called before any create functions.
+        // This prevents sibling component effects from interfering with each other,
+        // e.g. a destroy function in one component should never override a ref set
+        // by a create function in another component during the same commit.
+        commitHookEffectListUnmount(HookLayout | HookHasEffect, finishedWork);
         return;
       }
       case Profiler: {
@@ -1325,9 +1355,12 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
     case MemoComponent:
     case SimpleMemoComponent:
     case Chunk: {
-      // Note: We currently never use MountMutation, but useLayout uses
-      // UnmountMutation.
-      commitHookEffectList(UnmountMutation, MountMutation, finishedWork);
+      // Layout effects are destroyed during the mutation phase so that all
+      // destroy functions for all fibers are called before any create functions.
+      // This prevents sibling component effects from interfering with each other,
+      // e.g. a destroy function in one component should never override a ref set
+      // by a create function in another component during the same commit.
+      commitHookEffectListUnmount(HookLayout | HookHasEffect, finishedWork);
       return;
     }
     case ClassComponent: {

packages/react-reconciler/src/ReactFiberHooks.js

@@ -28,11 +28,9 @@ import {
   Passive as PassiveEffect,
 } from 'shared/ReactSideEffectTags';
 import {
-  NoEffect as NoHookEffect,
-  UnmountMutation,
-  MountLayout,
-  UnmountPassive,
-  MountPassive,
+  HasEffect as HookHasEffect,
+  Layout as HookLayout,
+  Passive as HookPassive,
 } from './ReactHookEffectTags';
 import {
   scheduleWork,
@@ -923,7 +921,12 @@ function mountEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
   const hook = mountWorkInProgressHook();
   const nextDeps = deps === undefined ? null : deps;
   currentlyRenderingFiber.effectTag |= fiberEffectTag;
-  hook.memoizedState = pushEffect(hookEffectTag, create, undefined, nextDeps);
+  hook.memoizedState = pushEffect(
+    HookHasEffect | hookEffectTag,
+    create,
+    undefined,
+    nextDeps,
+  );
 }
 
 function updateEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
@@ -937,15 +940,20 @@ function updateEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
     if (nextDeps !== null) {
       const prevDeps = prevEffect.deps;
       if (areHookInputsEqual(nextDeps, prevDeps)) {
-        pushEffect(NoHookEffect, create, destroy, nextDeps);
+        pushEffect(hookEffectTag, create, destroy, nextDeps);
         return;
       }
     }
   }
 
   currentlyRenderingFiber.effectTag |= fiberEffectTag;
 
-  hook.memoizedState = pushEffect(hookEffectTag, create, destroy, nextDeps);
+  hook.memoizedState = pushEffect(
+    HookHasEffect | hookEffectTag,
+    create,
+    destroy,
+    nextDeps,
+  );
 }
 
 function mountEffect(
@@ -960,7 +968,7 @@ function mountEffect(
   }
   return mountEffectImpl(
     UpdateEffect | PassiveEffect,
-    UnmountPassive | MountPassive,
+    HookPassive,
     create,
     deps,
   );
@@ -978,7 +986,7 @@ function updateEffect(
   }
   return updateEffectImpl(
     UpdateEffect | PassiveEffect,
-    UnmountPassive | MountPassive,
+    HookPassive,
     create,
     deps,
   );
@@ -988,24 +996,14 @@ function mountLayoutEffect(
   create: () => (() => void) | void,
   deps: Array<mixed> | void | null,
 ): void {
-  return mountEffectImpl(
-    UpdateEffect,
-    UnmountMutation | MountLayout,
-    create,
-    deps,
-  );
+  return mountEffectImpl(UpdateEffect, HookLayout, create, deps);
 }
 
 function updateLayoutEffect(
   create: () => (() => void) | void,
   deps: Array<mixed> | void | null,
 ): void {
-  return updateEffectImpl(
-    UpdateEffect,
-    UnmountMutation | MountLayout,
-    create,
-    deps,
-  );
+  return updateEffectImpl(UpdateEffect, HookLayout, create, deps);
 }
 
 function imperativeHandleEffect<T>(
@@ -1059,7 +1057,7 @@ function mountImperativeHandle<T>(
 
   return mountEffectImpl(
     UpdateEffect,
-    UnmountMutation | MountLayout,
+    HookLayout,
     imperativeHandleEffect.bind(null, create, ref),
     effectDeps,
   );
@@ -1086,7 +1084,7 @@ function updateImperativeHandle<T>(
 
   return updateEffectImpl(
     UpdateEffect,
-    UnmountMutation | MountLayout,
+    HookLayout,
     imperativeHandleEffect.bind(null, create, ref),
     effectDeps,
   );

packages/react-reconciler/src/ReactFiberWorkLoop.js

@@ -18,6 +18,7 @@ import type {Hook} from './ReactFiberHooks';
 
 import {
   warnAboutDeprecatedLifecycles,
+  deferPassiveEffectCleanupDuringUnmount,
   enableUserTimingAPI,
   enableSuspenseServerRenderer,
   replayFailedUnitOfWorkWithInvokeGuardedCallback,
@@ -257,6 +258,7 @@ let rootDoesHavePassiveEffects: boolean = false;
 let rootWithPendingPassiveEffects: FiberRoot | null = null;
 let pendingPassiveEffectsRenderPriority: ReactPriorityLevel = NoPriority;
 let pendingPassiveEffectsExpirationTime: ExpirationTime = NoWork;
+let pendingUnmountedPassiveEffectDestroyFunctions: Array<() => void> = [];
 
 let rootsWithPendingDiscreteUpdates: Map<
   FiberRoot,
@@ -2176,6 +2178,21 @@ export function flushPassiveEffects() {
   }
 }
 
+export function enqueuePendingPassiveEffectDestroyFn(
+  destroy: () => void,
+): void {
+  if (deferPassiveEffectCleanupDuringUnmount) {
+    pendingUnmountedPassiveEffectDestroyFunctions.push(destroy);
+    if (!rootDoesHavePassiveEffects) {
+      rootDoesHavePassiveEffects = true;
+      scheduleCallback(NormalPriority, () => {
+        flushPassiveEffects();
+        return null;
+      });
+    }
+  }
+}
+
 function flushPassiveEffectsImpl() {
   if (rootWithPendingPassiveEffects === null) {
     return false;
@@ -2193,6 +2210,20 @@ function flushPassiveEffectsImpl() {
   executionContext |= CommitContext;
   const prevInteractions = pushInteractions(root);
 
+  if (deferPassiveEffectCleanupDuringUnmount) {
+    // Flush any pending passive effect destroy functions that belong to
+    // components that were unmounted during the most recent commit.
+    for (
+      let i = 0;
+      i < pendingUnmountedPassiveEffectDestroyFunctions.length;
+      i++
+    ) {
+      const destroy = pendingUnmountedPassiveEffectDestroyFunctions[i];
+      invokeGuardedCallback(null, destroy, null);
+    }
+    pendingUnmountedPassiveEffectDestroyFunctions.length = 0;
+  }
+
   // Note: This currently assumes there are no passive effects on the root
   // fiber, because the root is not part of its own effect list. This could
   // change in the future.

packages/react-reconciler/src/ReactHookEffectTags.js

@@ -9,11 +9,11 @@
 
 export type HookEffectTag = number;
 
-export const NoEffect = /*             */ 0b00000000;
-export const UnmountSnapshot = /*      */ 0b00000010;
-export const UnmountMutation = /*      */ 0b00000100;
-export const MountMutation = /*        */ 0b00001000;
-export const UnmountLayout = /*        */ 0b00010000;
-export const MountLayout = /*          */ 0b00100000;
-export const MountPassive = /*         */ 0b01000000;
-export const UnmountPassive = /*       */ 0b10000000;
+export const NoEffect = /*  */ 0b000;
+
+// Represents whether effect should fire.
+export const HasEffect = /* */ 0b001;
+
+// Represents the phase in which the effect (not the clean-up) fires.
+export const Layout = /*    */ 0b010;
+export const Passive = /*   */ 0b100;