From c3b18571dbf27b47675619aefc9cc9a4a06dbe18 Mon Sep 17 00:00:00 2001
From: Luna Ruan <lunaris.ruan@gmail.com>
Date: Thu, 7 Jul 2022 10:43:25 -0400
Subject: [PATCH] [DevTools][Bugfix] Fix DevTools Perf Issue When Unmounting
 Large React Subtrees (#24863)

We've recently had multiple reports where, if React DevTools was installed, unmounting large React subtrees would take a huge performance hit (ex. from 50ms to 7 seconds).

Digging in more, we realized for every fiber that unmounts, we called `untrackFibers`, which calls `clearTimeout` (and does some work manipulating a set, but this wasn't the bulk of the time). We ten call `recordUnmount`, which adds the timer back. Adding and removing the timer so many times was taking upwards of 50ms per timer add/remove call, which was resulting in exorbitant amounts of time spent in DevTools deleting subtrees.

It looks like we are calling `untrackFibers` so many times to avoid a race condition with Suspense children where we unmount them twice (first a "virtual" unmount when the suspense boundary is toggled from visible to invisible, and then an actual unmount when the new children are rendered) without modifying `fiberIDMap`. We can fix this race condition by using the `untrackFibersSet` as a lock and not calling `recordUnmount` if the fiber is in the set and hasn't been processed yet. This works because the only way fibers are added in the set is via `recordUnmount` anyway.

This PR also adds a test to make sure this change doesn't regress the previous behavior.

**Before**
![image](https://user-images.githubusercontent.com/2735514/177655428-774ee306-0568-49ce-987e-b5213b613265.png)

**After**
![image](https://user-images.githubusercontent.com/2735514/177655604-a217583f-787e-438e-b6f9-18953fe32444.png)
---
 .../src/__tests__/store-test.js               | 44 +++++++++++++++++++
 .../src/backend/renderer.js                   | 17 +++----
 2 files changed, 53 insertions(+), 8 deletions(-)

diff --git a/packages/react-devtools-shared/src/__tests__/store-test.js b/packages/react-devtools-shared/src/__tests__/store-test.js
index 56795d232895c..a6dc02f844e5f 100644
--- a/packages/react-devtools-shared/src/__tests__/store-test.js
+++ b/packages/react-devtools-shared/src/__tests__/store-test.js
@@ -13,6 +13,7 @@ describe('Store', () => {
   let ReactDOMClient;
   let agent;
   let act;
+  let actAsync;
   let bridge;
   let getRendererID;
   let legacyRender;
@@ -30,6 +31,7 @@ describe('Store', () => {
 
     const utils = require('./utils');
     act = utils.act;
+    actAsync = utils.actAsync;
     getRendererID = utils.getRendererID;
     legacyRender = utils.legacyRender;
     withErrorsOrWarningsIgnored = utils.withErrorsOrWarningsIgnored;
@@ -2064,5 +2066,47 @@ describe('Store', () => {
       expect(store.errorCount).toBe(0);
       expect(store.warningCount).toBe(0);
     });
+
+    // Regression test for https://github.com/facebook/react/issues/23202
+    // @reactVersion >= 18.0
+    it('suspense boundary children should not double unmount and error', async () => {
+      async function fakeImport(result) {
+        return {default: result};
+      }
+
+      const ChildA = () => null;
+      const ChildB = () => null;
+
+      const LazyChildA = React.lazy(() => fakeImport(ChildA));
+      const LazyChildB = React.lazy(() => fakeImport(ChildB));
+
+      function App({renderA}) {
+        return (
+          <React.Suspense>
+            {renderA ? <LazyChildA /> : <LazyChildB />}
+          </React.Suspense>
+        );
+      }
+
+      const container = document.createElement('div');
+      const root = ReactDOMClient.createRoot(container);
+      await actAsync(() => root.render(<App renderA={true} />));
+
+      expect(store).toMatchInlineSnapshot(`
+          [root]
+            ▾ <App>
+              ▾ <Suspense>
+                  <ChildA>
+        `);
+
+      await actAsync(() => root.render(<App renderA={false} />));
+
+      expect(store).toMatchInlineSnapshot(`
+          [root]
+            ▾ <App>
+              ▾ <Suspense>
+                  <ChildB>
+        `);
+    });
   });
 });
diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js
index 9959cad7aa525..f2d6dad45f002 100644
--- a/packages/react-devtools-shared/src/backend/renderer.js
+++ b/packages/react-devtools-shared/src/backend/renderer.js
@@ -2632,14 +2632,15 @@ export function attach(
   }
 
   function handleCommitFiberUnmount(fiber) {
-    // Flush any pending Fibers that we are untracking before processing the new commit.
-    // If we don't do this, we might end up double-deleting Fibers in some cases (like Legacy Suspense).
-    untrackFibers();
-
-    // This is not recursive.
-    // We can't traverse fibers after unmounting so instead
-    // we rely on React telling us about each unmount.
-    recordUnmount(fiber, false);
+    // If the untrackFiberSet already has the unmounted Fiber, this means we've already
+    // recordedUnmount, so we don't need to do it again. If we don't do this, we might
+    // end up double-deleting Fibers in some cases (like Legacy Suspense).
+    if (!untrackFibersSet.has(fiber)) {
+      // This is not recursive.
+      // We can't traverse fibers after unmounting so instead
+      // we rely on React telling us about each unmount.
+      recordUnmount(fiber, false);
+    }
   }
 
   function handlePostCommitFiberRoot(root) {