TobiasBuchholz / Plugin.Firebase

Wrapper around the native Android and iOS Firebase Xamarin SDKs
MIT License
211 stars 49 forks source link

CrossFirebaseCloudMessaging.Current.NotificationTapped fired up twice if app is in a background state #285

Closed mizuhs closed 5 months ago

mizuhs commented 5 months ago

Hello,

I've been running some tests on the plugin on Android and I get a strange behavior whenever I click on a notification while the app is in the background, i.e. neither in the foreground or killed from the task manager:

I noticed that the handler at CrossFirebaseCloudMessaging.Current.NotificationTapped gets called twice in this scenario.

Here's my testing App.xaml OnStart method, as I copied it from some examples here. A simple log that registers a Received or Tapped event when it fires up (I keep the text on the app itself because I also test it while not debugging on VS):

protected override void OnStart()
{
    try
    {
        CrossFirebaseCloudMessaging.Current.CheckIfValidAsync().Wait();

        CrossFirebaseCloudMessaging.Current.Error += (s, p) =>
        {
            if (p == null)
                return;

            Console.WriteLine($"PushNotification.Error, Message = {p.Message}");
        };

        CrossFirebaseCloudMessaging.Current.TokenChanged += (s, p) =>
        {
            if (p == null || String.IsNullOrWhiteSpace(p.Token))
                return;

            FcmTokenRegistration(p.Token);
        };

        CrossFirebaseCloudMessaging.Current.NotificationReceived += (s, p) =>
        {
            if (p == null || p.Notification == null)
                return;
            if (p.Notification.Data.ContainsKey("Body"))
            {
                SecureStorage.SetAsync("NotificationLog", SecureStorage.GetAsync("NotificationLog").Result + "RECEIVED " + p.Notification.Data["Body"] + DateTime.Now.ToString(" yyyy-MM-dd HH:mm:ss") + System.Environment.NewLine);
            }
        };

        CrossFirebaseCloudMessaging.Current.NotificationTapped += (s, p) =>
        {
            if (p == null || p.Notification == null)
                return;
            if (p.Notification.Data.ContainsKey("Body"))
            {
                SecureStorage.SetAsync("NotificationLog", SecureStorage.GetAsync("NotificationLog").Result + "TAPPED " + p.Notification.Data["Body"] + DateTime.Now.ToString(" yyyy-MM-dd HH:mm:ss") + System.Environment.NewLine);
            }

            Console.WriteLine($"PushNotification.NotificationTapped");
        };
    }
    catch (Exception ex)
    {
        Console.WriteLine($"App.OnStart() crashed\n{ex}");
    }
}

I actually tried getting something out of ChatGPT and it said something I was already suspecting:

The behavior you're describing, where the NotificationTapped event fires twice when the app is in the background state and the user taps the notification, could be attributed to the way background execution and notification handling are managed within the .NET MAUI application lifecycle, combined with the behavior of the Firebase Cloud Messaging plugin.

When the user taps a notification while the app is in the background, the system typically launches the app and delivers the notification data to it. However, if the app was already in the background (e.g., it had been previously launched but then sent to the background), the system may bring it to the foreground to handle the notification tap event. This could result in the event being handled twice—once when the app is brought to the foreground and again when the app resumes normal execution.

To address this issue, you may need to implement logic within your app to prevent redundant handling of the NotificationTapped event. One approach could be to use a flag or a semaphore to track whether the event has already been handled, and skip the handling logic if it has. Additionally, you might need to carefully manage the app's lifecycle and understand how the Firebase Cloud Messaging plugin interacts with it, ensuring that events are handled appropriately based on the app's current state.

The thing is, can I achieve a solution to this redundancy within the plugin functions themselves? If not, is there an easy way to identify the state the app is in in MAUI, like foreground, new instance or coming back from background?

Thanks in advance

angelru commented 5 months ago

Hello,

I've been running some tests on the plugin on Android and I get a strange behavior whenever I click on a notification while the app is in the background, i.e. neither in the foreground or killed from the task manager:

I noticed that the handler at CrossFirebaseCloudMessaging.Current.NotificationTapped gets called twice in this scenario.

Here's my testing App.xaml OnStart method, as I copied it from some examples here. A simple log that registers a Received or Tapped event when it fires up (I keep the text on the app itself because I also test it while not debugging on VS):

protected override void OnStart()
{
    try
    {
        CrossFirebaseCloudMessaging.Current.CheckIfValidAsync().Wait();

        CrossFirebaseCloudMessaging.Current.Error += (s, p) =>
        {
            if (p == null)
                return;

            Console.WriteLine($"PushNotification.Error, Message = {p.Message}");
        };

        CrossFirebaseCloudMessaging.Current.TokenChanged += (s, p) =>
        {
            if (p == null || String.IsNullOrWhiteSpace(p.Token))
                return;

            FcmTokenRegistration(p.Token);
        };

        CrossFirebaseCloudMessaging.Current.NotificationReceived += (s, p) =>
        {
            if (p == null || p.Notification == null)
                return;
            if (p.Notification.Data.ContainsKey("Body"))
            {
                SecureStorage.SetAsync("NotificationLog", SecureStorage.GetAsync("NotificationLog").Result + "RECEIVED " + p.Notification.Data["Body"] + DateTime.Now.ToString(" yyyy-MM-dd HH:mm:ss") + System.Environment.NewLine);
            }
        };

        CrossFirebaseCloudMessaging.Current.NotificationTapped += (s, p) =>
        {
            if (p == null || p.Notification == null)
                return;
            if (p.Notification.Data.ContainsKey("Body"))
            {
                SecureStorage.SetAsync("NotificationLog", SecureStorage.GetAsync("NotificationLog").Result + "TAPPED " + p.Notification.Data["Body"] + DateTime.Now.ToString(" yyyy-MM-dd HH:mm:ss") + System.Environment.NewLine);
            }

            Console.WriteLine($"PushNotification.NotificationTapped");
        };
    }
    catch (Exception ex)
    {
        Console.WriteLine($"App.OnStart() crashed\n{ex}");
    }
}

I actually tried getting something out of ChatGPT and it said something I was already suspecting:

The behavior you're describing, where the NotificationTapped event fires twice when the app is in the background state and the user taps the notification, could be attributed to the way background execution and notification handling are managed within the .NET MAUI application lifecycle, combined with the behavior of the Firebase Cloud Messaging plugin.

When the user taps a notification while the app is in the background, the system typically launches the app and delivers the notification data to it. However, if the app was already in the background (e.g., it had been previously launched but then sent to the background), the system may bring it to the foreground to handle the notification tap event. This could result in the event being handled twice—once when the app is brought to the foreground and again when the app resumes normal execution.

To address this issue, you may need to implement logic within your app to prevent redundant handling of the NotificationTapped event. One approach could be to use a flag or a semaphore to track whether the event has already been handled, and skip the handling logic if it has. Additionally, you might need to carefully manage the app's lifecycle and understand how the Firebase Cloud Messaging plugin interacts with it, ensuring that events are handled appropriately based on the app's current state.

The thing is, can I achieve a solution to this redundancy within the plugin functions themselves? If not, is there an easy way to identify the state the app is in in MAUI, like foreground, new instance or coming back from background?

Thanks in advance

try putting all that code outside of OnStart, or assign a method to the event and before recreating it remove it.

mizuhs commented 5 months ago

Thanks for your suggestion.

I tried putting both private void OnNotificationReceived(object s, FCMNotificationReceivedEventArgs p) and private void OnNotificationTapped(object s, FCMNotificationTappedEventArgs p) as their own methods outside of OnStart() and in the App() constructor I added:

CrossFirebaseCloudMessaging.Current.NotificationReceived += OnNotificationReceived;
CrossFirebaseCloudMessaging.Current.NotificationTapped += OnNotificationTapped;

But now the duplication strangely also happens when the app is killed and the user taps a notification. Previsouly the killed-then-created-anew state didn't result in duplication of the handling. The only instance where it's not duplicated now is when the notification tap is handled while the app is in the foreground.

mizuhs commented 5 months ago

Well, I actually resorted to ChatGPT again and after a couple of prompts it came up with the suggestion to use an id table:

private HashSet<string> processedNotificationIds = new HashSet<string>();
private void OnNotificationTapped(object s, FCMNotificationTappedEventArgs p)
{
    if (p == null || p.Notification == null)
        return;

    string notificationId = p.Notification.Data["Id"];

    if (processedNotificationIds.Contains(notificationId))
        return;

    if (p.Notification.Data.ContainsKey("Body"))
    {
        // Handle notification tapped event here
        SecureStorage.SetAsync("NotificationLog", SecureStorage.GetAsync("NotificationLog").Result + "TAPPED " + p.Notification.Data["Body"] + DateTime.Now.ToString(" yyyy-MM-dd HH:mm:ss") + System.Environment.NewLine);
    }

    processedNotificationIds.Add(notificationId);

    Console.WriteLine($"PushNotification.NotificationTapped");
}

Well, it actually worked. It suggested the usage of a lock variable as an extreme measure but it wasn't really necessary. This way works well with all three states mentioned without firing the same notification twice in any of them. Of course the handled id list is lost when the app is killed but by then it doesn't really matter.

I didn't see this issue being reported here before, and I don't think it has anything to do with the plugin itself but I believe it's probably not very uncommon. It might even happen unnoticed in some scenarios but is harmless enough in them that it's not really a thing. But if someone really needs to prevent this duplicity I think this is a simple workaround.