fix loadSavedOperations index out of bounds issue

Since things can be added to the queue before loadSavedOperations is
called it has to account for duplicate entries. It did do this, however
it was missing the logic to account for not advancing the index on
duplicates so an out of bounds was possible.

We solved the problem by only incrementing the index if it wasn't a
duplicate, however this implementation has a future landmine. If
something ever removes something from the queue that
loadSavedOperations added and it is still executing index problems can
still happen. This scenario never happens now, but a new feature to
OperationRepo or a refactor could introduce the problem. A fast follow
is recommend so this landmine isn't left here.

Author: jkasten2

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

@@ -65,8 +65,11 @@ abstract class ModelStore<TModel>(
         }
     }
 
+    /**
+     * @return list of read-only models, cloned for thread safety
+     */
     override fun list(): Collection<TModel> {
-        return models
+        return synchronized(models) { models.toList() }
     }
 
     override fun get(id: String): TModel? {

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt

@@ -136,20 +136,25 @@ internal class OperationRepo(
         return waiter.waitForWake()
     }
 
-    // WARNING: Never set to true, until budget rules are added, even for internal use!
+    /**
+     * Only used inside this class, adds OperationQueueItem to queue
+     * WARNING: Never set flush=true until budget rules are added, even for internal use!
+     *
+     * @returns true if the OperationQueueItem was added, false if not
+     */
     private fun internalEnqueue(
         queueItem: OperationQueueItem,
         flush: Boolean,
         addToStore: Boolean,
         index: Int? = null,
-    ) {
-        val hasExisting = queue.any { it.operation.id == queueItem.operation.id }
-        if (hasExisting) {
-            Logging.debug("OperationRepo: internalEnqueue - operation.id: ${queueItem.operation.id} already exists in the queue.")
-            return
-        }
-
+    ): Boolean {
         synchronized(queue) {
+            val hasExisting = queue.any { it.operation.id == queueItem.operation.id }
+            if (hasExisting) {
+                Logging.debug("OperationRepo: internalEnqueue - operation.id: ${queueItem.operation.id} already exists in the queue.")
+                return false
+            }
+
             if (index != null) {
                 queue.add(index, queueItem)
             } else {
@@ -161,6 +166,7 @@ internal class OperationRepo(
         }
 
         waiter.wake(LoopWaiterMessage(flush, 0))
+        return true
     }
 
     /**
@@ -405,18 +411,25 @@ internal class OperationRepo(
 
     /**
      * Load saved operations from preference service and add them into the queue
+     * WARNING: Make sure queue.remove is NEVER called while this method is
+     * running, as internalEnqueue will throw IndexOutOfBounds or put things
+     * out of order if what was removed was something added by this method.
+     *   - This never happens now, but is a landmine to be aware of!
      * NOTE: Sometimes the loading might take longer than expected due to I/O reads from disk
      *      Any I/O implies executing time will vary greatly.
      */
     internal fun loadSavedOperations() {
         _operationModelStore.loadOperations()
-        for (operation in _operationModelStore.list().withIndex()) {
-            internalEnqueue(
-                OperationQueueItem(operation.value, bucket = enqueueIntoBucket),
-                flush = false,
-                addToStore = false,
-                operation.index,
-            )
+        var successfulIndex = 0
+        for (operation in _operationModelStore.list()) {
+            val successful =
+                internalEnqueue(
+                    OperationQueueItem(operation, bucket = enqueueIntoBucket),
+                    flush = false,
+                    addToStore = false,
+                    index = successfulIndex,
+                )
+            if (successful) successfulIndex++
         }
         loadedSubscription.fire { it.onOperationRepoLoaded() }
     }

OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt

@@ -38,7 +38,7 @@ private class Mocks {
             val operationStoreList = mutableListOf<Operation>()
             val mockOperationModelStore = mockk<OperationModelStore>()
             every { mockOperationModelStore.loadOperations() } just runs
-            every { mockOperationModelStore.list() } returns operationStoreList
+            every { mockOperationModelStore.list() } answers { operationStoreList.toList() }
             every { mockOperationModelStore.add(any()) } answers { operationStoreList.add(firstArg<Operation>()) }
             every { mockOperationModelStore.remove(any()) } answers {
                 val id = firstArg<String>()