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

[Crashlytics] TypeError: Cannot read property 'routes' of undefined #46118

Closed
CortneyOfstad opened this issue Jul 24, 2024 · 21 comments
Closed
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2

Comments

@CortneyOfstad
Copy link
Contributor

CortneyOfstad commented Jul 24, 2024

Coming from this GH — #45054 (comment)

Reported by @TMisiukiewicz

Fatal Exception: com.facebook.react.common.JavascriptException: TypeError: Cannot read property 'routes' of undefined, js engine: hermes, stack:
onBackPress@1:7675381
anonymous@1:1137735
emit@1:194390
emit@1:190480
__callFunction@1:200223
anonymous@1:197709
__guard@1:199579
callFunctionReturnFlushedQueue@1:197667

     at com.facebook.react.modules.core.ExceptionsManagerModule.reportException(ExceptionsManagerModule.java:65)
     at com.facebook.jni.NativeRunnable.run(NativeRunnable.java)
     at android.os.Handler.handleCallback(Handler.java:959)
     at android.os.Handler.dispatchMessage(Handler.java:100)
     at com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(MessageQueueThreadHandler.java:29)
     at android.os.Looper.loopOnce(Looper.java:232)
     at android.os.Looper.loop(Looper.java:317)
     at com.facebook.react.bridge.queue.MessageQueueThreadImpl$4.run(MessageQueueThreadImpl.java:234)
     at java.lang.Thread.run(Thread.java:1012)
Issue OwnerCurrent Issue Owner: @CortneyOfstad
@CortneyOfstad CortneyOfstad self-assigned this Jul 24, 2024
@CortneyOfstad CortneyOfstad added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 24, 2024
Copy link

melvin-bot bot commented Jul 24, 2024

Current assignee @CortneyOfstad is eligible for the Bug assigner, not assigning anyone new.

Copy link

melvin-bot bot commented Jul 29, 2024

@CortneyOfstad Eep! 4 days overdue now. Issues have feelings too...

@CortneyOfstad
Copy link
Contributor Author

@TMisiukiewicz it was indicated on one of the other Crashlytic GHs that the logs are truncated. Any way you could pull up the full logs? Thanks!

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jul 31, 2024
@TMisiukiewicz
Copy link
Contributor

I tried following the breadcrumbs from firebase, but couldn't reproduce it. Looks like when using system back button on Android, sometimes getRootState returns undefined. According to the docs, the returned state will be undefined if there are no navigators currently rendered, but i'm not sure how to reproduce this state.

We could prevent crashing by returning false when the root state is undefined. If opening a PR without repro steps is fine, I can create it

Copy link

melvin-bot bot commented Aug 5, 2024

@CortneyOfstad Huh... This is 4 days overdue. Who can take care of this?

Copy link

melvin-bot bot commented Aug 7, 2024

@CortneyOfstad this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

Copy link

melvin-bot bot commented Aug 7, 2024

@CortneyOfstad Still overdue 6 days?! Let's take care of this!

Copy link

melvin-bot bot commented Aug 9, 2024

@CortneyOfstad Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

@CortneyOfstad
Copy link
Contributor Author

@TMisiukiewicz sorry for the delay here! For this comment, I would be leery to create a PR without the workflow, so leaning towards closing this. However, if you feel strongly that the PR should still be created, let me know and we can work it out with the team. Thanks!

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Aug 12, 2024
Copy link

melvin-bot bot commented Aug 15, 2024

@CortneyOfstad Whoops! This issue is 2 days overdue. Let's get this updated quick!

@hurali97
Copy link
Contributor

Hey @CortneyOfstad 👋

I took it over from @TMisiukiewicz and I also can't reproduce this, however I think we should still go about creating a PR fixing this.

We can opt for a simpler solution, which is to use the optional chaining, see below:

diff --git a/src/libs/Navigation/setupCustomAndroidBackHandler/index.android.ts b/src/libs/Navigation/setupCustomAndroidBackHandler/index.android.ts
index 10aa8b99a4..107dae2bb7 100644
--- a/src/libs/Navigation/setupCustomAndroidBackHandler/index.android.ts
+++ b/src/libs/Navigation/setupCustomAndroidBackHandler/index.android.ts
@@ -13,8 +13,7 @@ type SearchPageProps = StackScreenProps<BottomTabNavigatorParamList, typeof SCRE
 function setupCustomAndroidBackHandler() {
     const onBackPress = () => {
         const rootState = navigationRef.getRootState();
-
-        const bottomTabRoute = rootState.routes.find((route) => route.name === NAVIGATORS.BOTTOM_TAB_NAVIGATOR);
+        const bottomTabRoute = rootState?.routes?.find((route) => route.name === NAVIGATORS.BOTTOM_TAB_NAVIGATOR);
         const bottomTabRoutes = bottomTabRoute?.state?.routes;
         const focusedRoute = findFocusedRoute(rootState);
 
@@ -23,7 +22,7 @@ function setupCustomAndroidBackHandler() {
             return false;
         }
 
-        const isLastScreenOnStack = bottomTabRoutes.length === 1 && rootState.routes.length === 1;
+        const isLastScreenOnStack = bottomTabRoutes.length === 1 && rootState?.routes?.length === 1;
 
         if (NativeModules.HybridAppModule && isLastScreenOnStack) {
             NativeModules.HybridAppModule.exitApp();
@@ -35,7 +34,7 @@ function setupCustomAndroidBackHandler() {
             navigationRef.dispatch({...StackActions.pop(), target: bottomTabRoute?.state?.key});
             navigationRef.dispatch({...StackActions.pop()});
 
-            const centralPaneRouteAfterPop = getTopmostCentralPaneRoute({routes: [rootState.routes.at(-2)]} as State<RootStackParamList>);
+            const centralPaneRouteAfterPop = getTopmostCentralPaneRoute({routes: [rootState?.routes?.at(-2)]} as State<RootStackParamList>);
             const bottomTabRouteAfterPop = bottomTabRoutes.at(-2);
 
             // It's possible that central pane search is desynchronized with the bottom tab search.
@@ -57,7 +56,7 @@ function setupCustomAndroidBackHandler() {
         // It's possible that central pane search is desynchronized with the bottom tab search.
         // e.g. opening a tab different than search will wipe out central pane screens.
         // In that case we have to push the proper one.
-        if (bottomTabRoutes && bottomTabRoutes?.length >= 2 && bottomTabRoutes[bottomTabRoutes.length - 2].name === SCREENS.SEARCH.BOTTOM_TAB && rootState.routes.length === 1) {
+        if (bottomTabRoutes && bottomTabRoutes?.length >= 2 && bottomTabRoutes[bottomTabRoutes.length - 2].name === SCREENS.SEARCH.BOTTOM_TAB && rootState?.routes?.length === 1) {
             const {policyID, ...restParams} = bottomTabRoutes[bottomTabRoutes.length - 2].params as SearchPageProps['route']['params'];
             navigationRef.dispatch({...StackActions.push(SCREENS.SEARCH.CENTRAL_PANE, {...restParams, policyIDs: policyID})});
             navigationRef.dispatch({...StackActions.pop(), target: bottomTabRoute?.state?.key});

This won't introduce any regressions and is more of a safety check. Ideally, when accessing nested optional properties, we use optional chaining, which was missing and causing this issue.


As for the testing, I manually set rootState = undefined and tested this code, which doesn't crash on hardware back press on Android.

@CortneyOfstad
Copy link
Contributor Author

That sounds great @hurali97! Thank you!

@melvin-bot melvin-bot bot removed the Overdue label Aug 19, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Aug 20, 2024
Copy link

melvin-bot bot commented Aug 26, 2024

Triggered auto assignment to @blimpich, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@mananjadhav
Copy link
Collaborator

@blimpich Can you confirm if this was deployed to production? I can't make out that. I can see it's deployed to staging 2 weeks back. If it is deployed then this would be ready for payout.

@blimpich
Copy link
Contributor

blimpich commented Sep 9, 2024

Yes looks like it was deployed to production 👍

@blimpich
Copy link
Contributor

blimpich commented Sep 9, 2024

Sorry for the delay, automation must have messed up with the deploy notification

@blimpich blimpich added Daily KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 Reviewing Has a PR in review labels Sep 9, 2024
@blimpich
Copy link
Contributor

blimpich commented Sep 9, 2024

Ah, part of this is probably because there was never external label applied to this issue. @CortneyOfstad can we treat this issue as a normal $250 issue and pay out to @mananjadhav as the C+ who reviewed the associated PR?

@mananjadhav
Copy link
Collaborator

thanks for taking a look @blimpich. @CortneyOfstad would be great if yo can share the payout summary.

@CortneyOfstad
Copy link
Contributor Author

Sorry for the delay here!

@CortneyOfstad
Copy link
Contributor Author

Payment Summary

@mananjadhav — to be paid $250 via NewDot

@garrettmknight
Copy link
Contributor

$250 approved for @mananjadhav

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2
Projects
None yet
Development

No branches or pull requests

6 participants