From ac2cff4b10624b87b14333d9b7c5d41a51fac0ec Mon Sep 17 00:00:00 2001
From: Andrew Clark <git@andrewclark.io>
Date: Wed, 18 Nov 2020 11:23:39 -0600
Subject: [PATCH] Warn if commit phase error thrown in detached tree (#20286)

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.
---
 .../DOMPluginEventSystem-test.internal.js     | 56 +++++++++++++------
 .../src/ReactFiberWorkLoop.new.js             | 16 ++++++
 2 files changed, 56 insertions(+), 16 deletions(-)

diff --git a/packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js b/packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js
index 8bf16bc94255c..004f0bb631f31 100644
--- 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
index f7a4f39bb1cba..0f881d853426c 100644
--- 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(