Fix Android crash: view created after ViewController destroyed (MM-68032)#9693
Open
nickmisasi wants to merge 1 commit intomainfrom
Open
Fix Android crash: view created after ViewController destroyed (MM-68032)#9693nickmisasi wants to merge 1 commit intomainfrom
nickmisasi wants to merge 1 commit intomainfrom
Conversation
Kotlin property viewController.view maps to Java getView(), which creates the view and throws if the controller was already destroyed during MainActivity teardown (Sentry MATTERMOST-MOBILE-ANDROID-AX68 / MM-68032). Add ViewController.getExistingViewOnly() and use it from StatusBarPresenter. Refs MM-68032 Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Coverage Comparison ReportGenerated on April 15, 2026 at 18:27:39 UTC |
Contributor
Author
|
@enahum first go at one of these - this is fully AI-driven. If it's on the right track I'm happy to pick up a few more of these off the list and run them through, if not, I will close and not waste your time |
Contributor
📝 WalkthroughWalkthroughThis patch modifies react-native-navigation to extend NavigationActivity from ReactActivity instead of AppCompatActivity, refactor NavigationModule to use Optional pattern for navigator access, add a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
patches/react-native-navigation+7.45.0.patch (2)
263-273:⚠️ Potential issue | 🟠 MajorPotential
ClassCastExceptionininvalidate()methodThe
activity()method now returnsActivityinstead ofNavigationActivity, butinvalidate()performs a direct cast without a guard. If the current activity is not aNavigationActivity, this will throw aClassCastException. This is inconsistent with the defensiveOptionalpattern applied elsewhere in this file.🐛 Proposed fix to add instanceof check
`@Override` public void invalidate() { - final NavigationActivity navigationActivity = (NavigationActivity)activity(); - if (navigationActivity != null) { + final Activity current = activity(); + if (current instanceof NavigationActivity) { + final NavigationActivity navigationActivity = (NavigationActivity) current; navigationActivity.onCatalystInstanceDestroy(); } super.invalidate(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patches/react-native-navigation`+7.45.0.patch around lines 263 - 273, invalidate() currently casts activity() to NavigationActivity directly which can throw ClassCastException; change the method to retrieve the Activity (via activity()), check "instanceof NavigationActivity" before casting, and only call navigationActivity.onCatalystInstanceDestroy() if the check passes; keep the call to super.invalidate() unmodified. Reference symbols: invalidate(), activity(), NavigationActivity, onCatalystInstanceDestroy(), super.invalidate().
353-372:⚠️ Potential issue | 🟡 MinorInconsistent null-safety:
!!assertion on nullablegetEventDispatcher()The
getEventDispatcher()return type was changed toEventDispatcher?(nullable), but lines 357, 364, and 371 use!!(non-null assertion). IfgetEventDispatcher()returns null, this will throw aNullPointerException, negating the null-safety improvements.🛡️ Proposed fix to handle nullable dispatcher consistently
override fun onChildStartedNativeGesture(child: View, androidEvent: MotionEvent?) { - androidEvent?.let { - mJSTouchDispatcher.onChildStartedNativeGesture(it, this.getEventDispatcher()!!) + val dispatcher = getEventDispatcher() ?: return + androidEvent?.let { + mJSTouchDispatcher.onChildStartedNativeGesture(it, dispatcher) } } override fun onChildStartedNativeGesture(androidEvent: MotionEvent?) { - androidEvent?.let { - mJSTouchDispatcher.onChildStartedNativeGesture(it, this.getEventDispatcher()!!) + val dispatcher = getEventDispatcher() ?: return + androidEvent?.let { + mJSTouchDispatcher.onChildStartedNativeGesture(it, dispatcher) } } override fun onChildEndedNativeGesture(child: View, androidEvent: MotionEvent?) { - androidEvent?.let { - mJSTouchDispatcher.onChildEndedNativeGesture(it, this.getEventDispatcher()!!) + val dispatcher = getEventDispatcher() ?: return + androidEvent?.let { + mJSTouchDispatcher.onChildEndedNativeGesture(it, dispatcher) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patches/react-native-navigation`+7.45.0.patch around lines 353 - 372, The code uses non-null assertions on a now-nullable getEventDispatcher() inside onChildStartedNativeGesture and onChildEndedNativeGesture; instead of using !!, check the dispatcher for null and bail out or safely call it (e.g., val dispatcher = getEventDispatcher() ?: return or getEventDispatcher()?.let { ... }) before calling mJSTouchDispatcher.onChildStartedNativeGesture / onChildEndedNativeGesture so you never force-unwrap a nullable EventDispatcher; update both overloads of onChildStartedNativeGesture and onChildEndedNativeGesture to use a safe-check on getEventDispatcher().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@patches/react-native-navigation`+7.45.0.patch:
- Around line 263-273: invalidate() currently casts activity() to
NavigationActivity directly which can throw ClassCastException; change the
method to retrieve the Activity (via activity()), check "instanceof
NavigationActivity" before casting, and only call
navigationActivity.onCatalystInstanceDestroy() if the check passes; keep the
call to super.invalidate() unmodified. Reference symbols: invalidate(),
activity(), NavigationActivity, onCatalystInstanceDestroy(), super.invalidate().
- Around line 353-372: The code uses non-null assertions on a now-nullable
getEventDispatcher() inside onChildStartedNativeGesture and
onChildEndedNativeGesture; instead of using !!, check the dispatcher for null
and bail out or safely call it (e.g., val dispatcher = getEventDispatcher() ?:
return or getEventDispatcher()?.let { ... }) before calling
mJSTouchDispatcher.onChildStartedNativeGesture / onChildEndedNativeGesture so
you never force-unwrap a nullable EventDispatcher; update both overloads of
onChildStartedNativeGesture and onChildEndedNativeGesture to use a safe-check on
getEventDispatcher().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d92241d-6386-4f9b-a26a-26e57762d7ec
📒 Files selected for processing (1)
patches/react-native-navigation+7.45.0.patch
Contributor
|
Do we really want this? I think the effort should be placed to merge #9402 |
Contributor
Author
|
@enahum I'm all for refactors, in the last 60 days this crash has happened nearly 2000 times :P If that PR is going to be merged soon then I'm happy to close the PR, but if it's going to be another month or two, I think we should fix the crash as it is |
Contributor
|
Lwt's leave this open for a week, let's see if the PR makes it by then (I hope it will) |
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
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.
Summary
Fixes a high-volume Android production crash (
RuntimeException: Tried to create view after it has already been destroyed) seen when destroyingMainActivity, with the stack going through react-native-navigation status bar handling.Root cause: In
StatusBarPresenter.setStatusBarVisible, KotlinviewController.viewresolves to the Java gettergetView(), not the backing field. During teardown that can callgetView()on a controller whose view was already torn down andisDestroyedis true, which throws the exact message from Sentry.Fix: Add
ViewController.getExistingViewOnly()(returns the field without creating a view) and use it fromStatusBarPresenterso status bar visibility uses either the existing view orwindow.decorView, nevergetView()implicitly.Change is applied via the existing
patches/react-native-navigation+7.45.0.patch.Ticket Link
Fixes https://mattermost.atlassian.net/browse/MM-68032
Sentry: https://mattermost-mr.sentry.io/issues/6640793436/
Checklist
E2E/Run(orE2E/Run-iOS/E2E/Run-Androidfor platform-specific runs).Device Information
This PR was tested on: not run (native Android patch;
npm run tscpassed)Screenshots
N/A
Release Note