Commit bc41897a authored by Roger Yang's avatar Roger Yang
Browse files

Closes #2887: Allow reporting exceptions without crashing

parent 8935953c
......@@ -17,6 +17,7 @@ crash_metrics:
types of crashes handled by lib-crash.
labels:
- uncaught_exception
- caught_exception
- native_code_crash
bugs:
- https://bugzilla.mozilla.org/1553935
......
......@@ -63,9 +63,9 @@ sealed class Crash {
* may be able to recover.
*/
data class NativeCodeCrash(
val minidumpPath: String,
val minidumpPath: String?,
val minidumpSuccess: Boolean,
val extrasPath: String,
val extrasPath: String?,
val isFatal: Boolean,
val breadcrumbs: ArrayList<Breadcrumb>
) : Crash() {
......@@ -79,9 +79,9 @@ sealed class Crash {
companion object {
internal fun fromBundle(bundle: Bundle) = NativeCodeCrash(
bundle.getString(INTENT_MINIDUMP_PATH, ""),
bundle.getString(INTENT_MINIDUMP_PATH, null),
bundle.getBoolean(INTENT_MINIDUMP_SUCCESS, false),
bundle.getString(INTENT_EXTRAS_PATH, ""),
bundle.getString(INTENT_EXTRAS_PATH, null),
bundle.getBoolean(INTENT_FATAL, false),
bundle.getParcelableArrayList(INTENT_BREADCRUMBS) ?: arrayListOf()
)
......
......@@ -9,6 +9,10 @@ import android.content.Context
import android.content.Intent
import androidx.annotation.StyleRes
import androidx.annotation.VisibleForTesting
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.Job
import kotlinx.coroutines.launch
import mozilla.components.lib.crash.handler.ExceptionHandler
import mozilla.components.lib.crash.notification.CrashNotification
import mozilla.components.lib.crash.prompt.CrashPrompt
......@@ -86,6 +90,19 @@ class CrashReporter(
logger.info("Crash report submitted to ${services.size} services")
}
/**
* Submit a caught exception report to all registered services.
*/
fun submitCaughtException(throwable: Throwable): Job {
logger.info("Caught Exception report submitted to ${services.size} services")
return GlobalScope.launch(Dispatchers.IO) {
services.forEach {
it.report(throwable)
}
}
}
/**
* Add a crash breadcrumb to all registered services with breadcrumb support.
*
......
......@@ -6,6 +6,8 @@ package mozilla.components.lib.crash.service
import mozilla.components.lib.crash.Crash
internal const val INFO_PREFIX = "[INFO]"
/**
* Interface to be implemented by external services that accept crash reports.
*/
......@@ -19,4 +21,9 @@ interface CrashReporterService {
* Submits a crash report for this [Crash.NativeCodeCrash].
*/
fun report(crash: Crash.NativeCodeCrash)
/**
* Submits a caught exception report for this [Throwable].
*/
fun report(throwable: Throwable)
}
......@@ -14,8 +14,8 @@ import java.io.File
/**
* A [CrashReporterService] implementation for recording metrics with Glean. The purpose of this
* crash reporter is to collect crash count metrics by capturing [Crash.UncaughtExceptionCrash] and
* [Crash.NativeCodeCrash] events and record to the respective
* crash reporter is to collect crash count metrics by capturing [Crash.UncaughtExceptionCrash],
* [Throwable] and [Crash.NativeCodeCrash] events and record to the respective
* [mozilla.components.service.glean.private.CounterMetricType].
*/
class GleanCrashReporterService(
......@@ -34,6 +34,7 @@ class GleanCrashReporterService(
// These keys correspond to the labels found for crashCount metric in metrics.yaml as well
// as the persisted crashes in the crash count file (see above comment)
const val UNCAUGHT_EXCEPTION_KEY = "uncaught_exception"
const val CAUGHT_EXCEPTION_KEY = "caught_exception"
const val NATIVE_CODE_CRASH_KEY = "native_code_crash"
}
......@@ -89,8 +90,8 @@ class GleanCrashReporterService(
/**
* Parses the crashes collected in the persisted crash file. The format of this file is simple,
* each line may contain either [UNCAUGHT_EXCEPTION_KEY], or [NATIVE_CODE_CRASH_KEY] followed by
* a newline character.
* each line may contain [UNCAUGHT_EXCEPTION_KEY], [CAUGHT_EXCEPTION_KEY], or [NATIVE_CODE_CRASH_KEY]
* followed by a newline character.
*
* Example:
*
......@@ -99,6 +100,7 @@ class GleanCrashReporterService(
* uncaught_exception\n
* native_code_crash\n
* uncaught_exception\n
* caught_exception\n
* <--End of file-->
*
* It is unlikely that there will be more than one crash in a file, but not impossible. This
......@@ -109,6 +111,7 @@ class GleanCrashReporterService(
internal fun parseCrashFile() {
val lines = file.readLines()
var uncaughtExceptionCount = 0
var caughtExceptionCount = 0
var nativeCodeCrashCount = 0
// It's possible that there was more than one crash recorded in the file so process each
......@@ -116,12 +119,14 @@ class GleanCrashReporterService(
lines.forEach { line ->
when (line) {
UNCAUGHT_EXCEPTION_KEY -> ++uncaughtExceptionCount
CAUGHT_EXCEPTION_KEY -> ++caughtExceptionCount
NATIVE_CODE_CRASH_KEY -> ++nativeCodeCrashCount
}
}
// Now, record the crash counts into Glean
CrashMetrics.crashCount[UNCAUGHT_EXCEPTION_KEY].add(uncaughtExceptionCount)
CrashMetrics.crashCount[CAUGHT_EXCEPTION_KEY].add(caughtExceptionCount)
CrashMetrics.crashCount[NATIVE_CODE_CRASH_KEY].add(nativeCodeCrashCount)
}
......@@ -133,8 +138,8 @@ class GleanCrashReporterService(
* [report] functions are called synchronously, and from lib-crash's own process, it is unlikely
* that this would be called from more than one place at the same time.
*
* @param crash Pass in the correct crash label to write to the file,
* either [UNCAUGHT_EXCEPTION_KEY] or [NATIVE_CODE_CRASH_KEY]
* @param crash Pass in the correct crash label to write to the file
* [UNCAUGHT_EXCEPTION_KEY], [CAUGHT_EXCEPTION_KEY] or [NATIVE_CODE_CRASH_KEY]
*/
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
internal fun reportCrash(crash: String) {
......@@ -159,4 +164,8 @@ class GleanCrashReporterService(
override fun report(crash: Crash.NativeCodeCrash) {
reportCrash(NATIVE_CODE_CRASH_KEY)
}
override fun report(throwable: Throwable) {
reportCrash(CAUGHT_EXCEPTION_KEY)
}
}
......@@ -28,8 +28,9 @@ import java.util.concurrent.TimeUnit
import java.util.zip.GZIPOutputStream
import kotlin.random.Random
private val defaultServerUrl = "https://crash-reports.mozilla.com/submit?" +
private const val DEFAULT_SERVER_URL = "https://crash-reports.mozilla.com/submit?" +
"id=${BuildConfig.MOZ_APP_ID}&version=${BuildConfig.MOZILLA_VERSION}&${BuildConfig.MOZ_APP_BUILDID}"
/**
* A [CrashReporterService] implementation uploading crash reports to crash-stats.mozilla.com.
*
......@@ -43,21 +44,30 @@ private val defaultServerUrl = "https://crash-reports.mozilla.com/submit?" +
class MozillaSocorroService(
private val applicationContext: Context,
private val appName: String,
private val serverUrl: String = defaultServerUrl
private val serverUrl: String = DEFAULT_SERVER_URL
) : CrashReporterService {
private val logger = Logger("mozac/MozillaSocorroCrashHelperService")
private val startTime = System.currentTimeMillis()
override fun report(crash: Crash.UncaughtExceptionCrash) {
sendReport(crash.throwable, null, null)
sendReport(crash.throwable, null, null, false)
}
override fun report(crash: Crash.NativeCodeCrash) {
sendReport(null, crash.minidumpPath, crash.extrasPath)
sendReport(null, crash.minidumpPath, crash.extrasPath, false)
}
override fun report(throwable: Throwable) {
sendReport(throwable, null, null, true)
}
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
internal fun sendReport(throwable: Throwable?, miniDumpFilePath: String?, extrasFilePath: String?) {
internal fun sendReport(
throwable: Throwable?,
miniDumpFilePath: String?,
extrasFilePath: String?,
isCaughtException: Boolean
) {
val serverUrl = URL(serverUrl)
val boundary = generateBoundary()
var conn: HttpURLConnection? = null
......@@ -69,7 +79,8 @@ class MozillaSocorroService(
conn.setRequestProperty("Content-Type", "multipart/form-data; boundary=$boundary")
conn.setRequestProperty("Content-Encoding", "gzip")
sendCrashData(conn.outputStream, boundary, throwable, miniDumpFilePath, extrasFilePath)
sendCrashData(conn.outputStream, boundary, throwable, miniDumpFilePath, extrasFilePath,
isCaughtException)
BufferedReader(InputStreamReader(conn.inputStream)).use {
val response = StringBuffer()
......@@ -88,12 +99,14 @@ class MozillaSocorroService(
}
}
@Suppress("LongParameterList")
private fun sendCrashData(
os: OutputStream,
boundary: String,
throwable: Throwable?,
miniDumpFilePath: String?,
extrasFilePath: String?
extrasFilePath: String?,
isCaughtException: Boolean
) {
val nameSet = mutableSetOf<String>()
val gzipOs = GZIPOutputStream(os)
......@@ -109,7 +122,8 @@ class MozillaSocorroService(
}
throwable?.let {
sendPart(gzipOs, boundary, "JavaStackTrace", getExceptionStackTrace(it), nameSet)
sendPart(gzipOs, boundary, "JavaStackTrace", getExceptionStackTrace(it,
isCaughtException), nameSet)
}
miniDumpFilePath?.let {
......@@ -263,12 +277,15 @@ class MozillaSocorroService(
return map
}
private fun getExceptionStackTrace(throwable: Throwable): String {
private fun getExceptionStackTrace(throwable: Throwable, isCaughtException: Boolean): String {
val stringWriter = StringWriter()
val printWriter = PrintWriter(stringWriter)
throwable.printStackTrace(printWriter)
printWriter.flush()
return stringWriter.toString()
return when (isCaughtException) {
true -> "$INFO_PREFIX $stringWriter"
false -> stringWriter.toString()
}
}
}
......@@ -12,6 +12,9 @@ import io.sentry.SentryClientFactory
import io.sentry.android.AndroidSentryClientFactory
import io.sentry.event.Breadcrumb
import io.sentry.event.BreadcrumbBuilder
import io.sentry.event.Event
import io.sentry.event.EventBuilder
import io.sentry.event.interfaces.ExceptionInterface
import mozilla.components.Build
import mozilla.components.lib.crash.Crash
......@@ -56,7 +59,11 @@ class SentryService(
crash.breadcrumbs.forEach {
client.context.recordBreadcrumb(it.toSentryBreadcrumb())
}
client.sendException(crash.throwable)
val eventBuilder = EventBuilder().withMessage(createMessage(crash))
.withLevel(Event.Level.FATAL)
.withSentryInterface(ExceptionInterface(crash.throwable))
client.sendEvent(eventBuilder)
}
override fun report(crash: Crash.NativeCodeCrash) {
......@@ -68,6 +75,13 @@ class SentryService(
}
}
override fun report(throwable: Throwable) {
val eventBuilder = EventBuilder().withMessage(createMessage(throwable))
.withLevel(Event.Level.ERROR)
.withSentryInterface(ExceptionInterface(throwable))
client.sendEvent(eventBuilder)
}
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
internal fun mozilla.components.lib.crash.Breadcrumb.toSentryBreadcrumb(): Breadcrumb {
return BreadcrumbBuilder()
......@@ -104,3 +118,11 @@ private fun createMessage(crash: Crash.NativeCodeCrash): String {
return "NativeCodeCrash(fatal=$fatal, minidumpSuccess=$minidumpSuccess)"
}
private fun createMessage(crash: Crash.UncaughtExceptionCrash): String {
return crash.throwable.message ?: ""
}
private fun createMessage(throwable: Throwable): String {
return "$INFO_PREFIX ${throwable.message}"
}
......@@ -12,6 +12,7 @@ import mozilla.components.lib.crash.service.CrashReporterService
import mozilla.components.support.test.any
import mozilla.components.support.test.eq
import mozilla.components.support.test.expectException
import mozilla.components.support.test.ext.joinBlocking
import mozilla.components.support.test.mock
import mozilla.components.support.test.robolectric.testContext
import org.junit.Assert.assertEquals
......@@ -167,16 +168,25 @@ class CrashReporterTest {
fun `CrashReporter forwards crashes to service`() {
var nativeCrash = false
var exceptionCrash = false
var caughtException = false
val service = object : CrashReporterService {
override fun report(crash: Crash.UncaughtExceptionCrash) {
exceptionCrash = true
nativeCrash = false
caughtException = false
}
override fun report(crash: Crash.NativeCodeCrash) {
exceptionCrash = false
nativeCrash = true
caughtException = false
}
override fun report(throwable: Throwable) {
exceptionCrash = false
nativeCrash = false
caughtException = true
}
}
......@@ -191,6 +201,7 @@ class CrashReporterTest {
assertTrue(exceptionCrash)
assertFalse(nativeCrash)
assertFalse(caughtException)
reporter.onCrash(
mock(),
......@@ -199,6 +210,13 @@ class CrashReporterTest {
assertFalse(exceptionCrash)
assertTrue(nativeCrash)
assertFalse(caughtException)
reporter.submitCaughtException(RuntimeException()).joinBlocking()
assertFalse(exceptionCrash)
assertFalse(nativeCrash)
assertTrue(caughtException)
}
@Test
......
......@@ -42,6 +42,10 @@ class CrashHandlerServiceTest {
override fun report(crash: Crash.NativeCodeCrash) {
caughtCrash = crash
}
override fun report(throwable: Throwable) {
fail("Didn't expect caught exception")
}
})
).install(testContext)
......
......@@ -34,6 +34,10 @@ class ExceptionHandlerTest {
override fun report(crash: Crash.NativeCodeCrash) {
fail("Did not expect native crash")
}
override fun report(throwable: Throwable) {
fail("Did not expect caught exception")
}
}
val crashReporter = spy(CrashReporter(
......@@ -70,6 +74,9 @@ class ExceptionHandlerTest {
override fun report(crash: Crash.NativeCodeCrash) {
}
override fun report(throwable: Throwable) {
}
})
).install(testContext)
......
......@@ -106,6 +106,44 @@ class GleanCrashReporterServiceTest {
}
}
@Test
fun `GleanCrashReporterService records caught exceptions`() {
// Because of how Glean is implemented, it can potentially persist information between
// tests or even between test classes, so we compensate by capturing the initial value
// to compare to.
val initialValue = try {
CrashMetrics.crashCount[GleanCrashReporterService.CAUGHT_EXCEPTION_KEY].testGetValue()
} catch (e: NullPointerException) {
0
}
run {
val service = spy(GleanCrashReporterService(context))
assertFalse("No previous persisted crashes must exist", service.file.exists())
val throwable = RuntimeException("Test")
service.report(throwable)
verify(service).report(throwable)
assertTrue("Persistence file must exist", service.file.exists())
val lines = service.file.readLines()
assertEquals("Must be caught exception",
GleanCrashReporterService.CAUGHT_EXCEPTION_KEY, lines.first())
}
// Initialize a fresh GleanCrashReporterService and ensure metrics are recorded in Glean
run {
GleanCrashReporterService(context)
assertTrue("Glean must record a value",
CrashMetrics.crashCount[GleanCrashReporterService.CAUGHT_EXCEPTION_KEY].testHasValue())
assertEquals("Glean must record correct value",
1,
CrashMetrics.crashCount[GleanCrashReporterService.CAUGHT_EXCEPTION_KEY].testGetValue() - initialValue)
}
}
@Test
fun `GleanCrashReporterService correctly handles multiple crashes in a single file`() {
val initialExceptionValue = try {
......
......@@ -13,6 +13,7 @@ import okhttp3.mockwebserver.MockResponse
import okhttp3.mockwebserver.MockWebServer
import org.junit.Test
import org.junit.runner.RunWith
import org.mockito.ArgumentMatchers.anyBoolean
import org.mockito.Mockito.doNothing
import org.mockito.Mockito.spy
import org.mockito.Mockito.verify
......@@ -31,13 +32,13 @@ class MozillaSocorroServiceTest {
testContext,
"Test App"
))
doNothing().`when`(service).sendReport(any(), any(), any())
doNothing().`when`(service).sendReport(any(), any(), any(), anyBoolean())
val crash = Crash.NativeCodeCrash("", true, "", false, arrayListOf())
service.report(crash)
verify(service).report(crash)
verify(service).sendReport(null, crash.minidumpPath, crash.extrasPath)
verify(service).sendReport(null, crash.minidumpPath, crash.extrasPath, false)
}
@Test
......@@ -46,13 +47,28 @@ class MozillaSocorroServiceTest {
testContext,
"Test App"
))
doNothing().`when`(service).sendReport(any(), any(), any())
doNothing().`when`(service).sendReport(any(), any(), any(), anyBoolean())
val crash = Crash.UncaughtExceptionCrash(RuntimeException("Test"), arrayListOf())
service.report(crash)
verify(service).report(crash)
verify(service).sendReport(crash.throwable, null, null)
verify(service).sendReport(crash.throwable, null, null, false)
}
@Test
fun `MozillaSocorroService send caught exception`() {
val service = spy(MozillaSocorroService(
testContext,
"Test App"
))
doNothing().`when`(service).sendReport(any(), any(), any(), anyBoolean())
val throwable = RuntimeException("Test")
service.report(throwable)
verify(service).report(throwable)
verify(service).sendReport(throwable, null, null, true)
}
@Test
......@@ -86,7 +102,41 @@ class MozillaSocorroServiceTest {
assert(request.contains("name=Android_Device\r\n\r\nrobolectric"))
verify(service).report(crash)
verify(service).sendReport(crash.throwable, null, null)
verify(service).sendReport(crash.throwable, null, null, false)
}
@Test
fun `MozillaSocorroService caught exception request is correct`() {
var mockWebServer = MockWebServer()
mockWebServer.enqueue(MockResponse().setResponseCode(200)
.setBody("CrashID=bp-924121d3-4de3-4b32-ab12-026fc0190928"))
mockWebServer.start()
val serverUrl = mockWebServer.url("/")
val service = spy(MozillaSocorroService(
testContext,
"Test App",
serverUrl.toString()
))
val throwable = RuntimeException("Test")
service.report(throwable)
val fileInputStream = ByteArrayInputStream(mockWebServer.takeRequest().body.inputStream().readBytes())
val inputStream = GZIPInputStream(fileInputStream)
val reader = InputStreamReader(inputStream)
val bufferedReader = BufferedReader(reader)
var request = bufferedReader.readText()
assert(request.contains("name=JavaStackTrace\r\n\r\n$INFO_PREFIX java.lang.RuntimeException: Test"))
assert(request.contains("name=Android_ProcessName\r\n\r\nmozilla.components.lib.crash.test"))
assert(request.contains("name=ProductID\r\n\r\n{aa3c5121-dab2-40e2-81ca-7ea25febc110}"))
assert(request.contains("name=Vendor\r\n\r\nMozilla"))
assert(request.contains("name=ReleaseChannel\r\n\r\nnightly"))
assert(request.contains("name=Android_PackageName\r\n\r\nmozilla.components.lib.crash.test"))
assert(request.contains("name=Android_Device\r\n\r\nrobolectric"))
verify(service).report(throwable)
verify(service).sendReport(throwable, null, null, true)
}
@Test
......@@ -107,7 +157,7 @@ class MozillaSocorroServiceTest {
mockWebServer.shutdown()
verify(service).report(crash)
verify(service).sendReport(crash.throwable, null, null)
verify(service).sendReport(crash.throwable, null, null, false)
}
@Test
......@@ -122,12 +172,12 @@ class MozillaSocorroServiceTest {
serverUrl.toString()
))
val crash = Crash.NativeCodeCrash("", true, "", false, arrayListOf())
val crash = Crash.NativeCodeCrash(null, true, null, false, arrayListOf())
service.report(crash)
mockWebServer.shutdown()
verify(service).report(crash)
verify(service).sendReport(null, crash.minidumpPath, crash.extrasPath)
verify(service).sendReport(null, crash.minidumpPath, crash.extrasPath, false)
}
@Test
......
......@@ -45,6 +45,9 @@ class SendCrashReportServiceTest {
override fun report(crash: Crash.NativeCodeCrash) {
}
override fun report(throwable: Throwable) {