[Bug]: OneSignal#onWillDisplayNotification getting called as many times as app gets hot restarted while debugging.

[luis901101]

What happened?

The callback for _handleMethod(MethodCall call) function on OneSignalNotifications class is getting called as many times as the app get's Hot Restarted, so listeners gets called several times times during debugging.

Steps to reproduce?

No need to add any listener, just put a break point on first line in `Future<Null> _handleMethod(MethodCall call) async` on `OneSignalNotifications`, then:
1. Run app in debug mode.
2. Initialize OneSignal
3. Do Hot Restart X times
4. Send one push notification to your debugging device
5. Note the function getting called X times.

What did you expect to happen?

The callback should be getting called just one time by notification event on each app hot restart.

OneSignal Flutter SDK version

5.0.3

Which platform(s) are affected?

• [X] iOS
• [X] Android

Relevant log output

No response

Code of Conduct

• [X] I agree to follow this project's Code of Conduct

[luis901101]

With OneSignal#onClickNotification happens similar, in this case as it's a manual event you have to add a listener for that event, so when calling addClickListener(...) it's triggering a _channel.invokeMethod("OneSignal#addNativeClickListener");, so every time you hot-restart your app and add your click listener the same channel method will be invoked and then you will be called back several times, one by each hot-restart.

The problem seems that on the native code side a List of listeners is kept, and this list remains the same on each hot-restart, so on each OneSignal initialization or adding click listener, the native list of listeners increases.

[emawby]

@luis901101 Thank you for reporting we will investigate. In the meantime the corresponding removeListener most likely needs to be called to remove the old listener or you could try not adding the second listeners on hot reload

[luis901101]

Sure, the remove listener should be properly called, but note in my description above I'm talking about Hot Restart not Hot Reload, and also about the OneSignal#onWillDisplayNotification it doesn't matter the listener, I mean, it will be called X times depending on the amount of X times you Hot Restart the app, no matter if you added a listener or not, because it seems that on native implementation side the listeners list are being added on every OneSignal.initialize().

[luis901101]

The call to OneSignal.Notifications.addForegroundWillDisplayListener(listener); is being handled only on dart side, I mean the listener you add there will be added only to: _willDisplayListeners.add(listener);, later when some notification is received the Future<Null> _handleMethod(MethodCall call) is called and _willDisplayListeners will be checked to notify each listener... but no matter if you added listeners to _willDisplayListeners or not... the handler will be called X times depending on the amount of times the app has been Hot Restarted. Then if you have added 1 listener, but you have Hot Restarted your app 10 times your listener will be called 10 times...

[emawby]

@luis901101 I see thank you for investigating! We will work on a fix for this

[alainjr10]

With OneSignal#onClickNotification happens similar, in this case as it's a manual event you have to add a listener for that event, so when calling addClickListener(...) it's triggering a _channel.invokeMethod("OneSignal#addNativeClickListener");, so every time you hot-restart your app and add your click listener the same channel method will be invoked and then you will be called back several times, one by each hot-restart.

The problem seems that on the native code side a List of listeners is kept, and this list remains the same on each hot-restart, so on each OneSignal initialization or adding click listener, the native list of listeners increases.

I been stuck on this issue for a few days now, and i couldn't quite reproduce it until now. Have you got a temporary workaround for this? @luis901101

[luis901101]

Nope, I just avoid onesignal initialization during debug unless I need to test something related to notifications which is sporadical.

[temcewen]

+1 I am also experiencing this issue

[meroo36]

+1

[ologunB]

+1

[Jeonguk-ninehire]

+1

[frankenten13]

+1

[singlapriyanka315]

facing same issue

[csnguyenskg]

same here

[vr0707]

Please help any one!!! i used one signal 5.0.4 version, adclicklistener lisent multiple times. how to handle this ?

[arhamsc]

Is there any ETA to when this issue is going to get resolved? Also will this affect the "release" version of flutter app or only debug?

[bugrevealingbme]

Same

[nan-li]

EDIT: I meant Hot Restart in this comment

Hi everyone, thanks for your reports.

The OneSignal-Flutter-SDK does support adding multiple listeners of each type. After investigating, this seems like a Flutter Hot Reload issue, and I am not sure how the SDK can work around it without impacting real usage.

When you run a Hot Reload, it doesn't cold start the app but still reruns your initialization code, which is why your listeners are added again. Just put a breakpoint or print statement in the place where you are adding your listener and you will see the Hot Reload does call your code again to add the listeners.

Because it is not a cold start, all of the SDK's data remains in memory and it cannot distinguish between a Hot Reload vs. normal app flow or backgrounding, etc. The SDK cannot distinguish that you are adding a listener for real or Hot Reload is adding the listener.

If you have suggestions how to work around this for easier debugging, please let us know. It doesn't seem the SDK can make this fix.

[luis901101]

Hi @nan-li and thanks for your feedback, but note that in my first comment I state Hot Restarts not Hot Reloads

The callback for _handleMethod(MethodCall call) function on OneSignalNotifications class is getting called as many times as the app get's Hot Restarted, so listeners gets called several times times during debugging.

So the way I see it, if the app is Hot Restarted the OneSignal.initialize(appId); will be called again and at this point the plugin should reset any previous state, I mean all listeners should be cleaned wether at dart side or native side.

[PetrKubes97]

I'm encountering this issue as well and I think resetting all listeners on native side when calling OneSignal.initialize as @luis901101 suggested would be the best behavior.

[nan-li]

Hi @nan-li and thanks for your feedback, but note that in my first comment I state Hot Restarts not Hot Reloads

The callback for _handleMethod(MethodCall call) function on OneSignalNotifications class is getting called as many times as the app get's Hot Restarted, so listeners gets called several times times during debugging.

So the way I see it, if the app is Hot Restarted the OneSignal.initialize(appId); will be called again and at this point the plugin should reset any previous state, I mean all listeners should be cleaned wether at dart side or native side.

Hi @luis901101,

I apologize, I misspoke in my comment, I meant to say Hot Restart as that is the flow I tested. I disagree that calling OneSignal.initialize(appId) should reset the SDK. Imagine this setup scenario:

    OneSignal.Debug.setLogLevel(OSLogLevel.verbose);
    OneSignal.consentRequired(_requireConsent);

    OneSignal.Notifications.addForegroundWillDisplayListener((event) {
        print('Foreground display listener triggered');
    });

    OneSignal.Notifications.addClickListener((event) {
        print('NOTIFICATION CLICK LISTENER CALLED WITH EVENT: $event');
    });

    OneSignal.initialize("YOUR_APP_ID");

There are some methods that could be called before OneSignal.initialize. All of the setup work done by the developer would be erased. This would be a significant behavior change and lead to unexpected behavior, just for the sake of Hot Reloads. If the SDK is already initialized and another call to initialize it is made, I would expect a no-op.

[luis901101]

Hi @nan-li thanks again for you feedback and your tests, oki then we are on the same page with the Hot Restart (by the way you mentioned Hot Reloads again at the end of your comment 😉)

Agree, resetting the whole plugin state is not ideal, but resetting only the listeners I think it makes sense and in fact only the listener are the ones with the problems mentioned in this thread so no need to reset anything else.

From your sample code I understand the setLogLevel and consentRequired could be called before initialization but I'm not sure the listeners make sense to be added before initialization, I mean, is there any case in which some of these listeners would be called if the plugin has not been initialized?

[PetrKubes97]

Thanks for the reply. I understand that changing the behavior in such way is not ideal, even though I myself wouldn't think of doing anything before initialization.

What about a method that clears all listeners in both Flutter and native side? Keeping track of the function references for their removal is annoying anyway.

[nan-li]

Hi @luis901101 oops yes you are right, I just can't stop saying Hot Reloads 😆.

The thing is we would like to avoid any "gotchas" that SDK users have to watch for. If they have complex code and already called initialize but then later call it again, we shouldn't produce unexpected behavior. This is same if they do make some add listener calls before calling initialize.

I also recall on our previous major release with the player-based model, some people's notification opened handlers didn't trigger early enough when the app is cold started from the open event. We had suggested to some users to add the opened handler as early as possible.

In addition, clearing listeners on initialize will differ from all the other OneSignal SDKs (native Android, native iOS, react native, etc), which is not preferable behavior either. There has not been issues with listeners being added on other SDKs since Hot Restarts is a Flutter specific issue.

@PetrKubes97 You should manage the removal and addition of listeners when debugging only. Perhaps call the remove method before the add methods during debugging with Hot Restarts.

[PetrKubes97]

@nan-li sorry if this comment is inaccurate, I'm trying to remember the code and typing it on a phone 😅.

From what I remember, the remove method only removes the listener from the list of listeners in flutter with native listeners still being registered. What happens then is that there are two native listeners, which call the flutter listener twice (even though there is only one in the list)

[luis901101]

Ok let's break this into parts:

1. IMPORTANT: the issue identified in this thread has nothing to do with SDK users adding/removing listeners.
2. Back to my initial comment I refer to the _handleMethod(MethodCall call) function getting called as many times as the app get Hot Restarted, so no matter if the SDK user has added listers or not.
3. The SDK users you are trying to protect by not doing any change on the SDK initialization so they don't experience unexpected behavior, those users are being affected anyway by this issue which by the way it is an unexpected behavior, (I'm not the only one reporting it 🤷‍♂️).
4. Ok, Hot Restart is a specific Flutter feature but this plugin is for Flutter, so we expect the plugin's behavior is according to the Flutter dev tools, Hot Restarting a debugging app is a valid use case, so the Plugin should work fine with hot restarts.

Follow this simple test case carefully (so far I only tested it on iOS):

1. Create a simple Flutter OneSignal app project.
2. Make sure you initialize OneSignal but DO NOT ADD ANY LISTENERS.
3. Run your app in debug mode
4. Put a break point inside the Future<Null> _handleMethod(MethodCall call){...} located in notifications.dart class on OneSignal plugin, specifically on the Line 133: https://github.com/OneSignal/OneSignal-Flutter-SDK/blob/0996e59a7162bda6f84f284592f1835154239f91/lib/src/notifications.dart#L126-L145
5. From OneSignal dashboard send a test notification to your device. (Create a test segment to avoid sending these notifications to real users)
6. Hot Restart your app and repeat from point 5.

If you hot restart your app 5 times, when you send the test notification to your device, you will see the _handleMethod(MethodCall call) get's called 6 times, in fact when this several calls happens the plugin shows an ERROR in the logs, something like:

2024-01-29 22:11:25.899960+0000 Test App[4657:128228] ERROR: OSNotificationWillDisplayEvent.notification.display cannot be called due to timing out or notification was already displayed.

[temcewen]

@nan-li Hi Nan,

First, I'd like to say that I appreciate the work you and the OneSignal team are doing!

I'd like to address your comment when you said, 'we would like to avoid any "gotchas" that SDK users have to watch for' and then proceeded to explain the reasoning behind allowing behaviors such as duplicate initialization calls or allowing hooks to be set before initialization is called.

Would it not be a "gotcha" that people will probably expect that calling initialize sometime after an app has started should re-initialize the app and clear any existing hooks? A call to "initialize" something implies that you have to call it before anything else. It implies that you're dealing with a state machine and it implies that after a call to "initialize" the state machine, you will be working with a "clean state".

By allowing the API to be used in erroneous ways like disregarding when initialized is called, ignoring duplicate initialize calls, etc, you actually significantly worsen the developer experience. For example, if someone is unintentionally calling initialize, don't you think they would want to know? Don't you think they would want to know, "Hey, you're calling this function twice when you only need to call it once just FYI."

This is akin to what JavaScript does when it allows users to do var x = "0" + 1 and then returns a value and WHO KNOWS what the type will be. It's antithetical to safe object oriented programming. Abstractions should use exceptions and/or warnings to indicate to developers when they are used incorrectly rather than just allowing the developer to do it and then ASSUMING some intention on behalf of the dev that obviously can't be known without explicitly asking.

I apologize for being a little dogmatic, but I would really appreciate some concrete reasons behind why initialize shouldn't clear the existing OneSignal state. Perhaps it could give a warning when it does so?

What if there were a happy medium where initialize has an optional parameter bool resetState = false?

[PetrKubes97]

Maybe a quick fix: Just move the _clickHandlerRegistered = true; to iOS and Android part of the SDK? But tbh random flags like this seem a bit hacky.

I think it could be done more declaratively, checking whether listeners are registered rather than checking some flag. The current bug is caused by the mismatch between the flags state and the registered listeners state.

[AchmadSyarippudin]

same issue on onesignal 5.2, this anybody have solved this problem ?

[sannykhan3777]

Have the same issue. I'm navigating to another screen on click of notification but get 10+ same screens under the navigation stack because of listener calling itself multiple times.

Onesignal SDK = onesignal_flutter: ^5.1.0 Flutter SDK = 3.16.3

OneSignal.Notifications.addClickListener((event) { String? screen = event.notification.additionalData!["screen"]; if(screen != null){ onesignalNavigatorKey.currentState!.pushNamed(RoutesName.orders_home_screen); } });