Merge #8243

8243: Closes #8242: Keep latest breadcrumbs r=pocmo a=rocketsroger



Co-authored-by: Roger Yang <royang@mozilla.com>

components/lib/crash/src/main/java/mozilla/components/lib/crash/BreadcrumbPriorityQueue.kt

-/* This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this
- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-
-package mozilla.components.lib.crash
-
-import mozilla.components.support.base.crash.Breadcrumb
-import java.util.PriorityQueue
-
-internal class BreadcrumbPriorityQueue(
-    private val maxSize: Int
-) : PriorityQueue<Breadcrumb>() {
-    @Synchronized
-    override fun add(element: Breadcrumb?): Boolean {
-        val result = super.add(element)
-        if (this.size > maxSize) {
-            this.poll()
-        }
-        return result
-    }
-
-    @Synchronized
-    fun toSortedArrayList(): ArrayList<Breadcrumb> {
-        val breadcrumbsArrayList: ArrayList<Breadcrumb> = arrayListOf()
-        if (isNotEmpty()) {
-            breadcrumbsArrayList.addAll(this)
-
-            /* Sort by timestamp before reporting */
-            breadcrumbsArrayList.sortBy { it.date }
-        }
-        return breadcrumbsArrayList
-    }
-}

components/lib/crash/src/main/java/mozilla/components/lib/crash/CrashReporter.kt

@@ -58,7 +58,7 @@ import mozilla.components.support.base.log.logger.Logger
  *                            happened. This gives the app the opportunity to show an in-app confirmation UI before
  *                            sending a crash report. See component README for details.
  */
-@Suppress("TooManyFunctions")
+@Suppress("TooManyFunctions", "LongParameterList")
 class CrashReporter(
     context: Context,
     private val services: List<CrashReporterService> = emptyList(),
@@ -67,12 +67,15 @@ class CrashReporter(
     var enabled: Boolean = true,
     internal val promptConfiguration: PromptConfiguration = PromptConfiguration(),
     private val nonFatalCrashIntent: PendingIntent? = null,
-    private val scope: CoroutineScope = CoroutineScope(Dispatchers.IO)
+    private val scope: CoroutineScope = CoroutineScope(Dispatchers.IO),
+    private val maxBreadCrumbs: Int = 30
 ) : CrashReporting {
     private val database: CrashDatabase by lazy { CrashDatabase.get(context) }
 
     internal val logger = Logger("mozac/CrashReporter")
-    internal val crashBreadcrumbs = BreadcrumbPriorityQueue(BREADCRUMB_MAX_NUM)
+
+    @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
+    internal val crashBreadcrumbs = ArrayList<Breadcrumb>()
 
     init {
         if (services.isEmpty() and telemetryServices.isEmpty()) {
@@ -155,7 +158,7 @@ class CrashReporter(
         logger.info("Caught Exception report submitted to ${services.size} services")
         return scope.launch {
             services.forEach {
-                it.report(reportThrowable, crashBreadcrumbs.toSortedArrayList())
+                it.report(reportThrowable, crashBreadcrumbs)
             }
         }
     }
@@ -170,6 +173,10 @@ class CrashReporter(
      * ```
      */
     override fun recordCrashBreadcrumb(breadcrumb: Breadcrumb) {
+        if (crashBreadcrumbs.size >= maxBreadCrumbs) {
+            crashBreadcrumbs.removeAt(0)
+        }
+
         crashBreadcrumbs.add(breadcrumb)
     }
 
@@ -288,9 +295,6 @@ class CrashReporter(
     )
 
     companion object {
-        @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
-        internal const val BREADCRUMB_MAX_NUM = 20
-
         @Volatile
         private var instance: CrashReporter? = null
 

components/lib/crash/src/main/java/mozilla/components/lib/crash/handler/ExceptionHandler.kt

@@ -37,7 +37,7 @@ class ExceptionHandler(
                 context,
                 Crash.UncaughtExceptionCrash(
                     throwable = throwable,
-                    breadcrumbs = crashReporter.crashBreadcrumbs.toSortedArrayList()
+                    breadcrumbs = crashReporter.crashBreadcrumbs
                 )
             )
 

components/lib/crash/src/test/java/mozilla/components/lib/crash/BreadcrumbPriorityQueueTest.kt

-/* This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this
- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-
-package mozilla.components.lib.crash
-
-import mozilla.components.support.base.crash.Breadcrumb
-import org.junit.Assert.assertEquals
-import org.junit.Assert.assertTrue
-import org.junit.Test
-import java.lang.Thread.sleep
-
-class BreadcrumbPriorityQueueTest {
-
-    @Test
-    fun `Breadcrumb priority queue stores only max number of breadcrumbs`() {
-        val testMessage = "test_Message"
-        val testData = hashMapOf("1" to "one", "2" to "two")
-        val testCategory = "testing_category"
-        val testLevel = Breadcrumb.Level.CRITICAL
-        val testType = Breadcrumb.Type.USER
-        var crashBreadcrumbs = BreadcrumbPriorityQueue(5)
-        repeat(10) {
-            crashBreadcrumbs.add(Breadcrumb(testMessage, testData, testCategory, testLevel, testType))
-        }
-        assertEquals(crashBreadcrumbs.size, 5)
-
-        crashBreadcrumbs = BreadcrumbPriorityQueue(10)
-        repeat(15) {
-            crashBreadcrumbs.add(Breadcrumb(testMessage, testData, testCategory, testLevel, testType))
-        }
-        assertEquals(crashBreadcrumbs.size, 10)
-    }
-
-    @Test
-    fun `Breadcrumb priority queue stores the correct priority`() {
-        val testMessage = "test_Message"
-        val testData = hashMapOf("1" to "one", "2" to "two")
-        val testCategory = "testing_category"
-        val testType = Breadcrumb.Type.USER
-        val maxNum = 5
-        val crashBreadcrumbs = BreadcrumbPriorityQueue(maxNum)
-
-        repeat(maxNum) {
-            crashBreadcrumbs.add(
-                    Breadcrumb(testMessage, testData, testCategory, Breadcrumb.Level.DEBUG, testType)
-            )
-        }
-        assertEquals(crashBreadcrumbs.size, maxNum)
-
-        repeat(maxNum) {
-            crashBreadcrumbs.add(
-                    Breadcrumb(testMessage, testData, testCategory, Breadcrumb.Level.INFO, testType)
-            )
-        }
-
-        for (i in 0 until maxNum) {
-            assertEquals(crashBreadcrumbs.elementAt(i).level, Breadcrumb.Level.INFO)
-        }
-
-        repeat(maxNum) {
-            crashBreadcrumbs.add(
-                    Breadcrumb(testMessage, testData, testCategory, Breadcrumb.Level.DEBUG, testType)
-            )
-        }
-
-        for (i in 0 until maxNum) {
-            assertEquals(crashBreadcrumbs.elementAt(i).level, Breadcrumb.Level.INFO)
-        }
-
-        repeat(maxNum) {
-            crashBreadcrumbs.add(
-                    Breadcrumb(testMessage, testData, testCategory, Breadcrumb.Level.WARNING, testType)
-            )
-        }
-
-        for (i in 0 until maxNum) {
-            assertEquals(crashBreadcrumbs.elementAt(i).level, Breadcrumb.Level.WARNING)
-        }
-    }
-
-    @Test
-    fun `Breadcrumb priority queue output list result is sorted by time`() {
-        val testMessage = "test_Message"
-        val testData = hashMapOf("1" to "one", "2" to "two")
-        val testCategory = "testing_category"
-        val testType = Breadcrumb.Type.USER
-        val maxNum = 10
-        val crashBreadcrumbs = BreadcrumbPriorityQueue(maxNum)
-
-        repeat(maxNum) {
-            crashBreadcrumbs.add(
-                    Breadcrumb(testMessage, testData, testCategory, Breadcrumb.Level.DEBUG, testType)
-            )
-            sleep(100) /* make sure time elapsed */
-        }
-
-        var result = crashBreadcrumbs.toSortedArrayList()
-        var time = result[0].date
-        for (i in 1 until result.size) {
-            assertTrue(time.before(result[i].date))
-            time = result[i].date
-        }
-
-        repeat(maxNum / 2) {
-            crashBreadcrumbs.add(
-                    Breadcrumb(testMessage, testData, testCategory, Breadcrumb.Level.INFO, testType)
-            )
-            sleep(100) /* make sure time elapsed */
-        }
-
-        result = crashBreadcrumbs.toSortedArrayList()
-        time = result[0].date
-        for (i in 1 until result.size) {
-            assertTrue(time.before(result[i].date))
-            time = result[i].date
-        }
-    }
-}

components/lib/crash/src/test/java/mozilla/components/lib/crash/CrashReporterTest.kt

@@ -35,6 +35,7 @@ import org.mockito.Mockito.times
 import org.mockito.Mockito.verify
 import org.robolectric.Robolectric
 import org.robolectric.Shadows.shadowOf
+import java.lang.Thread.sleep
 import java.lang.reflect.Modifier
 
 @ExperimentalCoroutinesApi
@@ -617,6 +618,126 @@ class CrashReporterTest {
         val instanceField = CrashReporter::class.java.getDeclaredField("instance")
         assertTrue(Modifier.isVolatile(instanceField.modifiers))
     }
+
+    @Test
+    fun `Breadcrumbs stores only max number of breadcrumbs`() {
+        val testMessage = "test_Message"
+        val testData = hashMapOf("1" to "one", "2" to "two")
+        val testCategory = "testing_category"
+        val testLevel = Breadcrumb.Level.CRITICAL
+        val testType = Breadcrumb.Type.USER
+
+        var crashReporter = CrashReporter(
+            context = testContext,
+            services = listOf(mock()),
+            maxBreadCrumbs = 5
+        )
+
+        repeat(10) {
+            crashReporter.recordCrashBreadcrumb(Breadcrumb(testMessage, testData, testCategory, testLevel, testType))
+        }
+        assertEquals(crashReporter.crashBreadcrumbs.size, 5)
+
+        crashReporter = CrashReporter(
+            context = testContext,
+            services = listOf(mock()),
+            maxBreadCrumbs = 5
+        )
+        repeat(15) {
+            crashReporter.recordCrashBreadcrumb(Breadcrumb(testMessage, testData, testCategory, testLevel, testType))
+        }
+        assertEquals(crashReporter.crashBreadcrumbs.size, 5)
+    }
+
+    @Test
+    fun `Breadcrumb priority queue stores the latest breadcrumbs`() {
+        val testMessage = "test_Message"
+        val testData = hashMapOf("1" to "one", "2" to "two")
+        val testCategory = "testing_category"
+        val testType = Breadcrumb.Type.USER
+        val maxNum = 10
+
+        var crashReporter = CrashReporter(
+            context = testContext,
+            services = listOf(mock()),
+            maxBreadCrumbs = maxNum
+        )
+
+        repeat(maxNum) {
+            crashReporter.recordCrashBreadcrumb(
+                Breadcrumb(testMessage, testData, testCategory, Breadcrumb.Level.CRITICAL, testType)
+            )
+            sleep(10) /* make sure time elapsed */
+        }
+
+        for (i in 0 until maxNum) {
+            assertEquals(crashReporter.crashBreadcrumbs.elementAt(i).level, Breadcrumb.Level.CRITICAL)
+        }
+
+        var time = crashReporter.crashBreadcrumbs[0].date
+        for (i in 1 until crashReporter.crashBreadcrumbs.size) {
+            assertTrue(time.before(crashReporter.crashBreadcrumbs[i].date))
+            time = crashReporter.crashBreadcrumbs[i].date
+        }
+
+        repeat(maxNum) {
+            crashReporter.recordCrashBreadcrumb(
+                Breadcrumb(testMessage, testData, testCategory, Breadcrumb.Level.DEBUG, testType)
+            )
+            sleep(10) /* make sure time elapsed */
+        }
+
+        for (i in 0 until maxNum) {
+            assertEquals(crashReporter.crashBreadcrumbs.elementAt(i).level, Breadcrumb.Level.DEBUG)
+        }
+
+        time = crashReporter.crashBreadcrumbs[0].date
+        for (i in 1 until crashReporter.crashBreadcrumbs.size) {
+            assertTrue(time.before(crashReporter.crashBreadcrumbs[i].date))
+            time = crashReporter.crashBreadcrumbs[i].date
+        }
+    }
+
+    @Test
+    fun `Breadcrumb priority queue output list result is sorted by time`() {
+        val testMessage = "test_Message"
+        val testData = hashMapOf("1" to "one", "2" to "two")
+        val testCategory = "testing_category"
+        val testType = Breadcrumb.Type.USER
+        val maxNum = 10
+
+        var crashReporter = CrashReporter(
+            context = testContext,
+            services = listOf(mock()),
+            maxBreadCrumbs = 5
+        )
+
+        repeat(maxNum) {
+            crashReporter.recordCrashBreadcrumb(
+                Breadcrumb(testMessage, testData, testCategory, Breadcrumb.Level.DEBUG, testType)
+            )
+            sleep(10) /* make sure time elapsed */
+        }
+
+        var time = crashReporter.crashBreadcrumbs[0].date
+        for (i in 1 until crashReporter.crashBreadcrumbs.size) {
+            assertTrue(time.before(crashReporter.crashBreadcrumbs[i].date))
+            time = crashReporter.crashBreadcrumbs[i].date
+        }
+
+        repeat(maxNum / 2) {
+            crashReporter.recordCrashBreadcrumb(
+                Breadcrumb(testMessage, testData, testCategory, Breadcrumb.Level.INFO, testType)
+            )
+            sleep(10) /* make sure time elapsed */
+        }
+
+        time = crashReporter.crashBreadcrumbs[0].date
+        for (i in 1 until crashReporter.crashBreadcrumbs.size) {
+            assertTrue(time.before(crashReporter.crashBreadcrumbs[i].date))
+            time = crashReporter.crashBreadcrumbs[i].date
+        }
+    }
 }
 
 private fun createUncaughtExceptionCrash(): Crash.UncaughtExceptionCrash {

components/support/base/src/main/java/mozilla/components/support/base/crash/Breadcrumb.kt

@@ -103,11 +103,7 @@ data class Breadcrumb(
     }
 
     override fun compareTo(other: Breadcrumb): Int {
-        if (this.level.ordinal == other.level.ordinal) {
-            return this.date.compareTo(other.date)
-        }
-
-        return this.level.ordinal.compareTo(other.level.ordinal)
+        return this.date.compareTo(other.date)
     }
 
     /**