Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix: Offscreen instance is null during setState #24734

Merged
merged 2 commits into from
Jun 16, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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);
});
});
1 change: 1 addition & 0 deletions scripts/merge-fork/forked-revisions
Original file line number Diff line number Diff line change
@@ -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