Skip to content

Commit b7f5802

Browse files
acdliteacdlite
committed
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 [85bb7b6](85bb7b6)
1 parent 267d109 commit b7f5802

18 files changed

+684
-450
lines changed

‎compiled/facebook-www/REVISION

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
9cfba0f6ecc15a342ef184552742c5db6c7f2d3e
1+
85bb7b685b7aec50879703b94dd31523cf69b34d

‎compiled/facebook-www/React-dev.modern.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ if (
2727
}
2828
"use strict";
2929

30-
var ReactVersion = "18.3.0-www-modern-ba4b0f21";
30+
var ReactVersion = "18.3.0-www-modern-81d6a6e4";
3131

3232
// ATTENTION
3333
// When adding new symbols to this file,

‎compiled/facebook-www/ReactART-dev.classic.js

Lines changed: 45 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ function _assertThisInitialized(self) {
6969
return self;
7070
}
7171

72-
var ReactVersion = "18.3.0-www-classic-d71a1ab4";
72+
var ReactVersion = "18.3.0-www-classic-aefca35b";
7373

7474
var LegacyRoot = 0;
7575
var ConcurrentRoot = 1;
@@ -7341,7 +7341,21 @@ var didWarnAboutUseWrappedInTryCatch;
73417341
{
73427342
didWarnAboutMismatchedHooksForComponent = new Set();
73437343
didWarnAboutUseWrappedInTryCatch = new Set();
7344-
} // These are set right before calling the component.
7344+
} // The effect "instance" is a shared object that remains the same for the entire
7345+
// lifetime of an effect. In Rust terms, a RefCell. We use it to store the
7346+
// "destroy" function that is returned from an effect, because that is stateful.
7347+
// The field is `undefined` if the effect is unmounted, or if the effect ran
7348+
// but is not stateful. We don't explicitly track whether the effect is mounted
7349+
// or unmounted because that can be inferred by the hiddenness of the fiber in
7350+
// the tree, i.e. whether there is a hidden Offscreen fiber above it.
7351+
//
7352+
// It's unfortunate that this is stored on a separate object, because it adds
7353+
// more memory per effect instance, but it's conceptually sound. I think there's
7354+
// likely a better data structure we could use for effects; perhaps just one
7355+
// array of effect instances per fiber. But I think this is OK for now despite
7356+
// the additional memory and we can follow up with performance
7357+
// optimizations later.
7358+
// These are set right before calling the component.
73457359

73467360
var renderLanes$1 = NoLanes; // The work-in-progress fiber. I've named it differently to distinguish it from
73477361
// the work-in-progress hook.
@@ -8650,7 +8664,7 @@ function mountSyncExternalStore(subscribe, getSnapshot, getServerSnapshot) {
86508664
pushEffect(
86518665
HasEffect | Passive,
86528666
updateStoreInstance.bind(null, fiber, inst, nextSnapshot, getSnapshot),
8653-
undefined,
8667+
createEffectInstance(),
86548668
null
86558669
);
86568670
return nextSnapshot;
@@ -8705,7 +8719,7 @@ function updateSyncExternalStore(subscribe, getSnapshot, getServerSnapshot) {
87058719
pushEffect(
87068720
HasEffect | Passive,
87078721
updateStoreInstance.bind(null, fiber, inst, nextSnapshot, getSnapshot),
8708-
undefined,
8722+
createEffectInstance(),
87098723
null
87108724
); // Unless we're rendering a blocking lane, schedule a consistency check.
87118725
// Right before committing, we will walk the tree and check if any of the
@@ -8830,11 +8844,11 @@ function rerenderState(initialState) {
88308844
return rerenderReducer(basicStateReducer);
88318845
}
88328846

8833-
function pushEffect(tag, create, destroy, deps) {
8847+
function pushEffect(tag, create, inst, deps) {
88348848
var effect = {
88358849
tag: tag,
88368850
create: create,
8837-
destroy: destroy,
8851+
inst: inst,
88388852
deps: deps,
88398853
// Circular
88408854
next: null
@@ -8861,6 +8875,12 @@ function pushEffect(tag, create, destroy, deps) {
88618875
return effect;
88628876
}
88638877

8878+
function createEffectInstance() {
8879+
return {
8880+
destroy: undefined
8881+
};
8882+
}
8883+
88648884
var stackContainsErrorMessage = null;
88658885

88668886
function getCallerStackFrame() {
@@ -8961,25 +8981,24 @@ function mountEffectImpl(fiberFlags, hookFlags, create, deps) {
89618981
hook.memoizedState = pushEffect(
89628982
HasEffect | hookFlags,
89638983
create,
8964-
undefined,
8984+
createEffectInstance(),
89658985
nextDeps
89668986
);
89678987
}
89688988

89698989
function updateEffectImpl(fiberFlags, hookFlags, create, deps) {
89708990
var hook = updateWorkInProgressHook();
89718991
var nextDeps = deps === undefined ? null : deps;
8972-
var destroy = undefined; // currentHook is null when rerendering after a render phase state update.
8992+
var effect = hook.memoizedState;
8993+
var inst = effect.inst; // currentHook is null when rerendering after a render phase state update.
89738994

89748995
if (currentHook !== null) {
8975-
var prevEffect = currentHook.memoizedState;
8976-
destroy = prevEffect.destroy;
8977-
89788996
if (nextDeps !== null) {
8997+
var prevEffect = currentHook.memoizedState;
89798998
var prevDeps = prevEffect.deps;
89808999

89819000
if (areHookInputsEqual(nextDeps, prevDeps)) {
8982-
hook.memoizedState = pushEffect(hookFlags, create, destroy, nextDeps);
9001+
hook.memoizedState = pushEffect(hookFlags, create, inst, nextDeps);
89839002
return;
89849003
}
89859004
}
@@ -8989,7 +9008,7 @@ function updateEffectImpl(fiberFlags, hookFlags, create, deps) {
89899008
hook.memoizedState = pushEffect(
89909009
HasEffect | hookFlags,
89919010
create,
8992-
destroy,
9011+
inst,
89939012
nextDeps
89949013
);
89959014
}
@@ -19293,10 +19312,12 @@ function commitHookEffectListUnmount(
1929319312
do {
1929419313
if ((effect.tag & flags) === flags) {
1929519314
// Unmount
19296-
var destroy = effect.destroy;
19297-
effect.destroy = undefined;
19315+
var inst = effect.inst;
19316+
var destroy = inst.destroy;
1929819317

1929919318
if (destroy !== undefined) {
19319+
inst.destroy = undefined;
19320+
1930019321
if (enableSchedulingProfiler) {
1930119322
if ((flags & Passive) !== NoFlags) {
1930219323
markComponentPassiveEffectUnmountStarted(finishedWork);
@@ -19360,7 +19381,9 @@ function commitHookEffectListMount(flags, finishedWork) {
1936019381
}
1936119382
}
1936219383

19363-
effect.destroy = create();
19384+
var inst = effect.inst;
19385+
var destroy = create();
19386+
inst.destroy = destroy;
1936419387

1936519388
{
1936619389
if ((flags & Insertion) !== NoFlags) {
@@ -19377,8 +19400,6 @@ function commitHookEffectListMount(flags, finishedWork) {
1937719400
}
1937819401

1937919402
{
19380-
var destroy = effect.destroy;
19381-
1938219403
if (destroy !== undefined && typeof destroy !== "function") {
1938319404
var hookName = void 0;
1938419405

@@ -20756,12 +20777,13 @@ function commitDeletionEffectsOnFiber(
2075620777
var effect = firstEffect;
2075720778

2075820779
do {
20759-
var _effect = effect,
20760-
destroy = _effect.destroy,
20761-
tag = _effect.tag;
20780+
var tag = effect.tag;
20781+
var inst = effect.inst;
20782+
var destroy = inst.destroy;
2076220783

2076320784
if (destroy !== undefined) {
2076420785
if ((tag & Insertion) !== NoFlags) {
20786+
inst.destroy = undefined;
2076520787
safelyCallDestroy(
2076620788
deletedFiber,
2076720789
nearestMountedAncestor,
@@ -20774,13 +20796,15 @@ function commitDeletionEffectsOnFiber(
2077420796

2077520797
if (shouldProfile(deletedFiber)) {
2077620798
startLayoutEffectTimer();
20799+
inst.destroy = undefined;
2077720800
safelyCallDestroy(
2077820801
deletedFiber,
2077920802
nearestMountedAncestor,
2078020803
destroy
2078120804
);
2078220805
recordLayoutEffectDuration(deletedFiber);
2078320806
} else {
20807+
inst.destroy = undefined;
2078420808
safelyCallDestroy(
2078520809
deletedFiber,
2078620810
nearestMountedAncestor,

‎compiled/facebook-www/ReactART-dev.modern.js

Lines changed: 45 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ function _assertThisInitialized(self) {
6969
return self;
7070
}
7171

72-
var ReactVersion = "18.3.0-www-modern-2104d0e4";
72+
var ReactVersion = "18.3.0-www-modern-c67d4626";
7373

7474
var LegacyRoot = 0;
7575
var ConcurrentRoot = 1;
@@ -7097,7 +7097,21 @@ var didWarnAboutUseWrappedInTryCatch;
70977097
{
70987098
didWarnAboutMismatchedHooksForComponent = new Set();
70997099
didWarnAboutUseWrappedInTryCatch = new Set();
7100-
} // These are set right before calling the component.
7100+
} // The effect "instance" is a shared object that remains the same for the entire
7101+
// lifetime of an effect. In Rust terms, a RefCell. We use it to store the
7102+
// "destroy" function that is returned from an effect, because that is stateful.
7103+
// The field is `undefined` if the effect is unmounted, or if the effect ran
7104+
// but is not stateful. We don't explicitly track whether the effect is mounted
7105+
// or unmounted because that can be inferred by the hiddenness of the fiber in
7106+
// the tree, i.e. whether there is a hidden Offscreen fiber above it.
7107+
//
7108+
// It's unfortunate that this is stored on a separate object, because it adds
7109+
// more memory per effect instance, but it's conceptually sound. I think there's
7110+
// likely a better data structure we could use for effects; perhaps just one
7111+
// array of effect instances per fiber. But I think this is OK for now despite
7112+
// the additional memory and we can follow up with performance
7113+
// optimizations later.
7114+
// These are set right before calling the component.
71017115

71027116
var renderLanes$1 = NoLanes; // The work-in-progress fiber. I've named it differently to distinguish it from
71037117
// the work-in-progress hook.
@@ -8406,7 +8420,7 @@ function mountSyncExternalStore(subscribe, getSnapshot, getServerSnapshot) {
84068420
pushEffect(
84078421
HasEffect | Passive,
84088422
updateStoreInstance.bind(null, fiber, inst, nextSnapshot, getSnapshot),
8409-
undefined,
8423+
createEffectInstance(),
84108424
null
84118425
);
84128426
return nextSnapshot;
@@ -8461,7 +8475,7 @@ function updateSyncExternalStore(subscribe, getSnapshot, getServerSnapshot) {
84618475
pushEffect(
84628476
HasEffect | Passive,
84638477
updateStoreInstance.bind(null, fiber, inst, nextSnapshot, getSnapshot),
8464-
undefined,
8478+
createEffectInstance(),
84658479
null
84668480
); // Unless we're rendering a blocking lane, schedule a consistency check.
84678481
// Right before committing, we will walk the tree and check if any of the
@@ -8586,11 +8600,11 @@ function rerenderState(initialState) {
85868600
return rerenderReducer(basicStateReducer);
85878601
}
85888602

8589-
function pushEffect(tag, create, destroy, deps) {
8603+
function pushEffect(tag, create, inst, deps) {
85908604
var effect = {
85918605
tag: tag,
85928606
create: create,
8593-
destroy: destroy,
8607+
inst: inst,
85948608
deps: deps,
85958609
// Circular
85968610
next: null
@@ -8617,6 +8631,12 @@ function pushEffect(tag, create, destroy, deps) {
86178631
return effect;
86188632
}
86198633

8634+
function createEffectInstance() {
8635+
return {
8636+
destroy: undefined
8637+
};
8638+
}
8639+
86208640
var stackContainsErrorMessage = null;
86218641

86228642
function getCallerStackFrame() {
@@ -8717,25 +8737,24 @@ function mountEffectImpl(fiberFlags, hookFlags, create, deps) {
87178737
hook.memoizedState = pushEffect(
87188738
HasEffect | hookFlags,
87198739
create,
8720-
undefined,
8740+
createEffectInstance(),
87218741
nextDeps
87228742
);
87238743
}
87248744

87258745
function updateEffectImpl(fiberFlags, hookFlags, create, deps) {
87268746
var hook = updateWorkInProgressHook();
87278747
var nextDeps = deps === undefined ? null : deps;
8728-
var destroy = undefined; // currentHook is null when rerendering after a render phase state update.
8748+
var effect = hook.memoizedState;
8749+
var inst = effect.inst; // currentHook is null when rerendering after a render phase state update.
87298750

87308751
if (currentHook !== null) {
8731-
var prevEffect = currentHook.memoizedState;
8732-
destroy = prevEffect.destroy;
8733-
87348752
if (nextDeps !== null) {
8753+
var prevEffect = currentHook.memoizedState;
87358754
var prevDeps = prevEffect.deps;
87368755

87378756
if (areHookInputsEqual(nextDeps, prevDeps)) {
8738-
hook.memoizedState = pushEffect(hookFlags, create, destroy, nextDeps);
8757+
hook.memoizedState = pushEffect(hookFlags, create, inst, nextDeps);
87398758
return;
87408759
}
87418760
}
@@ -8745,7 +8764,7 @@ function updateEffectImpl(fiberFlags, hookFlags, create, deps) {
87458764
hook.memoizedState = pushEffect(
87468765
HasEffect | hookFlags,
87478766
create,
8748-
destroy,
8767+
inst,
87498768
nextDeps
87508769
);
87518770
}
@@ -18958,10 +18977,12 @@ function commitHookEffectListUnmount(
1895818977
do {
1895918978
if ((effect.tag & flags) === flags) {
1896018979
// Unmount
18961-
var destroy = effect.destroy;
18962-
effect.destroy = undefined;
18980+
var inst = effect.inst;
18981+
var destroy = inst.destroy;
1896318982

1896418983
if (destroy !== undefined) {
18984+
inst.destroy = undefined;
18985+
1896518986
if (enableSchedulingProfiler) {
1896618987
if ((flags & Passive) !== NoFlags) {
1896718988
markComponentPassiveEffectUnmountStarted(finishedWork);
@@ -19025,7 +19046,9 @@ function commitHookEffectListMount(flags, finishedWork) {
1902519046
}
1902619047
}
1902719048

19028-
effect.destroy = create();
19049+
var inst = effect.inst;
19050+
var destroy = create();
19051+
inst.destroy = destroy;
1902919052

1903019053
{
1903119054
if ((flags & Insertion) !== NoFlags) {
@@ -19042,8 +19065,6 @@ function commitHookEffectListMount(flags, finishedWork) {
1904219065
}
1904319066

1904419067
{
19045-
var destroy = effect.destroy;
19046-
1904719068
if (destroy !== undefined && typeof destroy !== "function") {
1904819069
var hookName = void 0;
1904919070

@@ -20421,12 +20442,13 @@ function commitDeletionEffectsOnFiber(
2042120442
var effect = firstEffect;
2042220443

2042320444
do {
20424-
var _effect = effect,
20425-
destroy = _effect.destroy,
20426-
tag = _effect.tag;
20445+
var tag = effect.tag;
20446+
var inst = effect.inst;
20447+
var destroy = inst.destroy;
2042720448

2042820449
if (destroy !== undefined) {
2042920450
if ((tag & Insertion) !== NoFlags) {
20451+
inst.destroy = undefined;
2043020452
safelyCallDestroy(
2043120453
deletedFiber,
2043220454
nearestMountedAncestor,
@@ -20439,13 +20461,15 @@ function commitDeletionEffectsOnFiber(
2043920461

2044020462
if (shouldProfile(deletedFiber)) {
2044120463
startLayoutEffectTimer();
20464+
inst.destroy = undefined;
2044220465
safelyCallDestroy(
2044320466
deletedFiber,
2044420467
nearestMountedAncestor,
2044520468
destroy
2044620469
);
2044720470
recordLayoutEffectDuration(deletedFiber);
2044820471
} else {
20472+
inst.destroy = undefined;
2044920473
safelyCallDestroy(
2045020474
deletedFiber,
2045120475
nearestMountedAncestor,

0 commit comments

Comments
 (0)