@samitouri / QOS-React-2 / commits / 85bb7b685b

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>

Andrew Clark committed Apr 6, 2023 at 12:08 UTC 85bb7b685b7aec50879703b94dd31523cf69b34d
3 files changed +125 -18
packages/react-reconciler/src/ReactFiberCommitWork.js
+12 -5
@@ -592,9 +592,10 @@ function commitHookEffectListUnmount(
592 do {
593 if ((effect.tag & flags) === flags) {
594 // Unmount
595 - const destroy = effect.destroy;
596 - effect.destroy = undefined;
595 + const inst = effect.inst;
596 + const destroy = inst.destroy;
597 if (destroy !== undefined) {
598 + inst.destroy = undefined;
599 if (enableSchedulingProfiler) {
600 if ((flags & HookPassive) !== NoHookEffect) {
601 markComponentPassiveEffectUnmountStarted(finishedWork);
@@ -653,7 +654,9 @@ function commitHookEffectListMount(flags: HookFlags, finishedWork: Fiber) {
654 setIsRunningInsertionEffect(true);
655 }
656 }
656 - effect.destroy = create();
657 + const inst = effect.inst;
658 + const destroy = create();
659 + inst.destroy = destroy;
660 if (__DEV__) {
661 if ((flags & HookInsertion) !== NoHookEffect) {
662 setIsRunningInsertionEffect(false);
@@ -669,7 +672,6 @@ function commitHookEffectListMount(flags: HookFlags, finishedWork: Fiber) {
672 }
673
674 if (__DEV__) {
672 - const destroy = effect.destroy;
675 if (destroy !== undefined && typeof destroy !== 'function') {
676 let hookName;
677 if ((effect.tag & HookLayout) !== NoFlags) {
@@ -2188,9 +2190,12 @@ function commitDeletionEffectsOnFiber(
2190
2191 let effect = firstEffect;
2192 do {
2191 - const {destroy, tag} = effect;
2193 + const tag = effect.tag;
2194 + const inst = effect.inst;
2195 + const destroy = inst.destroy;
2196 if (destroy !== undefined) {
2197 if ((tag & HookInsertion) !== NoHookEffect) {
2198 + inst.destroy = undefined;
2199 safelyCallDestroy(
2200 deletedFiber,
2201 nearestMountedAncestor,
@@ -2203,6 +2208,7 @@ function commitDeletionEffectsOnFiber(
2208
2209 if (shouldProfile(deletedFiber)) {
2210 startLayoutEffectTimer();
2211 + inst.destroy = undefined;
2212 safelyCallDestroy(
2213 deletedFiber,
2214 nearestMountedAncestor,
@@ -2210,6 +2216,7 @@ function commitDeletionEffectsOnFiber(
2216 );
2217 recordLayoutEffectDuration(deletedFiber);
2218 } else {
2219 + inst.destroy = undefined;
2220 safelyCallDestroy(
2221 deletedFiber,
2222 nearestMountedAncestor,
packages/react-reconciler/src/ReactFiberHooks.js
+35 -13
@@ -180,11 +180,29 @@ export type Hook = {
180 next: Hook | null,
181 };
182
183 +// The effect "instance" is a shared object that remains the same for the entire
184 +// lifetime of an effect. In Rust terms, a RefCell. We use it to store the
185 +// "destroy" function that is returned from an effect, because that is stateful.
186 +// The field is `undefined` if the effect is unmounted, or if the effect ran
187 +// but is not stateful. We don't explicitly track whether the effect is mounted
188 +// or unmounted because that can be inferred by the hiddenness of the fiber in
189 +// the tree, i.e. whether there is a hidden Offscreen fiber above it.
190 +//
191 +// It's unfortunate that this is stored on a separate object, because it adds
192 +// more memory per effect instance, but it's conceptually sound. I think there's
193 +// likely a better data structure we could use for effects; perhaps just one
194 +// array of effect instances per fiber. But I think this is OK for now despite
195 +// the additional memory and we can follow up with performance
196 +// optimizations later.
197 +type EffectInstance = {
198 + destroy: void | (() => void),
199 +};
200 +
201 export type Effect = {
202 tag: HookFlags,
203 create: () => (() => void) | void,
186 - destroy: (() => void) | void,
187 - deps: Array<mixed> | void | null,
204 + inst: EffectInstance,
205 + deps: Array<mixed> | null,
206 next: Effect,
207 };
208
@@ -1662,7 +1680,7 @@ function mountSyncExternalStore<T>(
1680 pushEffect(
1681 HookHasEffect | HookPassive,
1682 updateStoreInstance.bind(null, fiber, inst, nextSnapshot, getSnapshot),
1665 - undefined,
1683 + createEffectInstance(),
1684 null,
1685 );
1686
@@ -1719,7 +1737,7 @@ function updateSyncExternalStore<T>(
1737 pushEffect(
1738 HookHasEffect | HookPassive,
1739 updateStoreInstance.bind(null, fiber, inst, nextSnapshot, getSnapshot),
1722 - undefined,
1740 + createEffectInstance(),
1741 null,
1742 );
1743
@@ -1860,13 +1878,13 @@ function rerenderState<S>(
1878 function pushEffect(
1879 tag: HookFlags,
1880 create: () => (() => void) | void,
1863 - destroy: (() => void) | void,
1864 - deps: Array<mixed> | void | null,
1881 + inst: EffectInstance,
1882 + deps: Array<mixed> | null,
1883 ): Effect {
1884 const effect: Effect = {
1885 tag,
1886 create,
1869 - destroy,
1887 + inst,
1888 deps,
1889 // Circular
1890 next: (null: any),
@@ -1891,6 +1909,10 @@ function pushEffect(
1909 return effect;
1910 }
1911
1912 +function createEffectInstance(): EffectInstance {
1913 + return {destroy: undefined};
1914 +}
1915 +
1916 let stackContainsErrorMessage: boolean | null = null;
1917
1918 function getCallerStackFrame(): string {
@@ -1994,7 +2016,7 @@ function mountEffectImpl(
2016 hook.memoizedState = pushEffect(
2017 HookHasEffect | hookFlags,
2018 create,
1997 - undefined,
2019 + createEffectInstance(),
2020 nextDeps,
2021 );
2022 }
@@ -2007,16 +2029,16 @@ function updateEffectImpl(
2029 ): void {
2030 const hook = updateWorkInProgressHook();
2031 const nextDeps = deps === undefined ? null : deps;
2010 - let destroy = undefined;
2032 + const effect: Effect = hook.memoizedState;
2033 + const inst = effect.inst;
2034
2035 // currentHook is null when rerendering after a render phase state update.
2036 if (currentHook !== null) {
2014 - const prevEffect = currentHook.memoizedState;
2015 - destroy = prevEffect.destroy;
2037 if (nextDeps !== null) {
2038 + const prevEffect: Effect = currentHook.memoizedState;
2039 const prevDeps = prevEffect.deps;
2040 if (areHookInputsEqual(nextDeps, prevDeps)) {
2019 - hook.memoizedState = pushEffect(hookFlags, create, destroy, nextDeps);
2041 + hook.memoizedState = pushEffect(hookFlags, create, inst, nextDeps);
2042 return;
2043 }
2044 }
@@ -2027,7 +2049,7 @@ function updateEffectImpl(
2049 hook.memoizedState = pushEffect(
2050 HookHasEffect | hookFlags,
2051 create,
2030 - destroy,
2052 + inst,
2053 nextDeps,
2054 );
2055 }
packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js
+78
@@ -496,4 +496,82 @@ describe('ReactSuspenseEffectsSemanticsDOM', () => {
496 ReactDOM.render(null, container);
497 assertLog(['Unmount']);
498 });
499 +
500 + it('does not call cleanup effects twice after a bailout', async () => {
501 + const never = new Promise(resolve => {});
502 + function Never() {
503 + throw never;
504 + }
505 +
506 + let setSuspended;
507 + let setLetter;
508 +
509 + function App() {
510 + const [suspended, _setSuspended] = React.useState(false);
511 + setSuspended = _setSuspended;
512 + const [letter, _setLetter] = React.useState('A');
513 + setLetter = _setLetter;
514 +
515 + return (
516 + <React.Suspense fallback="Loading...">
517 + <Child letter={letter} />
518 + {suspended && <Never />}
519 + </React.Suspense>
520 + );
521 + }
522 +
523 + let nextId = 0;
524 + const freed = new Set();
525 + let setStep;
526 +
527 + function Child({letter}) {
528 + const [, _setStep] = React.useState(0);
529 + setStep = _setStep;
530 +
531 + React.useLayoutEffect(() => {
532 + const localId = nextId++;
533 + Scheduler.log('Did mount: ' + letter + localId);
534 + return () => {
535 + if (freed.has(localId)) {
536 + throw Error('Double free: ' + letter + localId);
537 + }
538 + freed.add(localId);
539 + Scheduler.log('Will unmount: ' + letter + localId);
540 + };
541 + }, [letter]);
542 + }
543 +
544 + const root = ReactDOMClient.createRoot(container);
545 + await act(() => {
546 + root.render(<App />);
547 + });
548 + assertLog(['Did mount: A0']);
549 +
550 + await act(() => {
551 + setStep(1);
552 + setSuspended(false);
553 + });
554 + assertLog([]);
555 +
556 + await act(() => {
557 + setStep(1);
558 + });
559 + assertLog([]);
560 +
561 + await act(() => {
562 + setSuspended(true);
563 + });
564 + assertLog(['Will unmount: A0']);
565 +
566 + await act(() => {
567 + setSuspended(false);
568 + setLetter('B');
569 + });
570 + assertLog(['Did mount: B1']);
571 +
572 + await act(() => {
573 + root.unmount();
574 + });
575 + assertLog(['Will unmount: B1']);
576 + });
577 });