Skip to content

Commit

Permalink
Filter out deleted components that are added to the updaters list (fa…
Browse files Browse the repository at this point in the history
…cebook#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)
  • Loading branch information
lunaruan authored and zhengjitf committed Apr 15, 2022
1 parent 18a79e4 commit d80b761
Show file tree
Hide file tree
Showing 2 changed files with 96 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -752,4 +752,97 @@ describe('ProfilingCache', () => {
utils.act(() => store.profilerStore.stopProfiling());
expect(container.textContent).toBe('About');
});

it('components that were deleted and added to updaters during the layout phase should not crash', () => {
let setChildUnmounted;
function Child() {
const [, setState] = React.useState(false);

React.useLayoutEffect(() => {
return () => setState(true);
});

return null;
}

function App() {
const [childUnmounted, _setChildUnmounted] = React.useState(false);
setChildUnmounted = _setChildUnmounted;
return <>{!childUnmounted && <Child />}</>;
}

const root = ReactDOM.createRoot(document.createElement('div'));
utils.act(() => root.render(<App />));
utils.act(() => store.profilerStore.startProfiling());
utils.act(() => setChildUnmounted(true));
utils.act(() => store.profilerStore.stopProfiling());

const updaters = store.profilerStore.getCommitData(store.roots[0], 0)
.updaters;
expect(updaters.length).toEqual(1);
expect(updaters[0].displayName).toEqual('App');
});

it('components in a deleted subtree and added to updaters during the layout phase should not crash', () => {
let setChildUnmounted;
function Child() {
return <GrandChild />;
}

function GrandChild() {
const [, setState] = React.useState(false);

React.useLayoutEffect(() => {
return () => setState(true);
});

return null;
}

function App() {
const [childUnmounted, _setChildUnmounted] = React.useState(false);
setChildUnmounted = _setChildUnmounted;
return <>{!childUnmounted && <Child />}</>;
}

const root = ReactDOM.createRoot(document.createElement('div'));
utils.act(() => root.render(<App />));
utils.act(() => store.profilerStore.startProfiling());
utils.act(() => setChildUnmounted(true));
utils.act(() => store.profilerStore.stopProfiling());

const updaters = store.profilerStore.getCommitData(store.roots[0], 0)
.updaters;
expect(updaters.length).toEqual(1);
expect(updaters[0].displayName).toEqual('App');
});

it('components that were deleted should not be added to updaters during the passive phase', () => {
let setChildUnmounted;
function Child() {
const [, setState] = React.useState(false);
React.useEffect(() => {
return () => setState(true);
});

return null;
}

function App() {
const [childUnmounted, _setChildUnmounted] = React.useState(false);
setChildUnmounted = _setChildUnmounted;
return <>{!childUnmounted && <Child />}</>;
}

const root = ReactDOM.createRoot(document.createElement('div'));
utils.act(() => root.render(<App />));
utils.act(() => store.profilerStore.startProfiling());
utils.act(() => setChildUnmounted(true));
utils.act(() => store.profilerStore.stopProfiling());

const updaters = store.profilerStore.getCommitData(store.roots[0], 0)
.updaters;
expect(updaters.length).toEqual(1);
expect(updaters[0].displayName).toEqual('App');
});
});
4 changes: 3 additions & 1 deletion packages/react-devtools-shared/src/backend/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -2586,7 +2586,9 @@ export function attach(

function getUpdatersList(root): Array<SerializedElement> | null {
return root.memoizedUpdaters != null
? Array.from(root.memoizedUpdaters).map(fiberToSerializedElement)
? Array.from(root.memoizedUpdaters)
.filter(fiber => getFiberIDUnsafe(fiber) !== null)
.map(fiberToSerializedElement)
: null;
}

Expand Down

0 comments on commit d80b761

Please sign in to comment.