Resolve Paper leak on Android #46896
Closed
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
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D64054042 |
rozele
added a commit
to rozele/react-native-macos
that referenced
this pull request
Oct 8, 2024
Summary: On Android Paper UIManager, when calling `ReactRootView.unmountReactApplication`, the ReactRootView tag is unset [here](https://github.com/facebook/react-native/blob/main/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java#L926), before the round trip unmount operation to JS makes it's way back to a `dropView` call on `NativeViewHierarchyManager`. In practice, this means that legacy architecture apps that unmount surfaces via `ReactRootView.unmountReactApplication` leak references to Views, and the "finalization" step (`onDropViewInstance`) is not universally called. This is an attempt to fix the issue by skipping the `clearReactRoot` step on Paper, instead waiting for the round trip `UIManager.removeRootView` call. ## Changelog [Android][Fixed] Fix issue where `onDropViewInstance` cleanup was not being handled after `ReactRootView.unmountReactApplication` Differential Revision: D64054042
rozele
force-pushed
the
export-D64054042
branch
from
5bfa425
to
17a0b63
Compare
|
This pull request was exported from Phabricator. Differential Revision: D64054042 |
Summary: On Android Paper UIManager, when calling `ReactRootView.unmountReactApplication`, the ReactRootView tag is unset [here](https://github.com/facebook/react-native/blob/main/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java#L926), before the round trip unmount operation to JS makes it's way back to a `dropView` call on `NativeViewHierarchyManager`. In practice, this means that legacy architecture apps that unmount surfaces via `ReactRootView.unmountReactApplication` leak references to Views, and the "finalization" step (`onDropViewInstance`) is not universally called. This is an attempt to fix the issue by skipping the `clearReactRoot` step on Paper, instead waiting for the round trip `UIManager.removeRootView` call. ## Changelog [Android][Fixed] Fix issue where `onDropViewInstance` cleanup was not being handled after `ReactRootView.unmountReactApplication` Differential Revision: D64054042
rozele
force-pushed
the
export-D64054042
branch
from
17a0b63
to
ecc15f9
Compare
|
This pull request was exported from Phabricator. Differential Revision: D64054042 |
|
cc @wschurman, since you authored #41678 Would this change cause any problems for |
|
This pull request has been merged in 0449630. |
|
This pull request was successfully merged by @rozele in 0449630 When will my fix make it into a release? | How to file a pick request? |
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:
On Android Paper UIManager, when calling
ReactRootView.unmountReactApplication, the ReactRootView tag is unset here, before the round trip unmount operation to JS makes it's way back to adropViewcall onNativeViewHierarchyManager.In practice, this means that legacy architecture apps that unmount surfaces via
ReactRootView.unmountReactApplicationleak references to Views, and the "finalization" step (onDropViewInstance) is not universally called.This fixes the issue on Paper by temporarily resetting the
ReactRootViewtag before callingdropView.Changelog
[Android][Fixed] Fix issue where
onDropViewInstancecleanup was not being handled afterReactRootView.unmountReactApplicationDifferential Revision: D64054042