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.

Sign up for GitHub

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

Jump to bottom

fix: Prevent crash when resuming class component inside SuspenseList #18432

Closed
wants to merge 6 commits into from
Closed

fix: Prevent crash when resuming class component inside SuspenseList #18432

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
199b33c
test(SuspenseList): Add failing test for error boundaries
eps1lon Mar 29, 2020
234a9ed
test: reproducible with a simple class component
eps1lon Mar 29, 2020
13d9dd8
fix: initialize updatedQueue after it was reset
eps1lon Mar 29, 2020
87117c8
Move fix to `resetWorkInProgress`
eps1lon Mar 31, 2020
81b3372
reset updateQueue by re-initalizing for class components and host roots
eps1lon Mar 31, 2020
58f6c6a
test: adjust test description now that it's passing
eps1lon Mar 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/react-reconciler/src/ReactFiberClassComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -885,6 +885,10 @@ function resumeMountClassInstance(
nextContext = getMaskedContext(workInProgress, nextLegacyUnmaskedContext);
}

if (workInProgress.updateQueue === null) {
initializeUpdateQueue(workInProgress);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks more like it's fixing symptoms.

The queue is emptied in resetWorkInProgress if current === null:

react/packages/react-reconciler/src/ReactFiber.js

Lines 541 to 549 in 31a9e39

if (current === null) {
// Reset to createFiber's initial values.
workInProgress.childExpirationTime = NoWork;
workInProgress.expirationTime = renderExpirationTime;
workInProgress.child = null;
workInProgress.memoizedProps = null;
workInProgress.memoizedState = null;
workInProgress.updateQueue = null;

In updateClassComponent we resume in resumeMountClassInstance if current === null which straight goes to processUpdateQueue assuming the updateQueue has been initialized (but it isn't anymore).

Note that not reseting the updatedQueue in resetWorkInProgress fixes this as well while not failing any other test.

Copy link
Collaborator

@acdlite acdlite Mar 30, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the type were sound, the updateQueue on a class fiber should always be non-null. So my preference is to put the fix in resetWorkInProgress instead.

But also it's classes so whatever :D

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do move the fix to resetWorkInProgress, I would add a dev-only check to resumeMountClassInstance to guard against a regression.

if (__DEV__) {
  if (workInProgress.updateQueue === null) { 
    console.error('Internal error: Class fiber is missing an updateQueue');
  }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • don't clear updateQueue in resetWorkInProgress
  • add warning if updateQueue is empty in resumeWorkInProgress:

    Class fiber is missing an updateQueue. This error is likely caused by a bug in React. Please file an issue.

}

const getDerivedStateFromProps = ctor.getDerivedStateFromProps;
const hasNewLifecycles =
typeof getDerivedStateFromProps === 'function' ||
Expand Down
62 changes: 62 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactSuspenseList-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2485,4 +2485,66 @@ describe('ReactSuspenseList', () => {
</>,
);
});

it('crashes when it contains class components when revealed together', async () => {
let A = createAsyncText('A');
let B = createAsyncText('B');

class ClassComponent extends React.Component {
render() {
return this.props.children;
}
}

function Foo() {
return (
<Suspense fallback={<Text text="Loading" />}>
<SuspenseList revealOrder="together">
<ClassComponent>
<Suspense fallback={<Text text="Loading A" />}>
<A />
</Suspense>
</ClassComponent>
<ClassComponent>
<Suspense fallback={<Text text="Loading B" />}>
<B />
</Suspense>
</ClassComponent>
</SuspenseList>
</Suspense>
);
}

await A.resolve();

ReactNoop.render(<Foo />);

expect(Scheduler).toFlushAndYield([
'A',
'Suspend! [B]',
'Loading B',
'Loading A',
'Loading B',
]);

expect(ReactNoop).toMatchRenderedOutput(
<>
<span>Loading A</span>
<span>Loading B</span>
</>,
);

await B.resolve();

ReactNoop.render(<Foo />);

expect(Scheduler).toFlushAndYield(['A', 'B']);

expect(ReactNoop).toMatchRenderedOutput(
<>
<span>A</span>
<span>B</span>
</>,
);
});
});