Skip to content

Commit 7bee137

Browse files
authored
Filter out deleted components that are added to the updaters list (#23156)
There was a bug that occurred when a destroy effect is called that causes an update. The update would be added to the updaters list even though the fiber that was calling the destroy effect was unmounted and no longer exists. This PR: * Adds a patch to Devtools to filter out all in the update list that aren't in the fiberToIDMap (which contains all fibers currently on screen)
1 parent a637084 commit 7bee137

File tree

2 files changed

+96
-1
lines changed

2 files changed

+96
-1
lines changed

packages/react-devtools-shared/src/__tests__/profilingCache-test.js

+93
Original file line numberDiff line numberDiff line change
@@ -752,4 +752,97 @@ describe('ProfilingCache', () => {
752752
utils.act(() => store.profilerStore.stopProfiling());
753753
expect(container.textContent).toBe('About');
754754
});
755+
756+
it('components that were deleted and added to updaters during the layout phase should not crash', () => {
757+
let setChildUnmounted;
758+
function Child() {
759+
const [, setState] = React.useState(false);
760+
761+
React.useLayoutEffect(() => {
762+
return () => setState(true);
763+
});
764+
765+
return null;
766+
}
767+
768+
function App() {
769+
const [childUnmounted, _setChildUnmounted] = React.useState(false);
770+
setChildUnmounted = _setChildUnmounted;
771+
return <>{!childUnmounted && <Child />}</>;
772+
}
773+
774+
const root = ReactDOM.createRoot(document.createElement('div'));
775+
utils.act(() => root.render(<App />));
776+
utils.act(() => store.profilerStore.startProfiling());
777+
utils.act(() => setChildUnmounted(true));
778+
utils.act(() => store.profilerStore.stopProfiling());
779+
780+
const updaters = store.profilerStore.getCommitData(store.roots[0], 0)
781+
.updaters;
782+
expect(updaters.length).toEqual(1);
783+
expect(updaters[0].displayName).toEqual('App');
784+
});
785+
786+
it('components in a deleted subtree and added to updaters during the layout phase should not crash', () => {
787+
let setChildUnmounted;
788+
function Child() {
789+
return <GrandChild />;
790+
}
791+
792+
function GrandChild() {
793+
const [, setState] = React.useState(false);
794+
795+
React.useLayoutEffect(() => {
796+
return () => setState(true);
797+
});
798+
799+
return null;
800+
}
801+
802+
function App() {
803+
const [childUnmounted, _setChildUnmounted] = React.useState(false);
804+
setChildUnmounted = _setChildUnmounted;
805+
return <>{!childUnmounted && <Child />}</>;
806+
}
807+
808+
const root = ReactDOM.createRoot(document.createElement('div'));
809+
utils.act(() => root.render(<App />));
810+
utils.act(() => store.profilerStore.startProfiling());
811+
utils.act(() => setChildUnmounted(true));
812+
utils.act(() => store.profilerStore.stopProfiling());
813+
814+
const updaters = store.profilerStore.getCommitData(store.roots[0], 0)
815+
.updaters;
816+
expect(updaters.length).toEqual(1);
817+
expect(updaters[0].displayName).toEqual('App');
818+
});
819+
820+
it('components that were deleted should not be added to updaters during the passive phase', () => {
821+
let setChildUnmounted;
822+
function Child() {
823+
const [, setState] = React.useState(false);
824+
React.useEffect(() => {
825+
return () => setState(true);
826+
});
827+
828+
return null;
829+
}
830+
831+
function App() {
832+
const [childUnmounted, _setChildUnmounted] = React.useState(false);
833+
setChildUnmounted = _setChildUnmounted;
834+
return <>{!childUnmounted && <Child />}</>;
835+
}
836+
837+
const root = ReactDOM.createRoot(document.createElement('div'));
838+
utils.act(() => root.render(<App />));
839+
utils.act(() => store.profilerStore.startProfiling());
840+
utils.act(() => setChildUnmounted(true));
841+
utils.act(() => store.profilerStore.stopProfiling());
842+
843+
const updaters = store.profilerStore.getCommitData(store.roots[0], 0)
844+
.updaters;
845+
expect(updaters.length).toEqual(1);
846+
expect(updaters[0].displayName).toEqual('App');
847+
});
755848
});

packages/react-devtools-shared/src/backend/renderer.js

+3-1
Original file line numberDiff line numberDiff line change
@@ -2586,7 +2586,9 @@ export function attach(
25862586

25872587
function getUpdatersList(root): Array<SerializedElement> | null {
25882588
return root.memoizedUpdaters != null
2589-
? Array.from(root.memoizedUpdaters).map(fiberToSerializedElement)
2589+
? Array.from(root.memoizedUpdaters)
2590+
.filter(fiber => getFiberIDUnsafe(fiber) !== null)
2591+
.map(fiberToSerializedElement)
25902592
: null;
25912593
}
25922594

0 commit comments

Comments
 (0)