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

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Jun 16, 2022

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.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jun 16, 2022
@acdlite acdlite force-pushed the fix-offscreen-null-setState branch from c092510 to 55e409f Compare June 16, 2022 01:53
@sizebot
Copy link

sizebot commented Jun 16, 2022

Comparing: fcd720d...0a2937e

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 131.76 kB 131.76 kB = 42.31 kB 42.31 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 137.03 kB 137.03 kB = 43.90 kB 43.90 kB
facebook-www/ReactDOM-prod.classic.js = 440.91 kB 440.91 kB = 80.63 kB 80.63 kB
facebook-www/ReactDOM-prod.modern.js = 426.21 kB 426.21 kB = 78.44 kB 78.44 kB
facebook-www/ReactDOMForked-prod.classic.js +0.02% 441.61 kB 441.68 kB +0.02% 80.85 kB 80.86 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 0a2937e

@acdlite acdlite force-pushed the fix-offscreen-null-setState branch from 55e409f to 33559d0 Compare June 16, 2022 01:58
@acdlite
Copy link
Collaborator Author

acdlite commented Jun 16, 2022

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.

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 acdlite force-pushed the fix-offscreen-null-setState branch from 33559d0 to 0a2937e Compare June 16, 2022 02:03
@acdlite acdlite merged commit c3d7a7e into facebook:main Jun 16, 2022
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
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants