Skip to content

Commit

Permalink
Bug 1801349 - Sentry breadcrumb fixes
Browse files Browse the repository at this point in the history 
Hopefully the issue with the timestamps was simply that we were
recording them, but not copying them to the Sentry
breadcrumb class.

The issue with duplicate items looks like it could be a threading issue
to me and AFAICT when multiple threads report breadcrumbs, we are
currently mutating the array without any synchronization.
  • Loading branch information
@bendk @mergify
bendk authored and mergify[bot] committed Nov 21, 2022
1 parent 4b2fa1c commit 2a0d18a
Show file tree
Hide file tree
Showing 7 changed files with 55 additions and 10 deletions.
1 change: 1 addition & 0 deletions android-components/components/lib/crash-sentry-legacy/build.gradle
View file
Expand Up @@ -41,6 +41,7 @@ dependencies {
testImplementation project(':support-test')
testImplementation Dependencies.androidx_test_core
testImplementation Dependencies.androidx_test_junit
testImplementation Dependencies.testing_coroutines
testImplementation Dependencies.testing_robolectric
testImplementation Dependencies.testing_mockito
}
Expand Down
18 changes: 16 additions & 2 deletions ...ntry-legacy/src/test/java/mozilla/components/lib/crash/sentry/legacy/SentryServiceTest.kt
View file
Expand Up @@ -11,6 +11,8 @@ import io.sentry.context.Context
import io.sentry.dsn.Dsn
import io.sentry.event.Event
import io.sentry.event.EventBuilder
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.test.advanceUntilIdle
import mozilla.components.concept.base.crash.Breadcrumb
import mozilla.components.lib.crash.Crash
import mozilla.components.lib.crash.CrashReporter
Expand All @@ -19,10 +21,13 @@ import mozilla.components.support.test.eq
import mozilla.components.support.test.ext.isLazyInitialized
import mozilla.components.support.test.mock
import mozilla.components.support.test.robolectric.testContext
import mozilla.components.support.test.rule.MainCoroutineRule
import mozilla.components.support.test.rule.runTestOnMain
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertNotNull
import org.junit.Assert.assertTrue
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.mockito.ArgumentCaptor
Expand All @@ -36,9 +41,14 @@ import org.mockito.Mockito.verify
import org.mockito.Mockito.`when`
import java.util.Date

@ExperimentalCoroutinesApi
@RunWith(AndroidJUnit4::class)
class SentryServiceTest {

@get:Rule
val coroutinesTestRule = MainCoroutineRule()
private val scope = coroutinesTestRule.scope

@Test
fun `SentryService disables exception handler and forwards tags`() {
var usedDsn: Dsn? = null
Expand Down Expand Up @@ -351,7 +361,7 @@ class SentryServiceTest {
}

@Test
fun `SentryService records breadcrumb when CrashReporterService report is called`() {
fun `SentryService records breadcrumb when CrashReporterService report is called`() = runTestOnMain {
val client: SentryClient = mock()
val clientContext: Context = mock()
val testMessage = "test_Message"
Expand All @@ -378,6 +388,7 @@ class SentryServiceTest {
context = testContext,
services = listOf(service),
shouldPrompt = CrashReporter.Prompt.NEVER,
scope = scope,
).install(testContext),
)

Expand All @@ -392,6 +403,7 @@ class SentryServiceTest {
testType,
),
)
advanceUntilIdle()
val crashBreadCrumbs = arrayListOf<Breadcrumb>()
crashBreadCrumbs.addAll(reporter.crashBreadcrumbsCopy())
val nativeCrash = Crash.NativeCodeCrash(
Expand All @@ -408,7 +420,7 @@ class SentryServiceTest {
}

@Test
fun `SentryService records breadcrumb caught exception report is called`() {
fun `SentryService records breadcrumb caught exception report is called`() = runTestOnMain {
val client: SentryClient = mock()
val clientContext: Context = mock()
val testMessage = "test_Message"
Expand All @@ -435,6 +447,7 @@ class SentryServiceTest {
testContext,
services = listOf(service),
shouldPrompt = CrashReporter.Prompt.NEVER,
scope = scope,
).install(testContext),
)

Expand All @@ -449,6 +462,7 @@ class SentryServiceTest {
testType,
),
)
advanceUntilIdle()
val throwable = RuntimeException("Test")
val crashBreadCrumbs = arrayListOf<Breadcrumb>()
crashBreadCrumbs.addAll(reporter.crashBreadcrumbsCopy())
Expand Down
2 changes: 1 addition & 1 deletion ...nents/lib/crash-sentry/src/main/java/mozilla/components/lib/crash/sentry/SentryService.kt
View file
Expand Up @@ -192,7 +192,7 @@ class SentryService(
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
internal fun MozillaBreadcrumb.toSentryBreadcrumb(): Breadcrumb {
val sentryLevel = this.level.toSentryBreadcrumbLevel()
val breadcrumb = Breadcrumb().apply {
val breadcrumb = Breadcrumb(this.date).apply {
message = this@toSentryBreadcrumb.message
category = this@toSentryBreadcrumb.category
level = sentryLevel
Expand Down
3 changes: 3 additions & 0 deletions ...s/lib/crash-sentry/src/test/java/mozilla/components/lib/crash/sentry/SentryServiceTest.kt
View file
Expand Up @@ -17,6 +17,7 @@ import org.mockito.Mockito.never
import org.mockito.Mockito.spy
import org.mockito.Mockito.times
import org.mockito.Mockito.verify
import java.util.Date
import mozilla.components.concept.base.crash.Breadcrumb as MozillaBreadcrumb

@RunWith(AndroidJUnit4::class)
Expand Down Expand Up @@ -204,6 +205,7 @@ class SentryServiceTest {
category = "category",
level = MozillaBreadcrumb.Level.INFO,
type = MozillaBreadcrumb.Type.DEFAULT,
date = Date(1640995200L), // 2022-01-01
)
val sentryBreadcrumb = mozillaBreadcrumb.toSentryBreadcrumb()

Expand All @@ -212,6 +214,7 @@ class SentryServiceTest {
assertEquals(mozillaBreadcrumb.category, sentryBreadcrumb.category)
assertEquals(SentryLevel.INFO, sentryBreadcrumb.level)
assertEquals(MozillaBreadcrumb.Type.DEFAULT.value, sentryBreadcrumb.type)
assertEquals(mozillaBreadcrumb.date, sentryBreadcrumb.timestamp)
}

@Test
Expand Down
10 changes: 6 additions & 4 deletions ...mponents/components/lib/crash/src/main/java/mozilla/components/lib/crash/CrashReporter.kt
View file
Expand Up @@ -181,11 +181,13 @@ class CrashReporter(
* ```
*/
override fun recordCrashBreadcrumb(breadcrumb: Breadcrumb) {
if (crashBreadcrumbs.size >= maxBreadCrumbs) {
crashBreadcrumbs.removeAt(0)
}
scope.launch {
if (crashBreadcrumbs.size >= maxBreadCrumbs) {
crashBreadcrumbs.removeAt(0)
}

crashBreadcrumbs.add(breadcrumb)
crashBreadcrumbs.add(breadcrumb)
}
}

internal fun onCrash(context: Context, crash: Crash) {
Expand Down
26 changes: 23 additions & 3 deletions ...ponents/components/lib/crash/src/test/java/mozilla/components/lib/crash/BreadcrumbTest.kt
View file
Expand Up @@ -5,29 +5,39 @@
package mozilla.components.lib.crash

import androidx.test.ext.junit.runners.AndroidJUnit4
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.test.advanceUntilIdle
import mozilla.components.concept.base.crash.Breadcrumb
import mozilla.components.support.test.mock
import mozilla.components.support.test.robolectric.testContext
import mozilla.components.support.test.rule.MainCoroutineRule
import mozilla.components.support.test.rule.runTestOnMain
import org.junit.Assert.assertEquals
import org.junit.Assert.assertNotNull
import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.mockito.Mockito.spy
import java.lang.Thread.sleep
import java.util.Date

@ExperimentalCoroutinesApi
@RunWith(AndroidJUnit4::class)
class BreadcrumbTest {

@get:Rule
val coroutinesTestRule = MainCoroutineRule()
private val scope = coroutinesTestRule.scope

@Before
fun setUp() {
CrashReporter.reset()
}

@Test
fun `RecordBreadCrumb stores breadCrumb in reporter`() {
fun `RecordBreadCrumb stores breadCrumb in reporter`() = runTestOnMain {
val testMessage = "test_Message"
val testData = hashMapOf("1" to "one", "2" to "two")
val testCategory = "testing_category"
Expand All @@ -39,6 +49,7 @@ class BreadcrumbTest {
context = testContext,
services = listOf(mock()),
shouldPrompt = CrashReporter.Prompt.NEVER,
scope = scope,
).install(testContext),
)

Expand All @@ -52,6 +63,8 @@ class BreadcrumbTest {
),
)

advanceUntilIdle()

assertEquals(reporter.crashBreadcrumbs.elementAt(0).message, testMessage)
assertEquals(reporter.crashBreadcrumbs.elementAt(0).data, testData)
assertEquals(reporter.crashBreadcrumbs.elementAt(0).category, testCategory)
Expand All @@ -61,7 +74,7 @@ class BreadcrumbTest {
}

@Test
fun `Reporter stores current number of breadcrumbs`() {
fun `Reporter stores current number of breadcrumbs`() = runTestOnMain {
val testMessage = "test_Message"
val testData = hashMapOf("1" to "one", "2" to "two")
val testCategory = "testing_category"
Expand All @@ -73,6 +86,7 @@ class BreadcrumbTest {
context = testContext,
services = listOf(mock()),
shouldPrompt = CrashReporter.Prompt.NEVER,
scope = scope,
).install(testContext),
)

Expand All @@ -85,6 +99,7 @@ class BreadcrumbTest {
testType,
),
)
advanceUntilIdle()
assertEquals(reporter.crashBreadcrumbs.size, 1)

reporter.recordCrashBreadcrumb(
Expand All @@ -96,6 +111,7 @@ class BreadcrumbTest {
testType,
),
)
advanceUntilIdle()
assertEquals(reporter.crashBreadcrumbs.size, 2)

reporter.recordCrashBreadcrumb(
Expand All @@ -107,11 +123,12 @@ class BreadcrumbTest {
testType,
),
)
advanceUntilIdle()
assertEquals(reporter.crashBreadcrumbs.size, 3)
}

@Test
fun `RecordBreadcumb stores correct date`() {
fun `RecordBreadcumb stores correct date`() = runTestOnMain {
val testMessage = "test_Message"
val testData = hashMapOf("1" to "one", "2" to "two")
val testCategory = "testing_category"
Expand All @@ -123,6 +140,7 @@ class BreadcrumbTest {
context = testContext,
services = listOf(mock()),
shouldPrompt = CrashReporter.Prompt.NEVER,
scope = scope,
).install(testContext),
)

Expand All @@ -137,6 +155,7 @@ class BreadcrumbTest {
testType,
),
)
advanceUntilIdle()
sleep(100) /* make sure time elapsed */
val afterDate = Date()

Expand All @@ -154,6 +173,7 @@ class BreadcrumbTest {
date,
),
)
advanceUntilIdle()
assertEquals(reporter.crashBreadcrumbs.elementAt(1).date.compareTo(date), 0)
}

Expand Down
5 changes: 5 additions & 0 deletions docs/changelog.md
View file
Expand Up @@ -14,6 +14,11 @@ permalink: /changelog/
* **feature-awesomebar**
* `SearchSuggestionProvider` and `SearchActionProvider` now have a new parameter `suggestionsHeader`, to add title to suggestions.

* **lib-crash**
* 🚒 Bug fixed [bug #1801349](https://bugzilla.mozilla.org/show_bug.cgi?id=1801349). Execute `recordCrashBreadcrumb()` inside our coroutine scope to avoid issues with mutating the array from multiple threads at once.
* **lib-crash-sentry**
* 🚒 Bug fixed [bug #1801349](https://bugzilla.mozilla.org/show_bug.cgi?id=1801349). Copy the breadcrumb date to the Sentry breadcrumb.

# 108.0.0
* [Commits](https://github.com/mozilla-mobile/firefox-android/compare/v107.0.0...v108.0.0)
* [Dependencies](https://github.com/mozilla-mobile/firefox-android/blob/v108.0.0/android-components/buildSrc/src/main/java/Dependencies.kt)
Expand Down

0 comments on commit 2a0d18a

Please sign in to comment.