CrossGeeks / FirebasePushNotificationPlugin

Firebase Push Notification Plugin for Xamarin iOS and Android
MIT License
396 stars 178 forks source link

CrossFirebasePushNotification.Current.UnregisterForPushNotifications() crashes app #367

Open jtorvald opened 4 years ago

jtorvald commented 4 years ago

🐛 Bug Report

Value cannot be null. Parameter name: str at Foundation.NSString..ctor (System.String str) [0x0002e] in /Library/Frameworks/Xamarin.iOS.framework/Versions/14.7.0.166/src/Xamarin.iOS/Foundation/NSString.cs:162 at Foundation.NSUserDefaults.SetString (System.String value, System.String defaultName) [0x00000] in /Library/Frameworks/Xamarin.iOS.framework/Versions/14.7.0.166/src/Xamarin.iOS/Foundation/NSUserDefaults.cs:37 at Plugin.FirebasePushNotification.FirebasePushNotificationManager.DidReceiveRegistrationToken (Firebase.CloudMessaging.Messaging messaging, System.String fcmToken) [0x0006f] in /Users/runner/runners/2.169.1/work/1/s/Plugin.FirebasePushNotification/FirebasePushNotificationManager.ios.cs:567 at (wrapper managed-to-native) UIKit.UIApplication.UIApplicationMain(int,string[],intptr,intptr) at UIKit.UIApplication.Main (System.String[] args, System.IntPtr principal, System.IntPtr delegate) [0x00005] in /Library/Frameworks/Xamarin.iOS.framework/Versions/14.7.0.166/src/Xamarin.iOS/UIKit/UIApplication.cs:86 at UIKit.UIApplication.Main (System.String[] args, System.String principalClassName, System.String delegateClassName) [0x0000e] in /Library/Frameworks/Xamarin.iOS.framework/Versions/14.7.0.166/src/Xamarin.iOS/UIKit/UIApplication.cs:65 at SinglesProject.iOS.Application.Main (System.String[] args) [0x0001f] in /Users/Developer/Projects/Apps/App.iOS/Main.cs:18

Expected behavior

Set the token to empty string

Reproduction steps

For me I can reproduce it by adding this line of code to my logout method: CrossFirebasePushNotification.Current.UnregisterForPushNotifications()

The app logs out and shows the login screen for a second and then crashes.

Configuration

Version: 3.3.10

Platform:

antixaker commented 4 years ago

I faced the same issue after Firebase packages update. Probably, updating Firebase packages in Plugin will fix the issue (it needs more investigation).

sasa-bobic commented 4 years ago

This line: https://github.com/CrossGeeks/FirebasePushNotificationPlugin/blob/cda71416f3f847df1b01708868bc1ef1196e33f6/Plugin.FirebasePushNotification/FirebasePushNotificationManager.ios.cs#L305

triggers a call to Plugin.FirebasePushNotification.FirebasePushNotificationManager.DidReceiveRegistrationToken(Messaging messaging, string fcmToken)

This line: https://github.com/CrossGeeks/FirebasePushNotificationPlugin/blob/cda71416f3f847df1b01708868bc1ef1196e33f6/Plugin.FirebasePushNotification/FirebasePushNotificationManager.ios.cs#L568

crashes the app.

It seems "fcmToken" is null and app tries to construct NSString with null string (that's what crashes the app).

Easy fix should be: NSUserDefaults.StandardUserDefaults.SetString(fcmToken ?? string.Empty, FirebaseTokenKey);

jtorvald commented 4 years ago

@sasa-bobic seems like it. I just looked into it quick with the intent to create a pull request. The thing is that the user defaults is already overwritten to string.Empty before. I'd wonder if it's not better to move up that line of code to within the if(!String.IsNullOrEmpty()) block so it will never be set when empty or null. But that might mess up more logic if for some reason it can be null or empty string on another condition except for unregistered.

Besides that: from how it works now, the OnTokenRefresh in that case will also not be called with unregister, so the developer needs to be aware to remove the token him-/herself while on unregistered.

Not sure what the wanted behavior is here.

LeoJHarris commented 3 years ago

As of latest version app crashes on android when Calling the UnregisterForPushNotifications()

jtorvald commented 3 years ago

Is this library still being maintained?

vhugogarcia commented 3 years ago

I don't know @jtorvald, but I hope this fix is applied soon along with the updated firebase cloud messaging package.

LeoJHarris commented 3 years ago

@jtorvald I think you found your answer..