ConcurrentModificationException from JSONUtil Fix

* Fixes ConcurrentModificationException regression bug reintroduced in 3.8.0
  - Corrected test to mimic more realistic case to prevent another regression
* Resolves OneSignal#465

Author: jkasten2

OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignalPrefs.java

@@ -51,7 +51,7 @@ class OneSignalPrefs {
 
     static ConcurrentHashMap<String,SharedPreferences> preferencesMap = new ConcurrentHashMap<>();
     //use a thread pool executor to execute disk writes
-    private static final ScheduledThreadPoolExecutor prefsExecutor = new ScheduledThreadPoolExecutor(10);
+    private static final ScheduledThreadPoolExecutor prefsExecutor = new ScheduledThreadPoolExecutor(1);
     static {
         prefsExecutor.setThreadFactory(new ThreadFactory() {
             @Override

OneSignalSDK/onesignal/src/main/java/com/onesignal/UserStateSynchronizer.java

@@ -41,7 +41,7 @@ class NetworkHandlerThread extends HandlerThread {
 
         Handler mHandler = null;
 
-        static final int MAX_RETRIES = 3;
+        static final int MAX_RETRIES = 3, NETWORK_CALL_DELAY_TO_BUFFER_MS = 5_000;
         int currentRetry;
 
         NetworkHandlerThread(int type) {
@@ -52,15 +52,11 @@ class NetworkHandlerThread extends HandlerThread {
         }
 
         void runNewJobDelayed() {
-            currentRetry = 0;
-            mHandler.removeCallbacksAndMessages(null);
-            mHandler.postDelayed(getNewRunnable(), 5_000);
-        }
-
-        void runNewJobImmediate() {
-            currentRetry = 0;
-            mHandler.removeCallbacksAndMessages(null);
-            mHandler.post(getNewRunnable());
+            synchronized (mHandler) {
+                currentRetry = 0;
+                mHandler.removeCallbacksAndMessages(null);
+                mHandler.postDelayed(getNewRunnable(), NETWORK_CALL_DELAY_TO_BUFFER_MS);
+            }
         }
 
         private Runnable getNewRunnable() {
@@ -85,15 +81,17 @@ void stopScheduledRunnable() {
         // Retries if not passed limit.
         // Returns true if there retrying or there is another future sync scheduled already
         boolean doRetry() {
-            boolean doRetry = currentRetry < MAX_RETRIES;
-            boolean futureSync = mHandler.hasMessages(0);
+            synchronized (mHandler) {
+                boolean doRetry = currentRetry < MAX_RETRIES;
+                boolean futureSync = mHandler.hasMessages(0);
 
-            if (doRetry && !futureSync) {
-                currentRetry++;
-                mHandler.postDelayed(getNewRunnable(), currentRetry * 15_000);
-            }
+                if (doRetry && !futureSync) {
+                    currentRetry++;
+                    mHandler.postDelayed(getNewRunnable(), currentRetry * 15_000);
+                }
 
-            return mHandler.hasMessages(0);
+                return mHandler.hasMessages(0);
+            }
         }
     }
 
@@ -169,11 +167,11 @@ private void internalSyncUserState(boolean fromSyncService) {
         }
 
         final boolean isSessionCall = isSessionCall();
-
-        final JSONObject jsonBody = currentUserState.generateJsonDiff(toSyncUserState, isSessionCall);
-        final JSONObject dependDiff = generateJsonDiff(currentUserState.dependValues, toSyncUserState.dependValues, null, null);
-
+        JSONObject jsonBody, dependDiff;
         synchronized (syncLock) {
+            jsonBody = currentUserState.generateJsonDiff(toSyncUserState, isSessionCall);
+            dependDiff = generateJsonDiff(currentUserState.dependValues, toSyncUserState.dependValues, null, null);
+
             if (jsonBody == null) {
                 currentUserState.persistStateAfterSync(dependDiff, null);
                 return;

OneSignalSDK/unittest/src/test/java/com/onesignal/OneSignalPackagePrivateHelper.java

@@ -8,9 +8,11 @@
 
 import org.json.JSONArray;
 import org.json.JSONObject;
+import org.robolectric.shadows.ShadowMessageQueue;
 import org.robolectric.util.Scheduler;
 
 import java.lang.reflect.Field;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
@@ -19,10 +21,10 @@
 public class OneSignalPackagePrivateHelper {
 
    private static abstract class RunnableArg<T> {
-      abstract void run(T object);
+      abstract void run(T object) throws Exception;
    }
 
-   static private void processNetworkHandles(RunnableArg runnable) {
+   static private void processNetworkHandles(RunnableArg runnable) throws Exception {
       Set<Map.Entry<Integer, UserStateSynchronizer.NetworkHandlerThread>> entrySet;
 
       entrySet = OneSignalStateSynchronizer.getPushStateSynchronizer().networkHandlerThreads.entrySet();
@@ -35,15 +37,17 @@ static private void processNetworkHandles(RunnableArg runnable) {
    }
 
    private static boolean startedRunnable;
-   public static boolean runAllNetworkRunnables() {
+   public static boolean runAllNetworkRunnables() throws Exception {
       startedRunnable = false;
 
       RunnableArg runnable = new RunnableArg<UserStateSynchronizer.NetworkHandlerThread>() {
          @Override
-         void run(UserStateSynchronizer.NetworkHandlerThread handlerThread) {
-            Scheduler scheduler = shadowOf(handlerThread.getLooper()).getScheduler();
-            while (scheduler.runOneTask())
-               startedRunnable = true;
+         void run(UserStateSynchronizer.NetworkHandlerThread handlerThread) throws Exception {
+            synchronized (handlerThread.mHandler) {
+               Scheduler scheduler = shadowOf(handlerThread.getLooper()).getScheduler();
+               while (scheduler.runOneTask())
+                  startedRunnable = true;
+            }
          }
       };
 
@@ -92,7 +96,7 @@ public void run() {
       return true;
    }
 
-   public static void resetRunnables() {
+   public static void resetRunnables() throws Exception {
       RunnableArg runnable = new RunnableArg<UserStateSynchronizer.NetworkHandlerThread>() {
          @Override
          void run(UserStateSynchronizer.NetworkHandlerThread handlerThread) {

OneSignalSDK/unittest/src/test/java/com/test/onesignal/MainOneSignalClassRunner.java

@@ -1513,22 +1513,28 @@ public void testOneSignalEmptyPendingTaskQueue() throws Exception {
 
    private static boolean failedCurModTest;
    @Test
+   @Config(sdk = 26)
    public void testSendTagsConcurrentModificationException() throws Exception {
       OneSignalInit();
       threadAndTaskWait();
 
-      for(int a = 0; a < 10; a++) {
-         List<Thread> threadList = new ArrayList<>(30);
-         for (int i = 0; i < 30; i++) {
-            Thread lastThread = newSendTagTestThread(Thread.currentThread(), i);
+      final int TOTAL_RUNS = 75, CONCURRENT_THREADS = 15;
+      for(int a = 0; a < TOTAL_RUNS; a++) {
+         List<Thread> threadList = new ArrayList<>(CONCURRENT_THREADS);
+         for (int i = 0; i < CONCURRENT_THREADS; i++) {
+            Thread lastThread = newSendTagTestThread(Thread.currentThread(), a * i);
             lastThread.start();
             threadList.add(lastThread);
             assertFalse(failedCurModTest);
          }
 
+         OneSignalPackagePrivateHelper.runAllNetworkRunnables();
+
          for(Thread thread : threadList)
             thread.join();
+
          assertFalse(failedCurModTest);
+         System.out.println("Pass " + a + " out of " + TOTAL_RUNS);
       }
    }
 
@@ -1541,12 +1547,12 @@ public void run() {
                   if (failedCurModTest)
                      break;
                   OneSignal.sendTags("{\"key" + id + "\": " + i + "}");
-//                  OneSignalPackagePrivateHelper.OneSignalStateSynchronizer_syncUserState(false);
                }
             } catch (Throwable t) {
                // Ignore the flaky Robolectric null error.
                if (t.getStackTrace()[0].getClassName().equals("org.robolectric.shadows.ShadowMessageQueue"))
                   return;
+               t.printStackTrace();
                failedCurModTest = true;
                mainThread.interrupt();
                throw t;