From 0a9db87735ce2788866f1c6c7f6e63dd5c8a71c1 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 | 36 ++++- .../components/lib/crash/CrashReporter.kt | 5 +- .../lib/crash/handler/ExceptionHandler.kt | 3 +- .../components/lib/crash/BreadcrumbTest.kt | 126 ++++++++++++++++-- .../lib/crash/handler/ExceptionHandlerTest.kt | 13 +- .../lib/crash/service/SentryServiceTest.kt | 4 +- 6 files changed, 169 insertions(+), 18 deletions(-) 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..01e3e02315f 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 @@ -7,6 +7,9 @@ package mozilla.components.lib.crash import android.os.Parcelable import kotlinx.android.parcel.Parcelize import java.util.Date +import java.util.PriorityQueue +import kotlin.Comparator +import kotlin.collections.ArrayList /** * Represents a single crash breadcrumb. @@ -42,7 +45,7 @@ data class Breadcrumb( * Date of of the crash breadcrumb. */ val date: Date = Date() -) : Parcelable { +) : Parcelable, Comparable { /** * Crash breadcrumb priority level. */ @@ -97,4 +100,35 @@ 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) + } +} + +internal class BreadCrumbPriorityQueue( + private val maxSize: Int +) : PriorityQueue() { + override fun add(element: Breadcrumb?): Boolean { + val result = super.add(element) + if (this.size > maxSize) { + this.poll() + } + return result + } + + fun toSortedArrayList(): ArrayList { + val breadcrumbsArrayList: ArrayList = arrayListOf() + if (isNotEmpty()) { + breadcrumbsArrayList.addAll(this) + + /* Sort by timestamp before reporting */ + breadcrumbsArrayList.sortWith(Comparator { bc0, bc1 -> bc0.date.compareTo(bc1.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..38af50c17ac 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/BreadcrumbTest.kt b/components/lib/crash/src/test/java/mozilla/components/lib/crash/BreadcrumbTest.kt index 12eec9706b7..136a0c2a7cd 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 @@ -50,12 +50,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 @@ -163,13 +163,121 @@ 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) + } + + @Test + fun `Reporter stores the correct priority of breadcrumbs`() { + val testMessage = "test_Message" + val testData = hashMapOf("1" to "one", "2" to "two") + val testCategory = "testing_category" + 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, Breadcrumb.Level.DEBUG, testType) + ) + } + assertEquals(reporter.crashBreadcrumbs.size, CrashReporter.BREADCRUMB_MAX_NUM) + + repeat(CrashReporter.BREADCRUMB_MAX_NUM) { + reporter.recordCrashBreadcrumb( + Breadcrumb(testMessage, testData, testCategory, Breadcrumb.Level.INFO, testType) + ) + } + + for (i in 0 until CrashReporter.BREADCRUMB_MAX_NUM) { + assertEquals(reporter.crashBreadcrumbs.elementAt(i).level, Breadcrumb.Level.INFO) + } + + repeat(CrashReporter.BREADCRUMB_MAX_NUM) { + reporter.recordCrashBreadcrumb( + Breadcrumb(testMessage, testData, testCategory, Breadcrumb.Level.DEBUG, testType) + ) + } + + for (i in 0 until CrashReporter.BREADCRUMB_MAX_NUM) { + assertEquals(reporter.crashBreadcrumbs.elementAt(i).level, Breadcrumb.Level.INFO) + } + + repeat(CrashReporter.BREADCRUMB_MAX_NUM) { + reporter.recordCrashBreadcrumb( + Breadcrumb(testMessage, testData, testCategory, Breadcrumb.Level.WARNING, testType) + ) + } + + for (i in 0 until CrashReporter.BREADCRUMB_MAX_NUM) { + assertEquals(reporter.crashBreadcrumbs.elementAt(i).level, Breadcrumb.Level.WARNING) + } + } + + @Test + fun `Reporter breadcrumb 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 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, Breadcrumb.Level.DEBUG, testType) + ) + sleep(100) /* make sure time elapsed */ + } + assertEquals(reporter.crashBreadcrumbs.size, CrashReporter.BREADCRUMB_MAX_NUM) + + var result = reporter.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(CrashReporter.BREADCRUMB_MAX_NUM / 2) { + reporter.recordCrashBreadcrumb( + Breadcrumb(testMessage, testData, testCategory, Breadcrumb.Level.INFO, testType) + ) + sleep(100) /* make sure time elapsed */ + } + + result = reporter.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/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())