From d410f0a1bbb12e972b0e99bb9faea10c7e62894d Mon Sep 17 00:00:00 2001
From: Andrew Clark <git@andrewclark.io>
Date: Wed, 15 Jun 2022 21:12:46 -0400
Subject: [PATCH] [FORKED] Bugfix: Offscreen instance is null during setState

During a setState, we traverse up the return path and check if any
parent Offscreen components are currently hidden. To do that, we must
access the Offscreen fiber's `stateNode` field.

On a mounted Offscreen fiber, the `stateNode` is never null, so usually
we don't need to refine the type. When a fiber is unmounted, though,
we null out its `stateNode` field to prevent memory cycles. However,
we also null out its `return` field, so I had assumed that an unmounted
Offscreen fiber would never be reachable.

What I didn't consider is that it's possible to call `setState` on a
fiber that never finished mounting. Because it never mounted, it was
never deleted. Because it was never deleted, its `return` field was
never disconnected.

This pattern is always accompanied by a warning but we still need to
account for it. There may also be other patterns where an unmounted
Offscreen instance is reachable, too.

The discovery also suggests it may be better for memory
usage if we don't attach the `return` pointer until the commit phase,
though in order to do that we'd need some other way to track the return
pointer during initial render, like on the stack.

The fix is to add a null check before reading the instance
during setState.
---
 .../src/ReactFiberConcurrentUpdates.new.js    | 21 +++++++-
 .../src/__tests__/ReactOffscreen-test.js      | 51 +++++++++++++++++++
 2 files changed, 70 insertions(+), 2 deletions(-)

diff --git a/packages/react-reconciler/src/ReactFiberConcurrentUpdates.new.js b/packages/react-reconciler/src/ReactFiberConcurrentUpdates.new.js
index 74e19f40b4982..4114da9dd226c 100644
--- a/packages/react-reconciler/src/ReactFiberConcurrentUpdates.new.js
+++ b/packages/react-reconciler/src/ReactFiberConcurrentUpdates.new.js
@@ -193,8 +193,25 @@ function markUpdateLaneFromFiberToRoot(
     }
 
     if (parent.tag === OffscreenComponent) {
-      const offscreenInstance: OffscreenInstance = parent.stateNode;
-      if (offscreenInstance.isHidden) {
+      // Check if this offscreen boundary is currently hidden.
+      //
+      // The instance may be null if the Offscreen parent was unmounted. Usually
+      // the parent wouldn't be reachable in that case because we disconnect
+      // fibers from the tree when they are deleted. However, there's a weird
+      // edge case where setState is called on a fiber that was interrupted
+      // before it ever mounted. Because it never mounts, it also never gets
+      // deleted. Because it never gets deleted, its return pointer never gets
+      // disconnected. Which means it may be attached to a deleted Offscreen
+      // parent node. (This discovery suggests it may be better for memory usage
+      // if we don't attach the `return` pointer until the commit phase, though
+      // in order to do that we'd need some other way to track the return
+      // pointer during the initial render, like on the stack.)
+      //
+      // This case is always accompanied by a warning, but we still need to
+      // account for it. (There may be other cases that we haven't discovered,
+      // too.)
+      const offscreenInstance: OffscreenInstance | null = parent.stateNode;
+      if (offscreenInstance !== null && offscreenInstance.isHidden) {
         isHidden = true;
       }
     }
diff --git a/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js b/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
index 0f9fd6dd5b217..bed5691426a4a 100644
--- a/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
+++ b/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
@@ -7,6 +7,7 @@ let Offscreen;
 let useState;
 let useLayoutEffect;
 let useEffect;
+let startTransition;
 
 describe('ReactOffscreen', () => {
   beforeEach(() => {
@@ -21,6 +22,7 @@ describe('ReactOffscreen', () => {
     useState = React.useState;
     useLayoutEffect = React.useLayoutEffect;
     useEffect = React.useEffect;
+    startTransition = React.startTransition;
   });
 
   function Text(props) {
@@ -591,4 +593,53 @@ describe('ReactOffscreen', () => {
       </>,
     );
   });
+
+  // TODO: Create TestFlag alias for Offscreen
+  // @gate experimental || www
+  it('regression: Offscreen instance is sometimes null during setState', async () => {
+    let setState;
+    function Child() {
+      const [state, _setState] = useState('Initial');
+      setState = _setState;
+      return <Text text={state} />;
+    }
+
+    const root = ReactNoop.createRoot();
+    await act(async () => {
+      root.render(<Offscreen hidden={false} />);
+    });
+    expect(Scheduler).toHaveYielded([]);
+    expect(root).toMatchRenderedOutput(null);
+
+    await act(async () => {
+      // Partially render a component
+      startTransition(() => {
+        root.render(
+          <Offscreen hidden={false}>
+            <Child />
+            <Text text="Sibling" />
+          </Offscreen>,
+        );
+      });
+      expect(Scheduler).toFlushAndYieldThrough(['Initial']);
+
+      // Before it finishes rendering, the whole tree gets deleted
+      ReactNoop.flushSync(() => {
+        root.render(null);
+      });
+
+      // Something attempts to update the never-mounted component. When this
+      // regression test was written, we would walk up the component's return
+      // path and reach an unmounted Offscreen component fiber. Its `stateNode`
+      // would be null because it was nulled out when it was deleted, but there
+      // was no null check before we accessed it. A weird edge case but we must
+      // account for it.
+      expect(() => {
+        setState('Updated');
+      }).toErrorDev(
+        "Can't perform a React state update on a component that hasn't mounted yet",
+      );
+    });
+    expect(root).toMatchRenderedOutput(null);
+  });
 });