From 11166f377a3dc76c67e15150e616c5bec48e2313 Mon Sep 17 00:00:00 2001 From: Roger Yang Date: Mon, 16 Sep 2019 11:33:18 -0400 Subject: [PATCH] Closes #4299 Add Mechanism to Keep Higher Priority Breadcrumbs if Max Number of Breadcrumb is recorded --- .../components/lib/crash/Breadcrumb.kt | 10 +- .../lib/crash/BreadcrumbPriorityQueue.kt | 29 +++++ .../components/lib/crash/CrashReporter.kt | 5 +- .../lib/crash/handler/ExceptionHandler.kt | 3 +- .../lib/crash/BreadcrumbPriorityQueueTest.kt | 121 ++++++++++++++++++ .../components/lib/crash/BreadcrumbTest.kt | 89 ++----------- .../lib/crash/handler/ExceptionHandlerTest.kt | 13 +- .../lib/crash/service/SentryServiceTest.kt | 4 +- 8 files changed, 189 insertions(+), 85 deletions(-) create mode 100644 components/lib/crash/src/main/java/mozilla/components/lib/crash/BreadcrumbPriorityQueue.kt create mode 100644 components/lib/crash/src/test/java/mozilla/components/lib/crash/BreadcrumbPriorityQueueTest.kt diff --git a/components/lib/crash/src/main/java/mozilla/components/lib/crash/Breadcrumb.kt b/components/lib/crash/src/main/java/mozilla/components/lib/crash/Breadcrumb.kt index efe6af532c0..d7eb00b6676 100644 --- a/components/lib/crash/src/main/java/mozilla/components/lib/crash/Breadcrumb.kt +++ b/components/lib/crash/src/main/java/mozilla/components/lib/crash/Breadcrumb.kt @@ -42,7 +42,7 @@ data class Breadcrumb( * Date of of the crash breadcrumb. */ val date: Date = Date() -) : Parcelable { +) : Parcelable, Comparable { /** * Crash breadcrumb priority level. */ @@ -97,4 +97,12 @@ data class Breadcrumb( */ USER } + + 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) + } } diff --git a/components/lib/crash/src/main/java/mozilla/components/lib/crash/BreadcrumbPriorityQueue.kt b/components/lib/crash/src/main/java/mozilla/components/lib/crash/BreadcrumbPriorityQueue.kt new file mode 100644 index 00000000000..8ce4bd19f11 --- /dev/null +++ b/components/lib/crash/src/main/java/mozilla/components/lib/crash/BreadcrumbPriorityQueue.kt @@ -0,0 +1,29 @@ +package mozilla.components.lib.crash + +import java.util.PriorityQueue +import kotlin.collections.ArrayList + +internal class BreadcrumbPriorityQueue( + private val maxSize: Int +) : PriorityQueue() { + @Synchronized + override fun add(element: Breadcrumb?): Boolean { + val result = super.add(element) + if (this.size > maxSize) { + this.poll() + } + return result + } + + @Synchronized + fun toSortedArrayList(): ArrayList { + val breadcrumbsArrayList: ArrayList = arrayListOf() + if (isNotEmpty()) { + breadcrumbsArrayList.addAll(this) + + /* Sort by timestamp before reporting */ + breadcrumbsArrayList.sortBy { it.date } + } + return breadcrumbsArrayList + } +} diff --git a/components/lib/crash/src/main/java/mozilla/components/lib/crash/CrashReporter.kt b/components/lib/crash/src/main/java/mozilla/components/lib/crash/CrashReporter.kt index db24de0c67d..2e56553408a 100644 --- a/components/lib/crash/src/main/java/mozilla/components/lib/crash/CrashReporter.kt +++ b/components/lib/crash/src/main/java/mozilla/components/lib/crash/CrashReporter.kt @@ -49,7 +49,7 @@ class CrashReporter( private val nonFatalCrashIntent: PendingIntent? = null ) { internal val logger = Logger("mozac/CrashReporter") - internal val crashBreadcrumbs = arrayListOf() + internal val crashBreadcrumbs = BreadcrumbPriorityQueue(BREADCRUMB_MAX_NUM) init { if (services.isEmpty()) { @@ -96,9 +96,6 @@ class CrashReporter( * ``` */ fun recordCrashBreadcrumb(breadcrumb: Breadcrumb) { - if (crashBreadcrumbs.size >= BREADCRUMB_MAX_NUM) { - crashBreadcrumbs.removeAt(0) - } crashBreadcrumbs.add(breadcrumb) } diff --git a/components/lib/crash/src/main/java/mozilla/components/lib/crash/handler/ExceptionHandler.kt b/components/lib/crash/src/main/java/mozilla/components/lib/crash/handler/ExceptionHandler.kt index 3591dd63a2f..b167b5db122 100644 --- a/components/lib/crash/src/main/java/mozilla/components/lib/crash/handler/ExceptionHandler.kt +++ b/components/lib/crash/src/main/java/mozilla/components/lib/crash/handler/ExceptionHandler.kt @@ -26,9 +26,8 @@ class ExceptionHandler( try { crashing = true - crashReporter.onCrash(context, Crash.UncaughtExceptionCrash(throwable, - crashReporter.crashBreadcrumbs)) + crashReporter.crashBreadcrumbs.toSortedArrayList())) defaultExceptionHandler?.uncaughtException(thread, throwable) } finally { diff --git a/components/lib/crash/src/test/java/mozilla/components/lib/crash/BreadcrumbPriorityQueueTest.kt b/components/lib/crash/src/test/java/mozilla/components/lib/crash/BreadcrumbPriorityQueueTest.kt new file mode 100644 index 00000000000..07e6bc7f6fb --- /dev/null +++ b/components/lib/crash/src/test/java/mozilla/components/lib/crash/BreadcrumbPriorityQueueTest.kt @@ -0,0 +1,121 @@ +/* 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 androidx.test.ext.junit.runners.AndroidJUnit4 +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import java.lang.Thread.sleep + +@RunWith(AndroidJUnit4::class) +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 + var 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 + var 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 + } + } +} diff --git a/components/lib/crash/src/test/java/mozilla/components/lib/crash/BreadcrumbTest.kt b/components/lib/crash/src/test/java/mozilla/components/lib/crash/BreadcrumbTest.kt index 12eec9706b7..a0a7c75c578 100644 --- a/components/lib/crash/src/test/java/mozilla/components/lib/crash/BreadcrumbTest.kt +++ b/components/lib/crash/src/test/java/mozilla/components/lib/crash/BreadcrumbTest.kt @@ -5,7 +5,7 @@ package mozilla.components.lib.crash import androidx.test.ext.junit.runners.AndroidJUnit4 -import mozilla.components.lib.crash.service.CrashReporterService +import mozilla.components.support.test.mock import mozilla.components.support.test.robolectric.testContext import org.junit.Assert.assertEquals import org.junit.Assert.assertNotNull @@ -33,16 +33,8 @@ class BreadcrumbTest { val testLevel = Breadcrumb.Level.CRITICAL val testType = Breadcrumb.Type.USER - val service = object : CrashReporterService { - override fun report(crash: Crash.UncaughtExceptionCrash) { - } - - override fun report(crash: Crash.NativeCodeCrash) { - } - } - val reporter = spy(CrashReporter( - services = listOf(service), + services = listOf(mock()), shouldPrompt = CrashReporter.Prompt.NEVER ).install(testContext)) @@ -50,12 +42,12 @@ class BreadcrumbTest { Breadcrumb(testMessage, testData, testCategory, testLevel, testType) ) - assertEquals(reporter.crashBreadcrumbs[0].message, testMessage) - assertEquals(reporter.crashBreadcrumbs[0].data, testData) - assertEquals(reporter.crashBreadcrumbs[0].category, testCategory) - assertEquals(reporter.crashBreadcrumbs[0].level, testLevel) - assertEquals(reporter.crashBreadcrumbs[0].type, testType) - assertNotNull(reporter.crashBreadcrumbs[0].date) + assertEquals(reporter.crashBreadcrumbs.elementAt(0).message, testMessage) + assertEquals(reporter.crashBreadcrumbs.elementAt(0).data, testData) + assertEquals(reporter.crashBreadcrumbs.elementAt(0).category, testCategory) + assertEquals(reporter.crashBreadcrumbs.elementAt(0).level, testLevel) + assertEquals(reporter.crashBreadcrumbs.elementAt(0).type, testType) + assertNotNull(reporter.crashBreadcrumbs.elementAt(0).date) } @Test @@ -66,16 +58,8 @@ class BreadcrumbTest { val testLevel = Breadcrumb.Level.CRITICAL val testType = Breadcrumb.Type.USER - val service = object : CrashReporterService { - override fun report(crash: Crash.UncaughtExceptionCrash) { - } - - override fun report(crash: Crash.NativeCodeCrash) { - } - } - val reporter = spy(CrashReporter( - services = listOf(service), + services = listOf(mock()), shouldPrompt = CrashReporter.Prompt.NEVER ).install(testContext)) @@ -95,45 +79,6 @@ class BreadcrumbTest { assertEquals(reporter.crashBreadcrumbs.size, 3) } - @Test - fun `Reporter 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 - - val service = object : CrashReporterService { - override fun report(crash: Crash.UncaughtExceptionCrash) { - } - - override fun report(crash: Crash.NativeCodeCrash) { - } - } - - val reporter = spy(CrashReporter( - services = listOf(service), - shouldPrompt = CrashReporter.Prompt.NEVER - ).install(testContext)) - - repeat(CrashReporter.BREADCRUMB_MAX_NUM) { - reporter.recordCrashBreadcrumb( - Breadcrumb(testMessage, testData, testCategory, testLevel, testType) - ) - } - assertEquals(reporter.crashBreadcrumbs.size, CrashReporter.BREADCRUMB_MAX_NUM) - - reporter.recordCrashBreadcrumb( - Breadcrumb(testMessage, testData, testCategory, testLevel, testType) - ) - assertEquals(reporter.crashBreadcrumbs.size, CrashReporter.BREADCRUMB_MAX_NUM) - - reporter.recordCrashBreadcrumb( - Breadcrumb(testMessage, testData, testCategory, testLevel, testType) - ) - assertEquals(reporter.crashBreadcrumbs.size, CrashReporter.BREADCRUMB_MAX_NUM) - } - @Test fun `RecordBreadCrumb stores correct date`() { val testMessage = "test_Message" @@ -142,16 +87,8 @@ class BreadcrumbTest { val testLevel = Breadcrumb.Level.CRITICAL val testType = Breadcrumb.Type.USER - val service = object : CrashReporterService { - override fun report(crash: Crash.UncaughtExceptionCrash) { - } - - override fun report(crash: Crash.NativeCodeCrash) { - } - } - val reporter = spy(CrashReporter( - services = listOf(service), + services = listOf(mock()), shouldPrompt = CrashReporter.Prompt.NEVER ).install(testContext)) @@ -163,13 +100,13 @@ class BreadcrumbTest { sleep(100) /* make sure time elapsed */ val afterDate = Date() - assertTrue(reporter.crashBreadcrumbs[0].date.after(beginDate)) - assertTrue(reporter.crashBreadcrumbs[0].date.before(afterDate)) + assertTrue(reporter.crashBreadcrumbs.elementAt(0).date.after(beginDate)) + assertTrue(reporter.crashBreadcrumbs.elementAt(0).date.before(afterDate)) val date = Date() reporter.recordCrashBreadcrumb( Breadcrumb(testMessage, testData, testCategory, testLevel, testType, date) ) - assertTrue(reporter.crashBreadcrumbs[1].date.compareTo(date) == 0) + assertTrue(reporter.crashBreadcrumbs.elementAt(1).date.compareTo(date) == 0) } } diff --git a/components/lib/crash/src/test/java/mozilla/components/lib/crash/handler/ExceptionHandlerTest.kt b/components/lib/crash/src/test/java/mozilla/components/lib/crash/handler/ExceptionHandlerTest.kt index 7c3a144f386..a9d8fe42cf7 100644 --- a/components/lib/crash/src/test/java/mozilla/components/lib/crash/handler/ExceptionHandlerTest.kt +++ b/components/lib/crash/src/test/java/mozilla/components/lib/crash/handler/ExceptionHandlerTest.kt @@ -62,9 +62,20 @@ class ExceptionHandlerTest { fun `ExceptionHandler invokes default exception handler`() { val defaultExceptionHandler: Thread.UncaughtExceptionHandler = mock() + val crashReporter = CrashReporter( + shouldPrompt = CrashReporter.Prompt.NEVER, + services = listOf(object : CrashReporterService { + override fun report(crash: Crash.UncaughtExceptionCrash) { + } + + override fun report(crash: Crash.NativeCodeCrash) { + } + }) + ).install(testContext) + val handler = ExceptionHandler( testContext, - mock(), + crashReporter, defaultExceptionHandler ) diff --git a/components/lib/crash/src/test/java/mozilla/components/lib/crash/service/SentryServiceTest.kt b/components/lib/crash/src/test/java/mozilla/components/lib/crash/service/SentryServiceTest.kt index 7790142afe0..0f9ab6b2cdc 100644 --- a/components/lib/crash/src/test/java/mozilla/components/lib/crash/service/SentryServiceTest.kt +++ b/components/lib/crash/src/test/java/mozilla/components/lib/crash/service/SentryServiceTest.kt @@ -185,12 +185,14 @@ class SentryServiceTest { reporter.recordCrashBreadcrumb( Breadcrumb(testMessage, testData, testCategory, testLevel, testType) ) + var crashBreadCrumbs = arrayListOf() + crashBreadCrumbs.addAll(reporter.crashBreadcrumbs) val nativeCrash = Crash.NativeCodeCrash( "dump.path", true, "extras.path", isFatal = false, - breadcrumbs = reporter.crashBreadcrumbs) + breadcrumbs = crashBreadCrumbs) service.report(nativeCrash) verify(clientContext).recordBreadcrumb(any())