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

fix: expo users reported app crash on didFailToRegisterForRemoteNotificationsWithError #575

Merged
merged 2 commits into from
Mar 5, 2024

Conversation

ami-aman
Copy link
Contributor

@ami-aman ami-aman commented Feb 29, 2024

part of: https://linear.app/customerio/issue/MBL-141/expo-50-with-version-100-beta15-crash-startup-app-on-ios

Steps to reproduce the bug
There are no direct steps reported by the customer to reproduce the issue.
Other known ways to reproduce didFailToRegisterForRemoteNotificationsWithError :

  • Settings -> Notifications -> your app
  • Turn off notifications
  • Go back to app and attempt to register for push notifications
    OR
  • Turn off internet
  • Attempt to register for push notifications

How this PR fixes the bug
Let's understand what the bug is first. The issue is with the instance of messagingPush that is not initialised by the time it is called and hence the app crashes. This PR fixes it by using Self.shared instead of messagingPush and calls deleteDeviceToken method that further calls messagingPush.deleteDeviceToken() that has a living instance of messagingPush and hence the code is executed without any issue.

What you have done to test this PR
Since there are no steps reported by the customer, I reproduced the issue by calling messagingPush.deleteDeviceToken() in didRegisterForRemoteNotificationsWithDeviceToken instead of into method didFailToRegisterForRemoteNotificationsWithError. This helped me figure out where the issue lies.

Complete each step to get your pull request merged in. Learn more about the workflow this project uses.

  • Assign members of your team to review the pull request.
  • Wait for pull request status checks to complete. If there are problems, fix them until you see that all status checks are passing.
  • Wait until the pull request has been reviewed and approved by a teammate
  • After code reviews are approved and you determine this PR is ready to merge, select Squash and Merge button on this screen, leave the title and description to the default values, then merge the PR.

@ami-aman ami-aman requested a review from a team February 29, 2024 10:13
Copy link

github-actions bot commented Feb 29, 2024

Sample app builds 📱

Below you will find the list of the latest versions of the sample apps. It's recommended to always download the latest builds of the sample apps to accurately test the pull request.


  • CocoaPods-FCM: aman/crash-fix (1709632239)
  • APN-UIKit: aman/crash-fix (1709632200)

Copy link

codecov bot commented Feb 29, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 56.79%. Comparing base (251a958) to head (b6b9ec5).
Report is 3 commits behind head on main.

❗ Current head b6b9ec5 differs from pull request most recent head 1f16b0e. Consider uploading reports for the commit 1f16b0e to get more accurate results

Files Patch % Lines
...essagingPushAPN/MessagingPushAPN+PushConfigs.swift 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #575      +/-   ##
==========================================
+ Coverage   56.25%   56.79%   +0.53%     
==========================================
  Files         132      132              
  Lines        3715     3717       +2     
==========================================
+ Hits         2090     2111      +21     
+ Misses       1625     1606      -19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@levibostian levibostian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you write tests for this change?

It would be great if we had a test that could reproduce the crash and then show that after this change the test passes.

@ami-aman
Copy link
Contributor Author

ami-aman commented Mar 1, 2024

Can you write tests for this change?

It would be great if we had a test that could reproduce the crash and then show that after this change the test passes.

@levibostian I started writing test cases for this swizzled method but it looks like a larger effort and updates than I anticipated. As this fix is a result of a crash report by a customer, would would mind if I open a tech debt and let go this fix ?

@ami-aman ami-aman requested review from levibostian and a team March 5, 2024 09:49
Copy link
Member

@levibostian levibostian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I checked the CI status checks that are failing in this PR and they are also failing in main. Therefore, it's OK with me to merge this PR with the current failed status checks and fix the main branch in a separate PR.

@ami-aman ami-aman merged commit ac70292 into main Mar 5, 2024
5 of 8 checks passed
@ami-aman ami-aman deleted the aman/crash-fix branch March 5, 2024 15:01
github-actions bot pushed a commit that referenced this pull request Mar 5, 2024
## [2.12.3](2.12.2...2.12.3) (2024-03-05)

### Bug Fixes

* expo users reported app crash on didFailToRegisterForRemoteNotificationsWithError ([#575](#575)) ([ac70292](ac70292))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants