search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.53k stars 2.88k forks source link

[HybridApp] iOS HybridApp crash 💥 #43368

Closed AndrewGable closed 4 months ago

AndrewGable commented 5 months ago

Problem

@puneetlath's iOS HybridApp has crashes twice in the background

com.expensify.expensifylite_issue_e5144cd971163542ddd3f0eee46f4474_crash_session_e544b43e0d2b4bbf93781696735cbdd2_DNE_0_v2_stacktrace.txt

com.expensify.expensifylite_issue_e5144cd971163542ddd3f0eee46f4474_crash_session_b52147e45a614ceaa64a186099d9115f_DNE_0_v2_stacktrace.txt

Solution

Fix it!

cc @staszekscp - I am going to send you the logs

AndrewGable commented 5 months ago

More details: Puneet was using NewDot via HybridApp, then closed HybridApp and saw the app had crashes once the HybridApp was in the background

AndrewGable commented 4 months ago

I have seen this now, I think it's related to push notifications, but have not been able to confirm this is true.

AndrewGable commented 4 months ago

Ok Puneet had a crash at 11:51AM UTC, going to see if we sent a push notification around then

AndrewGable commented 4 months ago

Seems like he might have got a eReceipt notification around that time

melvin-bot[bot] commented 4 months ago

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

AndrewGable commented 4 months ago

cc @WoLewicki

WoLewicki commented 4 months ago

Commenting :rocket:

WoLewicki commented 4 months ago

Hey, from what I understand, the crash is connected to receiving notification when app is in background, I need to build the app on physical device since there no notifications on simulator on iOS. Is there an official guide of how can I build it? cc @AndrewGable

AndrewGable commented 4 months ago

Yes, in the readme! https://github.com/expensify/mobile-Expensify/?tab=readme-ov-file#ios-build

Should just be npm run ios once you get all dependencies sorted.

WoLewicki commented 4 months ago

Ok got it! What about the provisioning profiles needed for building on the physical device? And is there some dedicated setup to test the notifications?

AndrewGable commented 4 months ago

There is no dedicated setup, you should be able to run this in dev mode (just using your own personal profile/certs), however, I will set up a ngrok tunnel for you to hit to send push notifications.

WoLewicki commented 4 months ago

I’m trying to reproduce the crash on iOS, but not successful for now. The weird thing I can see is:

It does not resolve in crash, but maybe under some conditions, navigating to OD in the background may trigger the crash. Or is it expected behavior?

WoLewicki commented 4 months ago

One more thing I spotted is that if I had a FAB modal opened when the above scenario happens, when I go back to ND after all those actions, the button itself is in clicked mode but the modal is hidden, but it seems like a minor bug.

WoLewicki commented 4 months ago

Ok seems like the behavior seen here: https://github.com/Expensify/App/issues/43368#issuecomment-2194964033 is by design in OD app. When looking at https://github.com/Expensify/Mobile-Expensify/blob/abbe95356f3844d0a4c190343d121eeacfe4bc46/iOS/Expensify/Libraries/YAPL-Cocoa/PushNotificationManager.m#L210-L230, we can see that the comment: - App is backgrounded and the user tap on the notification seems wrong, as the method name indicates that didReceiveRemoteNotification. So it indeed fires when the notification comes to the phone.

Then, since the app is inactive, we run the onNotificationSelected callback: https://github.com/Expensify/Mobile-Expensify/blob/abbe95356f3844d0a4c190343d121eeacfe4bc46/iOS/Expensify/Libraries/YAPL-Cocoa/PushNotificationManager.m#L228, which takes us to OD: https://github.com/Expensify/Mobile-Expensify/blob/2750c9348b5f7701961d504ef7f10327fda96ed0/app/managers/PushNotificationManager.js#L61-L64.

It seems like a weird behavior to navigate in the app when the notification is received, not selected.

At the same time, looking at the crash stacktrace, it seems like it tries to open NewDot again. image

Looking at the code, it is possible if the notification type was reportComment: https://github.com/Expensify/Mobile-Expensify/blob/abbe95356f3844d0a4c190343d121eeacfe4bc46/app/managers/PushNotificationManager.js#L56. Was it the case when the crash happened?

In conclusion, I can see multiple cases where this can lead to crash. For example, you can quickly get a notification which wants to open the ND, then a second one that would want to close it, before the opening of ND is ended. I think the solution is not to call those things when the notification is received, but only when it is selected.