@samitouri / QOS-React-2 / commits / c3d7a7e3d7

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

* [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

Andrew Clark committed Jun 15, 2022 at 22:13 UTC c3d7a7e3d72937443ef75b7e29335c98ad0f1424

3 files changed +71 -2

packages/react-reconciler/src/ReactFiberConcurrentUpdates.new.js

+19 -2

@@ -193,8 +193,25 @@ function markUpdateLaneFromFiberToRoot(

193 }

194

195 if (parent.tag === OffscreenComponent) {

196 - const offscreenInstance: OffscreenInstance = parent.stateNode;

197 - if (offscreenInstance.isHidden) {

196 + // Check if this offscreen boundary is currently hidden.

197 + //

198 + // The instance may be null if the Offscreen parent was unmounted. Usually

199 + // the parent wouldn't be reachable in that case because we disconnect

200 + // fibers from the tree when they are deleted. However, there's a weird

201 + // edge case where setState is called on a fiber that was interrupted

202 + // before it ever mounted. Because it never mounts, it also never gets

203 + // deleted. Because it never gets deleted, its return pointer never gets

204 + // disconnected. Which means it may be attached to a deleted Offscreen

205 + // parent node. (This discovery suggests it may be better for memory usage

206 + // if we don't attach the `return` pointer until the commit phase, though

207 + // in order to do that we'd need some other way to track the return

208 + // pointer during the initial render, like on the stack.)

209 + //

210 + // This case is always accompanied by a warning, but we still need to

211 + // account for it. (There may be other cases that we haven't discovered,

212 + // too.)

213 + const offscreenInstance: OffscreenInstance | null = parent.stateNode;

214 + if (offscreenInstance !== null && offscreenInstance.isHidden) {

215 isHidden = true;

216 }

217 }

packages/react-reconciler/src/__tests__/ReactOffscreen-test.js

+51

@@ -7,6 +7,7 @@ let Offscreen;

7 let useState;

8 let useLayoutEffect;

9 let useEffect;

10 +let startTransition;

12 describe('ReactOffscreen', () => {

13 beforeEach(() => {

@@ -21,6 +22,7 @@ describe('ReactOffscreen', () => {

22 useState = React.useState;

23 useLayoutEffect = React.useLayoutEffect;

24 useEffect = React.useEffect;

25 + startTransition = React.startTransition;

26 });

28 function Text(props) {

@@ -591,4 +593,53 @@ describe('ReactOffscreen', () => {

593 </>,

594 );

595 });

596 +

597 + // TODO: Create TestFlag alias for Offscreen

598 + // @gate experimental || www

599 + it('regression: Offscreen instance is sometimes null during setState', async () => {

600 + let setState;

601 + function Child() {

602 + const [state, _setState] = useState('Initial');

603 + setState = _setState;

604 + return <Text text={state} />;

605 + }

606 +

607 + const root = ReactNoop.createRoot();

608 + await act(async () => {

609 + root.render(<Offscreen hidden={false} />);

610 + });

611 + expect(Scheduler).toHaveYielded([]);

612 + expect(root).toMatchRenderedOutput(null);

613 +

614 + await act(async () => {

615 + // Partially render a component

616 + startTransition(() => {

617 + root.render(

618 + <Offscreen hidden={false}>

619 + <Child />

620 + <Text text="Sibling" />

621 + </Offscreen>,

622 + );

623 + });

624 + expect(Scheduler).toFlushAndYieldThrough(['Initial']);

625 +

626 + // Before it finishes rendering, the whole tree gets deleted

627 + ReactNoop.flushSync(() => {

628 + root.render(null);

629 + });

630 +

631 + // Something attempts to update the never-mounted component. When this

632 + // regression test was written, we would walk up the component's return

633 + // path and reach an unmounted Offscreen component fiber. Its `stateNode`

634 + // would be null because it was nulled out when it was deleted, but there

635 + // was no null check before we accessed it. A weird edge case but we must

636 + // account for it.

637 + expect(() => {

638 + setState('Updated');

639 + }).toErrorDev(

640 + "Can't perform a React state update on a component that hasn't mounted yet",

641 + );

642 + });

643 + expect(root).toMatchRenderedOutput(null);

644 + });

645 });

scripts/merge-fork/forked-revisions

@@ -1,3 +1,4 @@

1 +d410f0a1bbb12e972b0e99bb9faea10c7e62894d [FORKED] Bugfix: Offscreen instance is null during setState

2 58bb11764bf0bb6db47527a64f693f67cdd3b0bb [FORKED] Check for infinite update loops even if unmounted

3 31882b5dd66f34f70d341ea2781cacbe802bf4d5 [FORKED] Bugfix: Revealing a hidden update

4 17691acc071d56261d43c3cf183f287d983baa9b [FORKED] Don't update childLanes until after current render