Skip to content

Commit

Permalink
Warn if commit phase error thrown in detached tree (#20286)
Browse files Browse the repository at this point in the history
Until `skipUnmountedBoundaries` lands again, we need some way to detect
when errors are thrown inside a deleted tree. I've added a warning to
`captureCommitPhaseError` that fires when we reach the root of a subtree
without finding either a boundary or a HostRoot.

Even after `skipUnmountedBoundaries`  lands, this warning could be a
useful guard against internal bugs, like a bug in the
`skipUnmountedBoundaries` implementation itself.

In the meantime, do not add this warning to the allowlist; this is only
for our internal use. For this reason, I've also only added it to the
new fork, not the old one, to prevent this from accidentally leaking
into the open source build.
  • Loading branch information
acdlite committed Nov 18, 2020
1 parent 0f83a64 commit ac2cff4
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 16 deletions.
Expand Up @@ -1669,16 +1669,28 @@ describe('DOMPluginEventSystem', () => {

function Test() {
React.useEffect(() => {
setClick1(buttonRef.current, targetListener1);
setClick2(buttonRef.current, targetListener2);
setClick3(buttonRef.current, targetListener3);
setClick4(buttonRef.current, targetListener4);
const clearClick1 = setClick1(
buttonRef.current,
targetListener1,
);
const clearClick2 = setClick2(
buttonRef.current,
targetListener2,
);
const clearClick3 = setClick3(
buttonRef.current,
targetListener3,
);
const clearClick4 = setClick4(
buttonRef.current,
targetListener4,
);

return () => {
setClick1();
setClick2();
setClick3();
setClick4();
clearClick1();
clearClick2();
clearClick3();
clearClick4();
};
});

Expand All @@ -1703,16 +1715,28 @@ describe('DOMPluginEventSystem', () => {

function Test2() {
React.useEffect(() => {
setClick1(buttonRef.current, targetListener1);
setClick2(buttonRef.current, targetListener2);
setClick3(buttonRef.current, targetListener3);
setClick4(buttonRef.current, targetListener4);
const clearClick1 = setClick1(
buttonRef.current,
targetListener1,
);
const clearClick2 = setClick2(
buttonRef.current,
targetListener2,
);
const clearClick3 = setClick3(
buttonRef.current,
targetListener3,
);
const clearClick4 = setClick4(
buttonRef.current,
targetListener4,
);

return () => {
setClick1();
setClick2();
setClick3();
setClick4();
clearClick1();
clearClick2();
clearClick3();
clearClick4();
};
});

Expand Down
16 changes: 16 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Expand Up @@ -2844,6 +2844,22 @@ export function captureCommitPhaseError(sourceFiber: Fiber, error: mixed) {
}
fiber = fiber.return;
}

if (__DEV__) {
// TODO: Until we re-land skipUnmountedBoundaries (see #20147), this warning
// will fire for errors that are thrown by destroy functions inside deleted
// trees. What it should instead do is propagate the error to the parent of
// the deleted tree. In the meantime, do not add this warning to the
// allowlist; this is only for our internal use.
console.error(
'Internal React error: Attempted to capture a commit phase error ' +
'inside a detached tree. This indicates a bug in React. Likely ' +
'causes include deleting the same fiber more than once, committing an ' +
'already-finished tree, or an inconsistent return pointer.\n\n' +
'Error message:\n\n%s',
error,
);
}
}

export function pingSuspendedRoot(
Expand Down

0 comments on commit ac2cff4

Please sign in to comment.