fix: Prevent crash when resuming class component inside SuspenseList #18432
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 58f6c6a:
|
Details of bundled changes.Comparing: 689d275...58f6c6a react-test-renderer
react-dom
react-art
react-native-renderer
react-reconciler
Size changes (experimental) |
Details of bundled changes.Comparing: 689d275...58f6c6a react-native-renderer
react-art
react-test-renderer
react-dom
react-reconciler
ReactDOM: size: 0.0%, gzip: -0.0% Size changes (stable) |
eps1lon
commented
Mar 29, 2020
| @@ -885,6 +885,10 @@ function resumeMountClassInstance( | |||
| nextContext = getMaskedContext(workInProgress, nextLegacyUnmaskedContext); | |||
| } | |||
|
|
|||
| if (workInProgress.updateQueue === null) { | |||
| initializeUpdateQueue(workInProgress); | |||
There was a problem hiding this comment.
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
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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');
}
}There was a problem hiding this comment.
- 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.
eps1lon
changed the title
gaearon
approved these changes
Mar 30, 2020
There was a problem hiding this comment.
This makes sense to me but would be nice if @sebmarkbage could confirm
|
@eps1lon Wanna do this change? #18432 (comment) |
- workInProgress.updateQueue = null;
+ initializeUpdateQueue(workInProgress);The first approach passed locally. |
|
I guess 2 it is (to have a sound type) if I follow
|
|
Sorry I'm not sure what you're asking. Can you rephase? |
|
Only 1 passes so I guess this is answered for me. I forgot a word there: "1. remove |
sebmarkbage
reviewed
Mar 31, 2020
| @@ -546,7 +546,6 @@ export function resetWorkInProgress( | |||
| workInProgress.child = null; | |||
| workInProgress.memoizedProps = null; | |||
| workInProgress.memoizedState = null; | |||
| workInProgress.updateQueue = null; | |||
There was a problem hiding this comment.
I think this is going to break other things that are likely not covered by tests. We’ve had many bugs related to not resetting enough.
There was a problem hiding this comment.
I'd have to dive deeper to be able to fix this then. I tried initializeUpdateQueue(workInProgress) instead which caused a bunch of "Unexpected pop" or "Unexpected Fiber popped". Also tried
if (workInProgress.updateQueue !== null) {
initializeUpdateQueue(workInProgress);
}
which caused the same issue
There was a problem hiding this comment.
Nevermind. VSCode has problems with flow and didn't realize initializeUpdateQueue was never defined because I imported from the same line as import type. The ReferenceError was just buried below 5 screens of console errors.
eps1lon
commented
Mar 31, 2020
| @@ -546,7 +548,15 @@ export function resetWorkInProgress( | |||
| workInProgress.child = null; | |||
| workInProgress.memoizedProps = null; | |||
| workInProgress.memoizedState = null; | |||
| workInProgress.updateQueue = null; | |||
| // updateQueue must never be null on ClassComponent or HostRoot | |||
There was a problem hiding this comment.
In line with
would it make more sense to move this logic into a resetUpdateQueue inside ReactUpdatedQueue.js?
|
Thanks for the well written test! Opened another PR with a fix. https://github.com/facebook/react/pull/18448/files The issue was that we were resuming in the first place. We shouldn't trigger the resume path here because it's not equipped for true resuming. |
|
Yeah my approach did not feel right. Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Test for #18429 (includes a fix but it's unclear if the semantics are correct)
Test Plan
Might be interesting add matchers for the case where one of the error boundaries is actually triggered.No longer concerned with error boundaries. They caused the crash originally but the bug could be reduced to basic class components.