Bugfix: Offscreen instance is null during setState (#24734)

acdlite · web-flow · commit c3d7a7e3d729 · 2022-06-15T22:13:22.000-04:00
* [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.

* Add previous commit to list of forked revisions
diff --git 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
@@ -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);
+  });
 });
diff --git a/scripts/merge-fork/forked-revisions b/scripts/merge-fork/forked-revisions
@@ -1,3 +1,4 @@
+d410f0a1bbb12e972b0e99bb9faea10c7e62894d [FORKED] Bugfix: Offscreen instance is null during setState
 58bb11764bf0bb6db47527a64f693f67cdd3b0bb [FORKED] Check for infinite update loops even if unmounted
 31882b5dd66f34f70d341ea2781cacbe802bf4d5 [FORKED] Bugfix: Revealing a hidden update
 17691acc071d56261d43c3cf183f287d983baa9b [FORKED] Don't update childLanes until after current render
