Merge pull request #1876 from OneSignal/fix/concurrent_modification_exception

jennantilla · web-flow · commit d8abaeefc47d · 2023-11-03T14:49:48.000-07:00
Fix: Add synchronized blocks to prevent ConcurrentModificationException
diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/events/EventProducer.kt b/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)
+            }
         }
     }
 
diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/Model.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/Model.kt
@@ -51,6 +51,7 @@ open class Model(
      * specified, must also specify [_parentModel]
      */
     private val _parentProperty: String? = null,
+    private val initializationLock: Any = Any(),
 ) : IEventNotifier<IModelChangedHandler> {
     /**
      * A unique identifier for this model.
@@ -123,19 +124,25 @@ open class Model(
         id: String?,
         model: Model,
     ) {
-        data.clear()
+        val newData = Collections.synchronizedMap(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(initializationLock) {
+            data.clear()
+            data.putAll(newData)
         }
     }
 
@@ -429,19 +436,21 @@ open class Model(
         tag: String = ModelChangeTags.NORMAL,
         forceChange: Boolean = false,
     ) {
-        val oldValue = data[name]
+        synchronized(data) {
+            val oldValue = data[name]
 
-        if (oldValue == value && !forceChange) {
-            return
-        }
+            if (oldValue == value && !forceChange) {
+                return
+            }
 
-        if (value != null) {
-            data[name] = value
-        } else if (data.containsKey(name)) {
-            data.remove(name)
-        }
+            if (value != null) {
+                data[name] = value
+            } else if (data.containsKey(name)) {
+                data.remove(name)
+            }
 
-        notifyChanged(name, name, tag, oldValue, value)
+            notifyChanged(name, name, tag, oldValue, value)
+        }
     }
 
     /**
@@ -627,12 +636,14 @@ open class Model(
         name: String,
         create: (() -> Any?)? = null,
     ): Any? {
-        return if (data.containsKey(name) || create == null) {
-            data[name]
-        } else {
-            val defaultValue = create()
-            data[name] = defaultValue as Any?
-            defaultValue
+        synchronized(data) {
+            return if (data.containsKey(name) || create == null) {
+                data[name]
+            } else {
+                val defaultValue = create()
+                data[name] = defaultValue as Any?
+                defaultValue
+            }
         }
     }
 
@@ -660,29 +671,31 @@ 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(initializationLock) {
+            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)
diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/ModelStore.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/ModelStore.kt
@@ -40,25 +40,29 @@ 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> {
@@ -73,40 +77,48 @@ abstract class ModelStore<TModel>(
         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 +127,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())
         }
     }
 
diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/SingletonModelStore.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/SingletonModelStore.kt
