Skip to content

Commit

Permalink
Fix: Move destroy field to shared instance object (#26561)
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@acdlite @gaearon
@rickhanlonii @kassens
4 people authored Apr 6, 2023
1 parent 60cfeee commit 85bb7b6
Show file tree
Hide file tree
Showing 3 changed files with 125 additions and 18 deletions.
17 changes: 12 additions & 5 deletions packages/react-reconciler/src/ReactFiberCommitWork.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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) {
Expand Down Expand Up @@ -2188,9 +2190,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,
Expand All @@ -2203,13 +2208,15 @@ function commitDeletionEffectsOnFiber(

if (shouldProfile(deletedFiber)) {
startLayoutEffectTimer();
inst.destroy = undefined;
safelyCallDestroy(
deletedFiber,
nearestMountedAncestor,
destroy,
);
recordLayoutEffectDuration(deletedFiber);
} else {
inst.destroy = undefined;
safelyCallDestroy(
deletedFiber,
nearestMountedAncestor,
Expand Down
48 changes: 35 additions & 13 deletions packages/react-reconciler/src/ReactFiberHooks.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand Down Expand Up @@ -1662,7 +1680,7 @@ function mountSyncExternalStore<T>(
pushEffect(
HookHasEffect | HookPassive,
updateStoreInstance.bind(null, fiber, inst, nextSnapshot, getSnapshot),
undefined,
createEffectInstance(),
null,
);

Expand Down Expand Up @@ -1719,7 +1737,7 @@ function updateSyncExternalStore<T>(
pushEffect(
HookHasEffect | HookPassive,
updateStoreInstance.bind(null, fiber, inst, nextSnapshot, getSnapshot),
undefined,
createEffectInstance(),
null,
);

Expand Down Expand Up @@ -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),
Expand All @@ -1891,6 +1909,10 @@ function pushEffect(
return effect;
}

function createEffectInstance(): EffectInstance {
return {destroy: undefined};
}

let stackContainsErrorMessage: boolean | null = null;

function getCallerStackFrame(): string {
Expand Down Expand Up @@ -1994,7 +2016,7 @@ function mountEffectImpl(
hook.memoizedState = pushEffect(
HookHasEffect | hookFlags,
create,
undefined,
createEffectInstance(),
nextDeps,
);
}
Expand All @@ -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;
}
}
Expand All @@ -2027,7 +2049,7 @@ function updateEffectImpl(
hook.memoizedState = pushEffect(
HookHasEffect | hookFlags,
create,
destroy,
inst,
nextDeps,
);
}
Expand Down
78 changes: 78 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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']);
});
});

0 comments on commit 85bb7b6

Please sign in to comment.