Skip to content

Commit

Permalink
[FORKED] Bugfix: Offscreen instance is null during setState
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
acdlite committed Jun 16, 2022
1 parent fcd720d commit d410f0a
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 2 deletions.
21 changes: 19 additions & 2 deletions packages/react-reconciler/src/ReactFiberConcurrentUpdates.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down
51 changes: 51 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ let Offscreen;
let useState;
let useLayoutEffect;
let useEffect;
let startTransition;

describe('ReactOffscreen', () => {
beforeEach(() => {
Expand All @@ -21,6 +22,7 @@ describe('ReactOffscreen', () => {
useState = React.useState;
useLayoutEffect = React.useLayoutEffect;
useEffect = React.useEffect;
startTransition = React.startTransition;
});

function Text(props) {
Expand Down Expand Up @@ -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);
});
});

0 comments on commit d410f0a

Please sign in to comment.