Merge pull request #2081 from OneSignal/fix/loadSavedOperations-index-out-of-bounds

[Fix] loadSavedOperations IndexOutOfBoundsException (5.1.11 only issue)

Author: jinliu9508

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>()
@@ -630,6 +630,26 @@ class OperationRepoTests : FunSpec({
         mocks.operationRepo.queue.size shouldBe 1
         mocks.operationRepo.queue.first().operation shouldBe op
     }
+
+    // Real world scenario is this can happen if a few operations are added when the device is
+    // offline then the app is restarted.
+    test("ensure loadSavedOperations doesn't index out of bounds on queue when duplicates exist") {
+        // Given
+        val mocks = Mocks()
+        val op1 = mockOperation()
+        val op2 = mockOperation()
+
+        repeat(2) { mocks.operationModelStore.add(op1) }
+        mocks.operationModelStore.add(op2)
+
+        // When
+        mocks.operationRepo.loadSavedOperations()
+
+        // Then
+        mocks.operationRepo.queue.size shouldBe 2
+        mocks.operationRepo.queue[0].operation shouldBe op1
+        mocks.operationRepo.queue[1].operation shouldBe op2
+    }
 }) {
     companion object {
         private fun mockOperation(