Skip to content

Fix dangling surfaces in ReactHostImpl - #44393

Closed
Kudo wants to merge 1 commit into
react:mainfrom
Kudo:@kudo/cleanup-attached-surfaces
Closed

Fix dangling surfaces in ReactHostImpl#44393
Kudo wants to merge 1 commit into
react:mainfrom
Kudo:@kudo/cleanup-attached-surfaces

Conversation

@Kudo

@KudoKudo commented May 2, 2024

Copy link
Copy Markdown
Contributor

Summary:

Though the ReactHost.destroy() is not being used from OSS code, we use it at Expo for expo-dev-client to change loading apps from different dev servers. Without cleanup the mAttachedSurfaces, it will have dangling or duplicated attached surfaces that cause duplicated react trees.

This PR tries to cleanup the mAttachedSurfaces from destroying.

Changelog:

[ANDROID] [FIXED] - Fixed dangling mAttachedSurfaces after ReactHost.destroy()

Test Plan:

have to manually call ReactHost.destroy() and recreate the MainActivity without killing the process. then reload the app will startSurface for the same attached surfaces.

@facebook-github-botfacebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. p: Expo Partner: Expo Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels May 2, 2024
@analysis-bot

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a19,527,863-9
androidhermesarmeabi-v7an/a--
androidhermesx86n/a--
androidhermesx86_64n/a--
androidjscarm64-v8a22,900,308-10
androidjscarmeabi-v7an/a--
androidjscx86n/a--
androidjscx86_64n/a--

Base commit: 00b3661
Branch: main

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@javache merged this pull request in c70d791.

@github-actions

Copy link
Copy Markdown

This pull request was successfully merged by @Kudo in c70d791.

When will my fix make it into a release? | How to file a pick request?

@Kudo
Kudo deleted the @kudo/cleanup-attached-surfaces branch June 3, 2024 14:17
cipolleschi pushed a commit that referenced this pull request Jun 4, 2024
Summary:
Though the `ReactHost.destroy()` is not being used from OSS code, we use it at Expo for expo-dev-client to change loading apps from different dev servers. Without cleanup the `mAttachedSurfaces`, it will have dangling or duplicated attached surfaces that cause duplicated react trees.
<img src="https://github.com/facebook/react-native/assets/46429/f84d274e-aaad-4352-9e3c-6262571a5625">
This PR tries to cleanup the `mAttachedSurfaces` from destroying.
## Changelog:
[ANDROID] [FIXED] - Fixed dangling `mAttachedSurfaces` after `ReactHost.destroy()`
Pull Request resolved: #44393
Test Plan: have to manually call `ReactHost.destroy()` and recreate the MainActivity without killing the process. then reload the app will startSurface for the same attached surfaces.
Reviewed By: RSNara
Differential Revision: D56901863
Pulled By: javache
fbshipit-source-id: c7f822501d971810ac6aa7262b15da69ec41355e
kosmydel pushed a commit to kosmydel/react-native that referenced this pull request Jun 11, 2024
Summary:
Though the `ReactHost.destroy()` is not being used from OSS code, we use it at Expo for expo-dev-client to change loading apps from different dev servers. Without cleanup the `mAttachedSurfaces`, it will have dangling or duplicated attached surfaces that cause duplicated react trees.
<img src="https://github.com/facebook/react-native/assets/46429/f84d274e-aaad-4352-9e3c-6262571a5625">
This PR tries to cleanup the `mAttachedSurfaces` from destroying.
## Changelog:
[ANDROID] [FIXED] - Fixed dangling `mAttachedSurfaces` after `ReactHost.destroy()`
Pull Request resolved: react#44393
Test Plan: have to manually call `ReactHost.destroy()` and recreate the MainActivity without killing the process. then reload the app will startSurface for the same attached surfaces.
Reviewed By: RSNara
Differential Revision: D56901863
Pulled By: javache
fbshipit-source-id: c7f822501d971810ac6aa7262b15da69ec41355e
This was referenced Jun 28, 2024
@cipolleschicipolleschi mentioned this pull request Jul 22, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.ContributorA React Native contributor.MergedThis PR has been merged.p: ExpoPartner: ExpoPartnerShared with MetaApplied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@Kudo@analysis-bot@facebook-github-bot