Fix: Add synchronized blocks to prevent ConcurrentModificationException

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/events/EventProducer.kt

@@ -16,19 +16,25 @@ open class EventProducer<THandler> : IEventNotifier<THandler> {
     private val subscribers: MutableList<THandler> = Collections.synchronizedList(mutableListOf())
 
     override fun subscribe(handler: THandler) {
-        subscribers.add(handler)
+        synchronized(subscribers) {
+            subscribers.add(handler)
+        }
     }
 
     override fun unsubscribe(handler: THandler) {
-        subscribers.remove(handler)
+        synchronized(subscribers) {
+            subscribers.remove(handler)
+        }
     }
 
     /**
      * Subscribe all from an existing producer to this subscriber.
      */
     fun subscribeAll(from: EventProducer<THandler>) {
-        for (s in from.subscribers) {
-            subscribe(s)
+        synchronized(subscribers) {
+            for (s in from.subscribers) {
+                subscribe(s)
+            }
         }
     }
 
@@ -39,8 +45,10 @@ open class EventProducer<THandler> : IEventNotifier<THandler> {
      * @param callback The callback will be invoked for each subscribed handler, allowing you to call the handler.
      */
     fun fire(callback: (THandler) -> Unit) {
-        for (s in subscribers) {
-            callback(s)
+        synchronized(subscribers) {
+            for (s in subscribers) {
+                callback(s)
+            }
         }
     }
 

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/Model.kt

@@ -51,12 +51,15 @@ open class Model(
      * specified, must also specify [_parentModel]
      */
     private val _parentProperty: String? = null,
+    private val modelSynchronizationLock: Any = Any(),
 ) : IEventNotifier<IModelChangedHandler> {
     /**
      * A unique identifier for this model.
      */
     var id: String
-        get() = getStringProperty(::id.name)
+        get() = synchronized(modelSynchronizationLock) {
+            getStringProperty(::id.name)
+        }
         set(value) {
             setStringProperty(::id.name, value)
         }
@@ -123,19 +126,25 @@ open class Model(
         id: String?,
         model: Model,
     ) {
-        data.clear()
+        val newData = mutableMapOf<String, Any?>()
+
         for (item in model.data) {
             if (item.value is Model) {
                 val childModel = item.value as Model
                 childModel._parentModel = this
-                data[item.key] = childModel
+                newData[item.key] = childModel
             } else {
-                data[item.key] = item.value
+                newData[item.key] = item.value
             }
         }
 
         if (id != null) {
-            data[::id.name] = id
+            newData[::id.name] = id
+        }
+
+        synchronized(modelSynchronizationLock) {
+            data.clear()
+            data.putAll(newData)
         }
     }
 
@@ -660,35 +669,39 @@ open class Model(
      * @return The resulting [JSONObject].
      */
     fun toJSON(): JSONObject {
-        val jsonObject = JSONObject()
-        for (kvp in data) {
-            when (val value = kvp.value) {
-                is Model -> {
-                    jsonObject.put(kvp.key, value.toJSON())
-                }
-                is List<*> -> {
-                    val jsonArray = JSONArray()
-                    for (arrayItem in value) {
-                        if (arrayItem is Model) {
-                            jsonArray.put(arrayItem.toJSON())
-                        } else {
-                            jsonArray.put(arrayItem)
+        synchronized(modelSynchronizationLock) {
+            val jsonObject = JSONObject()
+            for (kvp in data) {
+                when (val value = kvp.value) {
+                    is Model -> {
+                        jsonObject.put(kvp.key, value.toJSON())
+                    }
+                    is List<*> -> {
+                        val jsonArray = JSONArray()
+                        for (arrayItem in value) {
+                            if (arrayItem is Model) {
+                                jsonArray.put(arrayItem.toJSON())
+                            } else {
+                                jsonArray.put(arrayItem)
+                            }
                         }
+                        jsonObject.put(kvp.key, jsonArray)
+                    }
+                    else -> {
+                        jsonObject.put(kvp.key, value)
                     }
-                    jsonObject.put(kvp.key, jsonArray)
-                }
-                else -> {
-                    jsonObject.put(kvp.key, value)
                 }
             }
+            return jsonObject
         }
-        return jsonObject
     }
 
     override fun subscribe(handler: IModelChangedHandler) = changeNotifier.subscribe(handler)
 
     override fun unsubscribe(handler: IModelChangedHandler) = changeNotifier.unsubscribe(handler)
 
     override val hasSubscribers: Boolean
-        get() = changeNotifier.hasSubscribers
+        get() = synchronized(modelSynchronizationLock) {
+            changeNotifier.hasSubscribers
+        }
 }

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/ModelStore.kt

@@ -40,73 +40,87 @@ abstract class ModelStore<TModel>(
         model: TModel,
         tag: String,
     ) {
-        val oldModel = models.firstOrNull { it.id == model.id }
-        if (oldModel != null) {
-            removeItem(oldModel, tag)
-        }
+        synchronized(models) {
+            val oldModel = models.firstOrNull { it.id == model.id }
+            if (oldModel != null) {
+                removeItem(oldModel, tag)
+            }
 
-        addItem(model, tag)
+            addItem(model, tag)
+        }
     }
 
     override fun add(
         index: Int,
         model: TModel,
         tag: String,
     ) {
-        val oldModel = models.firstOrNull { it.id == model.id }
-        if (oldModel != null) {
-            removeItem(oldModel, tag)
-        }
+        synchronized(models) {
+            val oldModel = models.firstOrNull { it.id == model.id }
+            if (oldModel != null) {
+                removeItem(oldModel, tag)
+            }
 
-        addItem(model, tag, index)
+            addItem(model, tag, index)
+        }
     }
 
     override fun list(): Collection<TModel> {
         return models
     }
 
     override fun get(id: String): TModel? {
-        return models.firstOrNull { it.id == id }
+        synchronized(models) {
+            return models.firstOrNull { it.id == id }
+        }
     }
 
     override fun remove(
         id: String,
         tag: String,
     ) {
-        val model = models.firstOrNull { it.id == id } ?: return
-        removeItem(model, tag)
+        synchronized(models) {
+            val model = models.firstOrNull { it.id == id } ?: return
+            removeItem(model, tag)
+        }
     }
 
     override fun onChanged(
         args: ModelChangedArgs,
         tag: String,
     ) {
-        persist()
+        synchronized(models) {
+            persist()
 
-        changeSubscription.fire { it.onModelUpdated(args, tag) }
+            changeSubscription.fire { it.onModelUpdated(args, tag) }
+        }
     }
 
     override fun replaceAll(
         models: List<TModel>,
         tag: String,
     ) {
-        clear(tag)
+        synchronized(models) {
+            clear(tag)
 
-        for (model in models) {
-            add(model, tag)
+            for (model in models) {
+                add(model, tag)
+            }
         }
     }
 
     override fun clear(tag: String) {
-        val localList = models.toList()
-        models.clear()
+        synchronized(models) {
+            val localList = models.toList()
+            models.clear()
 
-        persist()
+            persist()
 
-        for (item in localList) {
-            // no longer listen for changes to this model
-            item.unsubscribe(this)
-            changeSubscription.fire { it.onModelRemoved(item, tag) }
+            for (item in localList) {
+                // no longer listen for changes to this model
+                item.unsubscribe(this)
+                changeSubscription.fire { it.onModelRemoved(item, tag) }
+            }
         }
     }
 
@@ -115,55 +129,63 @@ abstract class ModelStore<TModel>(
         tag: String,
         index: Int? = null,
     ) {
-        if (index != null) {
-            models.add(index, model)
-        } else {
-            models.add(model)
-        }
+        synchronized(models) {
+            if (index != null) {
+                models.add(index, model)
+            } else {
+                models.add(model)
+            }
 
-        // listen for changes to this model
-        model.subscribe(this)
+            // listen for changes to this model
+            model.subscribe(this)
 
-        persist()
+            persist()
 
-        changeSubscription.fire { it.onModelAdded(model, tag) }
+            changeSubscription.fire { it.onModelAdded(model, tag) }
+        }
     }
 
     private fun removeItem(
         model: TModel,
         tag: String,
     ) {
-        models.remove(model)
+        synchronized(models) {
+            models.remove(model)
 
-        // no longer listen for changes to this model
-        model.unsubscribe(this)
+            // no longer listen for changes to this model
+            model.unsubscribe(this)
 
-        persist()
+            persist()
 
-        changeSubscription.fire { it.onModelRemoved(model, tag) }
+            changeSubscription.fire { it.onModelRemoved(model, tag) }
+        }
     }
 
     protected fun load() {
-        if (name != null && _prefs != null) {
-            val str = _prefs.getString(PreferenceStores.ONESIGNAL, PreferenceOneSignalKeys.MODEL_STORE_PREFIX + name, "[]")
-            val jsonArray = JSONArray(str)
-            for (index in 0 until jsonArray.length()) {
-                val newModel = create(jsonArray.getJSONObject(index)) ?: continue
-                models.add(newModel)
-                // listen for changes to this model
-                newModel.subscribe(this)
+        synchronized(models) {
+            if (name != null && _prefs != null) {
+                val str = _prefs.getString(PreferenceStores.ONESIGNAL, PreferenceOneSignalKeys.MODEL_STORE_PREFIX + name, "[]")
+                val jsonArray = JSONArray(str)
+                for (index in 0 until jsonArray.length()) {
+                    val newModel = create(jsonArray.getJSONObject(index)) ?: continue
+                    models.add(newModel)
+                    // listen for changes to this model
+                    newModel.subscribe(this)
+                }
             }
         }
     }
 
     fun persist() {
-        if (name != null && _prefs != null) {
-            val jsonArray = JSONArray()
-            for (model in models) {
-                jsonArray.put(model.toJSON())
+        synchronized(models) {
+            if (name != null && _prefs != null) {
+                val jsonArray = JSONArray()
+                for (model in models) {
+                    jsonArray.put(model.toJSON())
+                }
+
+                _prefs.saveString(PreferenceStores.ONESIGNAL, PreferenceOneSignalKeys.MODEL_STORE_PREFIX + name, jsonArray.toString())
             }
-
-            _prefs.saveString(PreferenceStores.ONESIGNAL, PreferenceOneSignalKeys.MODEL_STORE_PREFIX + name, jsonArray.toString())
         }
     }
 
@@ -172,5 +194,9 @@ abstract class ModelStore<TModel>(
     override fun unsubscribe(handler: IModelStoreChangeHandler<TModel>) = changeSubscription.unsubscribe(handler)
 
     override val hasSubscribers: Boolean
-        get() = changeSubscription.hasSubscribers
+        get() {
+            synchronized(changeSubscription) {
+                return changeSubscription.hasSubscribers
+            }
+        }
 }