react-native-onesignal icon indicating copy to clipboard operation
react-native-onesignal copied to clipboard

[Bug]: Crash ModelStore.add java.util.ConcurrentModificationException

Open Chasty opened this issue 9 months ago • 7 comments

What happened?

Crashlytics are reporting those crashes.

Steps to reproduce?

-

What did you expect to happen?

React Native OneSignal SDK version

5.0.0

Which platform(s) are affected?

  • [ ] iOS
  • [X] Android

Relevant log output

com.onesignal.common.modeling
ModelStore.kt:154

Fatal Exception: java.util.ConcurrentModificationException:
       at java.util.ArrayList$Itr.checkForComodification(ArrayList.java:1029)
       at java.util.ArrayList$Itr.next(ArrayList.java:982)
       at com.onesignal.common.modeling.ModelStore.add(ModelStore.kt:154)
       at com.onesignal.common.modeling.IModelStore$DefaultImpls.add$default(IModelStore.kt:37)
       at com.onesignal.core.internal.operations.impl.OperationRepo.internalEnqueue(OperationRepo.kt:76)
       at com.onesignal.core.internal.operations.impl.OperationRepo.enqueue(OperationRepo.kt:60)
       at com.onesignal.core.internal.operations.IOperationRepo$DefaultImpls.enqueue$default(IOperationRepo.kt:16)
       at com.onesignal.core.internal.operations.listeners.SingletonModelStoreListener.onModelUpdated(SingletonModelStoreListener.kt:51)
       at com.onesignal.common.modeling.SingletonModelStore$onModelUpdated$1.invoke(SingletonModelStore.kt:50)
       at com.onesignal.common.modeling.SingletonModelStore$onModelUpdated$1.invoke(SingletonModelStore.kt:50)
       at com.onesignal.common.events.EventProducer.fire(EventProducer.kt:44)
       at com.onesignal.common.modeling.SingletonModelStore.onModelUpdated(SingletonModelStore.kt:50)
       at com.onesignal.common.modeling.ModelStore$onChanged$1.invoke(ModelStore.kt:74)
       at com.onesignal.common.modeling.ModelStore$onChanged$1.invoke(ModelStore.kt:74)
       at com.onesignal.common.events.EventProducer.fire(EventProducer.kt:44)
       at com.onesignal.common.modeling.ModelStore.onChanged(ModelStore.kt:74)
       at com.onesignal.common.modeling.Model$notifyChanged$1.invoke(Model.kt:295)
       at com.onesignal.common.modeling.Model$notifyChanged$1.invoke(Model.kt:295)
       at com.onesignal.common.events.EventProducer.fire(EventProducer.kt:44)
       at com.onesignal.common.modeling.Model.notifyChanged(Model.kt:295)
       at com.onesignal.common.modeling.Model.notifyChanged(Model.kt:300)
       at com.onesignal.common.modeling.Model.setOptAnyProperty(Model.kt:200)
       at com.onesignal.common.modeling.Model.setOptAnyProperty$default(Model.kt:187)
       at com.onesignal.common.modeling.MapModel.put(MapModel.kt:47)
       at com.onesignal.common.modeling.MapModel.put(MapModel.kt:8)
       at com.onesignal.user.internal.UserManager.addTag(UserManager.kt:173)
       at com.onesignal.rnonesignalandroid.RNOneSignal.addTag(RNOneSignal.java:551)
       at java.lang.reflect.Method.invoke(Method.java)
       at com.facebook.react.bridge.JavaMethodWrapper.invoke(JavaMethodWrapper.java:372)
       at com.facebook.react.bridge.JavaModuleWrapper.invoke(JavaModuleWrapper.java:188)
       at com.facebook.jni.NativeRunnable.run(NativeRunnable.java)
       at android.os.Handler.handleCallback(Handler.java:984)
       at android.os.Handler.dispatchMessage(Handler.java:104)
       at com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(MessageQueueThreadHandler.java:27)
       at android.os.Looper.loopOnce(Looper.java:238)
       at android.os.Looper.loop(Looper.java:357)
       at com.facebook.react.bridge.queue.MessageQueueThreadImpl$4.run(MessageQueueThreadImpl.java:228)
       at java.lang.Thread.run(Thread.java:1012)

Code of Conduct

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

Chasty avatar Oct 02 '23 16:10 Chasty

Same problem on OneSignalNotifications.checkNotificationPermission(); function

HugoKessler avatar Oct 05 '23 09:10 HugoKessler

I was doing

  promptForNotificationsPermission = async () => {
    OneSignal.User.pushSubscription.optIn(); // not working, lol
    await OneSignal.Notifications.requestPermission(true);
    this.getOneSignalPushInfos('promptForNotificationsPermission');
  };

and I had the same bug

removing the non-working-(at-least-for-me) OneSignal.User.pushSubscription.optIn() made the crash disappear and the behaviour correct

arnaudambro avatar Oct 06 '23 12:10 arnaudambro

Hi @Chasty, thanks for reporting this crash. I don't see a line 154 in v5.0.0 of the ModelStore.kt file. Can you confirm it is crashing on line 154, as I am perplexed.

Hi @HugoKessler AND @arnaudambro,

Are the crashes you are seeing occurring in the same place as the Original Poster? In the ModelStore's add() method? Can you share your crash logs as well?

nan-li avatar Oct 06 '23 19:10 nan-li

I fixed it so I don't remember sorry, but I think it was directly at com.onesignal.common.events.EventProducer.fire(EventProducer.kt:44)

But you can reproduce easily I think with this code

  promptForNotificationsPermission = async () => {
    OneSignal.User.pushSubscription.optIn(); // this function did not work for me, and having it with the next one creates the bug
    await OneSignal.Notifications.requestPermission(true);
    this.getOneSignalPushInfos('promptForNotificationsPermission');
  };

arnaudambro avatar Oct 10 '23 03:10 arnaudambro

Hi @arnaudambro,

It looks like you have 2 issues you are reporting:

  1. OneSignal.User.pushSubscription.optIn() not working for you.

  2. The ConcurrentModificationException which we are actively working on the fix.

Please note that the 2 methods OneSignal.User.pushSubscription.optIn() and OneSignal.Notifications.requestPermission(bool) have the same functionality to request notification permission. Unless you have previously opted out by calling OneSignal.User.pushSubscription.optOut(), you should only use one of these 2 methods.

nan-li avatar Oct 12 '23 18:10 nan-li

Please note that the 2 methods OneSignal.User.pushSubscription.optIn() and OneSignal.Notifications.requestPermission(bool) have the same functionality to request notification permission. Unless you have previously opted out by calling OneSignal.User.pushSubscription.optOut(), you should only use one of these 2 methods.

ok well noted I think it's weird to have two methods for the same job I didn't know which one to use so I think IIRC I used both to check then it seemed one was not working (OneSignal.User.pushSubscription.optIn()) so I let it but actually it was causing the bug ConcurrentModificationException

so if you had only one method, I would

  • not have hesitated
  • not have a bug 😁

maybe you could rename optIn to reOptIn or subscribeAgain ? so that we know which one we should use ? I don't know...

anyway, thanks for clarification, and good luck, the new API in 5.0 seems to give a better DX !

arnaudambro avatar Oct 17 '23 14:10 arnaudambro

Hi @arnaudambro,

Thanks for that feedback, you're right that it can be pretty confusing. We will have to think best way to communicate this.

The default method should be OneSignal.User.pushSubscription.optIn() and OneSignal.Notifications.requestPermission(bool) is meant for developers who want greater control over the push prompt, such as if you want to show a dialog prompting them to go to system settings if they have already disabled permission, or you want to receive their response to the prompt.

nan-li avatar Nov 03 '23 23:11 nan-li

Just a heads up ICYMI: ConcurrentModificationException errors were resolved in Q1. Please upgrade to our latest SDK version to resolve. Please let us know if you have any further questions or concerns!

jennantilla avatar Apr 18 '24 19:04 jennantilla