Bugfix: Offscreen instance is null during setState #24734
Merged
Conversation
This file contains hidden or 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
facebook-github-bot
added
CLA Signed
React Core Team
acdlite
force-pushed
the
fix-offscreen-null-setState
branch
from
c092510 to
55e409f
Compare
rickhanlonii
approved these changes
Jun 16, 2022
|
Comparing: fcd720d...0a2937e Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
acdlite
force-pushed
the
fix-offscreen-null-setState
branch
from
55e409f to
33559d0
Compare
We would only have to do this at the root of the newly created tree, though. Where the Placement effect is. |
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.
acdlite
force-pushed
the
fix-offscreen-null-setState
branch
from
33559d0 to
0a2937e
Compare
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Aug 17, 2022
Summary: This sync includes the following changes: - **[229c86af0](facebook/react@229c86af0 )**: Revert "Land enableClientRenderFallbackOnTextMismatch" ([#24738](facebook/react#24738)) //<Andrew Clark>// - **[c3d7a7e3d](facebook/react@c3d7a7e3d )**: Bugfix: Offscreen instance is null during setState ([#24734](facebook/react#24734)) //<Andrew Clark>// - **[fcd720d36](facebook/react@fcd720d36 )**: [Transition Tracing] Push Transition When Offscreen Becomes Visible ([#24718](facebook/react#24718)) //<Luna Ruan>// - **[5cc2487e0](facebook/react@5cc2487e0 )**: bump versions for next release ([#24725](facebook/react#24725)) //<Josh Story>// - **[54f17e490](facebook/react@54f17e490 )**: [Transition Tracing] Fix Cache and Transitions Pop Order ([#24719](facebook/react#24719)) //<Luna Ruan>// - **[7cf8dfd94](facebook/react@7cf8dfd94 )**: [Transition Tracing] Create/Process Marker Complete Callback ([#24700](facebook/react#24700)) //<Luna Ruan>// - **[327e4a1f9](facebook/react@327e4a1f9 )**: [Follow-up] Land enableClientRenderFallbackOnTextMismatch //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions a8c9cb1...229c86a jest_e2e[run_all_tests] Reviewed By: rickhanlonii Differential Revision: D38738652 fbshipit-source-id: 35b6b3cbfdbdafc28a356b53af6456aaa1949432
roryabraham
pushed a commit
to Expensify/react-native
that referenced
this pull request
Aug 17, 2022
Summary: This sync includes the following changes: - **[229c86af0](facebook/react@229c86af0 )**: Revert "Land enableClientRenderFallbackOnTextMismatch" ([facebook#24738](facebook/react#24738)) //<Andrew Clark>// - **[c3d7a7e3d](facebook/react@c3d7a7e3d )**: Bugfix: Offscreen instance is null during setState ([facebook#24734](facebook/react#24734)) //<Andrew Clark>// - **[fcd720d36](facebook/react@fcd720d36 )**: [Transition Tracing] Push Transition When Offscreen Becomes Visible ([facebook#24718](facebook/react#24718)) //<Luna Ruan>// - **[5cc2487e0](facebook/react@5cc2487e0 )**: bump versions for next release ([facebook#24725](facebook/react#24725)) //<Josh Story>// - **[54f17e490](facebook/react@54f17e490 )**: [Transition Tracing] Fix Cache and Transitions Pop Order ([facebook#24719](facebook/react#24719)) //<Luna Ruan>// - **[7cf8dfd94](facebook/react@7cf8dfd94 )**: [Transition Tracing] Create/Process Marker Complete Callback ([facebook#24700](facebook/react#24700)) //<Luna Ruan>// - **[327e4a1f9](facebook/react@327e4a1f9 )**: [Follow-up] Land enableClientRenderFallbackOnTextMismatch //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions a8c9cb1...229c86a jest_e2e[run_all_tests] Reviewed By: rickhanlonii Differential Revision: D38738652 fbshipit-source-id: 35b6b3cbfdbdafc28a356b53af6456aaa1949432
roryabraham
pushed a commit
to Expensify/react-native
that referenced
this pull request
Aug 17, 2022
Summary: This sync includes the following changes: - **[229c86af0](facebook/react@229c86af0 )**: Revert "Land enableClientRenderFallbackOnTextMismatch" ([facebook#24738](facebook/react#24738)) //<Andrew Clark>// - **[c3d7a7e3d](facebook/react@c3d7a7e3d )**: Bugfix: Offscreen instance is null during setState ([facebook#24734](facebook/react#24734)) //<Andrew Clark>// - **[fcd720d36](facebook/react@fcd720d36 )**: [Transition Tracing] Push Transition When Offscreen Becomes Visible ([facebook#24718](facebook/react#24718)) //<Luna Ruan>// - **[5cc2487e0](facebook/react@5cc2487e0 )**: bump versions for next release ([facebook#24725](facebook/react#24725)) //<Josh Story>// - **[54f17e490](facebook/react@54f17e490 )**: [Transition Tracing] Fix Cache and Transitions Pop Order ([facebook#24719](facebook/react#24719)) //<Luna Ruan>// - **[7cf8dfd94](facebook/react@7cf8dfd94 )**: [Transition Tracing] Create/Process Marker Complete Callback ([facebook#24700](facebook/react#24700)) //<Luna Ruan>// - **[327e4a1f9](facebook/react@327e4a1f9 )**: [Follow-up] Land enableClientRenderFallbackOnTextMismatch //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions a8c9cb1...229c86a jest_e2e[run_all_tests] Reviewed By: rickhanlonii Differential Revision: D38738652 fbshipit-source-id: 35b6b3cbfdbdafc28a356b53af6456aaa1949432
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
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
stateNodefield.On a mounted Offscreen fiber, the
stateNodeis never null, so usually we don't need to refine the type. When a fiber is unmounted, though, we null out itsstateNodefield to prevent memory cycles. However, we also null out itsreturnfield, 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
setStateon a fiber that never finished mounting. Because it never mounted, it was never deleted. Because it was never deleted, itsreturnfield 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
returnpointer 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.