fix(Android,Fabric,bridgeless): crash on RN hot reload in dev mode when redbox in presentation #2289
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
… initialization code
WoLewicki
reviewed
Aug 8, 2024
| if (!reactContext.hasCurrentActivity()) { | ||
| return false | ||
| } | ||
|
|
||
| // We need to use activity here, as react context does not have theme attributes required by | ||
| // AppBarLayout attached leading to crash. | ||
| val contextWithTheme = | ||
| requireNotNull(reactContext.currentActivity) { "[RNScreens] Attempt to use context detached from activity" } | ||
| requireNotNull(reactContext.currentActivity) { "[RNScreens] Attempt to use context detached from activity. This could happen only due to race-condition." } |
There was a problem hiding this comment.
Why aren't you using reactContextRef everywhere?
There was a problem hiding this comment.
@WoLewicki this method takes reactContext: ReactApplicationContext as a parameter, there is no point of using the reactContextRef field here. It makes more sense for me to take this as parameter, because:
- this method requires nonnull react context to do anything,
- moving context validation out of the method allows to throw better error messages which are tailored to specific code path & context, see e.g.
onHostResumemethod; - during package initialisation I do have direct access to
reactContext(of non-nullable type), thus can pass it directly / omit checks in the method itself.
There was a problem hiding this comment.
I do not see any advantages of using reactContextRef directly. I do need to sanitise the reference anyway in almost all cases, because I do want to throw / log an message everytime reactContextRef.get() == null & it is unexpected.
ja1ns
pushed a commit
to WiseOwlTech/react-native-screens
that referenced
this pull request
Oct 9, 2024
…en redbox in presentation (software-mansion#2289) ## Description There was a crash on Android + Fabric + bridgeless: 1. trigger a redbox by triggering some JS error (e.g. call a method that does not exist), 2. fast refresh react-native, 3. see the crash in `onHostResume`. The application crashed because we had a check for not null activity in reactContext. However after such reload ReactContext is recreated with null activity!!!. In such case, when we don't have access to the activity, we're currently unable to create the dummy layout, so the best we can do is not fail. The fix for jumping layout won't work, however. This PR also adds synchornization on `synchornied` block in `maybeInitDummyLayoutWithHeaderMethod`, which should prevent a possible data race, where both threads could try to create dummy layout simmultaneously. ## Changes * Do not fail in `onHostResume` when there is no activity present in the provided context, * only log a warning in the described case, * synchronize layout initialisation, so that there is no data race <- I'm not sure it was real due to JVM atomicity guarantees, however this time I prefer the "better safe than sorry" approach, * mark `isLayoutInitialized` variable as `Volatile`, to ensure that updates from one thread are visible on the other thread. ## Test code and steps to reproduce Desribed above ^ in the "Description" section. ## Checklist - [ ] Ensured that CI passes
1 task
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.
Description
There was a crash on Android + Fabric + bridgeless:
onHostResume.The application crashed because we had a check for not null activity in reactContext.
However after such reload ReactContext is recreated with null activity!!!.
In such case, when we don't have access to the activity, we're currently unable to create the dummy layout,
so the best we can do is not fail. The fix for jumping layout won't work, however.
This PR also adds synchornization on
synchorniedblock inmaybeInitDummyLayoutWithHeaderMethod, which should preventa possible data race, where both threads could try to create dummy layout simmultaneously.
Changes
onHostResumewhen there is no activity present in the provided context,isLayoutInitializedvariable asVolatile, to ensure that updates from one thread are visible on the other thread.Test code and steps to reproduce
Desribed above ^ in the "Description" section.
Checklist