Warn if commit phase error thrown in detached tree (#20286)

acdlite · web-flow · commit ac2cff4b1062 · 2020-11-18T09:23:39.000-08:00
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.
diff --git a/packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js b/packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js
@@ -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();
                 };
               });
 
@@ -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();
                 };
               });
 
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -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(
