fix: prevent ConcurrentModificationException when setting redactedKeys

bugsnag · Oct 7, 2020 · 515a8d4 · 515a8d4
1 parent d5fd1d4
commit 515a8d4
Show file tree

Hide file tree

Showing 5 changed files with 71 additions and 21 deletions.
diff --git a/bugsnag-android-core/src/androidTest/java/com/bugsnag/android/ClientTest.java b/bugsnag-android-core/src/androidTest/java/com/bugsnag/android/ClientTest.java
@@ -3,7 +3,6 @@
 import static com.bugsnag.android.BugsnagTestUtils.generateClient;
 import static com.bugsnag.android.BugsnagTestUtils.generateConfiguration;
 import static com.bugsnag.android.BugsnagTestUtils.getSharedPrefs;
-import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotEquals;
@@ -25,13 +24,15 @@
 
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Queue;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.Executor;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
 
 @SuppressWarnings("unchecked")
 @SmallTest
@@ -344,4 +345,32 @@ public void testDeviceIdEqualsUserId() {
         String deviceId = client.getDeviceDataCollector().generateDevice().getId();
         assertEquals(userId, deviceId);
     }
+
+    /**
+     * Verifies that calling notify() concurrently delivers event payloads and
+     * does not crash the app.
+     */
+    @Test
+    public void testClientMultiNotify() throws InterruptedException {
+        // concurrently call notify()
+        client =  BugsnagTestUtils.generateClient();
+        Executor executor = Executors.newSingleThreadExecutor();
+        int count = 200;
+        final CountDownLatch latch = new CountDownLatch(count);
+
+        for (int k = 0; k < count / 2; k++) {
+            client.notify(new RuntimeException("Whoops"));
+            latch.countDown();
+
+            executor.execute(new Runnable() {
+                @Override
+                public void run() {
+                    client.notify(new RuntimeException("Oh dear"));
+                    latch.countDown();
+                }
+            });
+        }
+        // wait for all events to be delivered
+        assertTrue(latch.await(5, TimeUnit.SECONDS));
+    }
 }
diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/ConfigInternal.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/ConfigInternal.kt
@@ -36,7 +36,7 @@ internal class ConfigInternal(var apiKey: String) : CallbackAware, MetadataAware
 
     var redactedKeys: Set<String> = metadataState.metadata.redactedKeys
         set(value) {
-            metadataState.metadata.setRedactedKeys(value)
+            metadataState.metadata.redactedKeys = value
             field = value
         }
 

diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/Metadata.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/Metadata.kt
@@ -3,7 +3,6 @@
 package com.bugsnag.android
 
 import java.io.IOException
-import java.util.HashSet
 import java.util.concurrent.ConcurrentHashMap
 
 /**
@@ -13,11 +12,17 @@ import java.util.concurrent.ConcurrentHashMap
  * Diagnostic information is presented on your Bugsnag dashboard in tabs.
  */
 internal data class Metadata @JvmOverloads constructor(
-    internal val store: ConcurrentHashMap<String, Any> = ConcurrentHashMap(),
-    val jsonStreamer: ObjectJsonStreamer = ObjectJsonStreamer(),
-    val redactedKeys: Set<String> = jsonStreamer.redactedKeys
+    internal val store: ConcurrentHashMap<String, Any> = ConcurrentHashMap()
 ) : JsonStream.Streamable, MetadataAware {
 
+    val jsonStreamer: ObjectJsonStreamer = ObjectJsonStreamer()
+
+    var redactedKeys: Set<String>
+        get() = jsonStreamer.redactedKeys
+        set(value) {
+            jsonStreamer.redactedKeys = value
+        }
+
     @Throws(IOException::class)
     override fun toStream(writer: JsonStream) {
         jsonStreamer.objectToStream(store, writer, true)
@@ -93,18 +98,12 @@ internal data class Metadata @JvmOverloads constructor(
         return hashMap
     }
 
-    fun setRedactedKeys(redactKeys: Collection<String>) {
-        val data = HashSet(redactKeys)
-        jsonStreamer.redactedKeys.clear()
-        jsonStreamer.redactedKeys.addAll(data)
-    }
-
     companion object {
         fun merge(vararg data: Metadata): Metadata {
             val stores = data.map { it.toMap() }
             val redactKeys = data.flatMap { it.jsonStreamer.redactedKeys }
             val newMeta = Metadata(mergeMaps(stores))
-            newMeta.setRedactedKeys(redactKeys.toSet())
+            newMeta.redactedKeys = redactKeys.toSet()
             return newMeta
         }
 
@@ -145,9 +144,8 @@ internal data class Metadata @JvmOverloads constructor(
         }
     }
 
-    fun copy() = this.copy(
-        store = toMap(),
-        jsonStreamer = jsonStreamer,
-        redactedKeys = redactedKeys
-    )
+    fun copy(): Metadata {
+        return this.copy(store = toMap())
+            .also { it.redactedKeys = redactedKeys }
+    }
 }
diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/ObjectJsonStreamer.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/ObjectJsonStreamer.kt
@@ -10,7 +10,7 @@ internal class ObjectJsonStreamer {
         private const val OBJECT_PLACEHOLDER = "[OBJECT]"
     }
 
-    val redactedKeys = mutableSetOf("password")
+    var redactedKeys = setOf("password")
 
     // Write complex/nested values to a JsonStreamer
     @Throws(IOException::class)

diff --git a/bugsnag-android-core/src/test/java/com/bugsnag/android/MetadataConcurrentModificationTest.kt b/bugsnag-android-core/src/test/java/com/bugsnag/android/MetadataConcurrentModificationTest.kt
@@ -1,6 +1,7 @@
 package com.bugsnag.android
 
 import org.junit.Assert.assertNotNull
+import org.junit.Assert.assertSame
 import org.junit.Test
 import java.util.concurrent.Executors
 
@@ -18,4 +19,26 @@ internal class MetadataConcurrentModificationTest {
             }
         }
     }
+
+    @Test()
+    fun testConcurrentModificationRedactedKeys() {
+        val keys = mutableSetOf("alpha", "omega")
+        val orig = Metadata()
+        val executor = Executors.newSingleThreadExecutor()
+
+        repeat(100) {
+            orig.redactedKeys = keys
+            executor.execute {
+                keys.add("$it")
+            }
+        }
+    }
+
+    @Test()
+    fun testRedactedKeysCopy() {
+        val orig = Metadata()
+        orig.redactedKeys = mutableSetOf("alpha", "omega")
+        val copy = orig.copy()
+        assertSame(orig.redactedKeys, copy.redactedKeys)
+    }
 }