From 171df35c41f4f0ad488c6e2fe4ee75ce64c259cb Mon Sep 17 00:00:00 2001 From: Craig Russell Date: Wed, 22 Apr 2020 17:30:36 +0100 Subject: [PATCH 1/3] InterruptedException and IOException now excluded exceptions to alert on --- .../AlertingUncaughtExceptionHandler.kt | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/app/src/main/java/com/duckduckgo/app/global/AlertingUncaughtExceptionHandler.kt b/app/src/main/java/com/duckduckgo/app/global/AlertingUncaughtExceptionHandler.kt index f6744f45fc34..d4afb0d24b58 100644 --- a/app/src/main/java/com/duckduckgo/app/global/AlertingUncaughtExceptionHandler.kt +++ b/app/src/main/java/com/duckduckgo/app/global/AlertingUncaughtExceptionHandler.kt @@ -19,10 +19,13 @@ package com.duckduckgo.app.global import com.duckduckgo.app.global.exception.UncaughtExceptionRepository import com.duckduckgo.app.global.exception.UncaughtExceptionSource import com.duckduckgo.app.statistics.store.OfflinePixelCountDataStore +import io.reactivex.exceptions.UndeliverableException import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.GlobalScope import kotlinx.coroutines.NonCancellable import kotlinx.coroutines.launch +import timber.log.Timber +import java.io.IOException class AlertingUncaughtExceptionHandler( private val originalHandler: Thread.UncaughtExceptionHandler, @@ -32,6 +35,11 @@ class AlertingUncaughtExceptionHandler( override fun uncaughtException(t: Thread?, originalException: Throwable?) { + if (shouldIgnore(originalException)) { + Timber.w(originalException, "An exception occurred but we don't need to handle it") + return + } + GlobalScope.launch(Dispatchers.IO + NonCancellable) { uncaughtExceptionRepository.recordUncaughtException(originalException, UncaughtExceptionSource.GLOBAL) offlinePixelCountDataStore.applicationCrashCount += 1 @@ -40,4 +48,17 @@ class AlertingUncaughtExceptionHandler( originalHandler.uncaughtException(t, originalException) } } + + /** + * Some exceptions happen due to lifecycle issues, such as our sync service being forced to stop. + * In such cases, we don't need to alert about the exception as they are exceptions essentially signalling that work was interrupted. + * Examples of this would be if the internet was lost during the sync, + * or when two or more sync operations are scheduled to run at the same time; one would run and the rest would be interrupted. + */ + private fun shouldIgnore(exception: Throwable?): Boolean { + return when (exception) { + is UndeliverableException, is InterruptedException, is IOException -> true + else -> false + } + } } \ No newline at end of file From d4f854f301d291d8643af37fc7e861ccf0044996 Mon Sep 17 00:00:00 2001 From: Craig Russell Date: Thu, 23 Apr 2020 09:15:20 +0100 Subject: [PATCH 2/3] Change IOException to InterruptedIOException and permit crash on DEBUG --- .../AlertingUncaughtExceptionHandler.kt | 40 ++++++++++++------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/global/AlertingUncaughtExceptionHandler.kt b/app/src/main/java/com/duckduckgo/app/global/AlertingUncaughtExceptionHandler.kt index d4afb0d24b58..54b23a0b76d8 100644 --- a/app/src/main/java/com/duckduckgo/app/global/AlertingUncaughtExceptionHandler.kt +++ b/app/src/main/java/com/duckduckgo/app/global/AlertingUncaughtExceptionHandler.kt @@ -16,6 +16,7 @@ package com.duckduckgo.app.global +import com.duckduckgo.app.browser.BuildConfig import com.duckduckgo.app.global.exception.UncaughtExceptionRepository import com.duckduckgo.app.global.exception.UncaughtExceptionSource import com.duckduckgo.app.statistics.store.OfflinePixelCountDataStore @@ -24,8 +25,7 @@ import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.GlobalScope import kotlinx.coroutines.NonCancellable import kotlinx.coroutines.launch -import timber.log.Timber -import java.io.IOException +import java.io.InterruptedIOException class AlertingUncaughtExceptionHandler( private val originalHandler: Thread.UncaughtExceptionHandler, @@ -33,20 +33,17 @@ class AlertingUncaughtExceptionHandler( private val uncaughtExceptionRepository: UncaughtExceptionRepository ) : Thread.UncaughtExceptionHandler { - override fun uncaughtException(t: Thread?, originalException: Throwable?) { + override fun uncaughtException(thread: Thread?, originalException: Throwable?) { - if (shouldIgnore(originalException)) { - Timber.w(originalException, "An exception occurred but we don't need to handle it") + if (shouldRecordExceptionAndCrashApp(originalException)) { + recordExceptionAndAllowCrash(thread, originalException) return } - GlobalScope.launch(Dispatchers.IO + NonCancellable) { - uncaughtExceptionRepository.recordUncaughtException(originalException, UncaughtExceptionSource.GLOBAL) - offlinePixelCountDataStore.applicationCrashCount += 1 - - // wait until the exception has been fully processed before propagating exception - originalHandler.uncaughtException(t, originalException) + if (shouldCrashApp()) { + originalHandler.uncaughtException(thread, originalException) } + } /** @@ -55,10 +52,25 @@ class AlertingUncaughtExceptionHandler( * Examples of this would be if the internet was lost during the sync, * or when two or more sync operations are scheduled to run at the same time; one would run and the rest would be interrupted. */ - private fun shouldIgnore(exception: Throwable?): Boolean { + private fun shouldRecordExceptionAndCrashApp(exception: Throwable?): Boolean { return when (exception) { - is UndeliverableException, is InterruptedException, is IOException -> true - else -> false + is UndeliverableException, is InterruptedException, is InterruptedIOException -> false + else -> true + } + } + + /** + * If the exception is one we don't report on, we still want to see a crash when we're in DEBUG builds for safety we aren't ignoring important issues + */ + private fun shouldCrashApp(): Boolean = BuildConfig.DEBUG + + private fun recordExceptionAndAllowCrash(thread: Thread?, originalException: Throwable?) { + GlobalScope.launch(Dispatchers.IO + NonCancellable) { + uncaughtExceptionRepository.recordUncaughtException(originalException, UncaughtExceptionSource.GLOBAL) + offlinePixelCountDataStore.applicationCrashCount += 1 + + // wait until the exception has been fully processed before propagating exception + originalHandler.uncaughtException(thread, originalException) } } } \ No newline at end of file From 5b19fb75585c160e845a48bce41f215c7195cacc Mon Sep 17 00:00:00 2001 From: Craig Russell Date: Thu, 23 Apr 2020 09:24:11 +0100 Subject: [PATCH 3/3] Remove UndeliverableException from alerting handler; handled elsewhere --- .../duckduckgo/app/global/AlertingUncaughtExceptionHandler.kt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/global/AlertingUncaughtExceptionHandler.kt b/app/src/main/java/com/duckduckgo/app/global/AlertingUncaughtExceptionHandler.kt index 54b23a0b76d8..d8c3217ce6f1 100644 --- a/app/src/main/java/com/duckduckgo/app/global/AlertingUncaughtExceptionHandler.kt +++ b/app/src/main/java/com/duckduckgo/app/global/AlertingUncaughtExceptionHandler.kt @@ -20,7 +20,6 @@ import com.duckduckgo.app.browser.BuildConfig import com.duckduckgo.app.global.exception.UncaughtExceptionRepository import com.duckduckgo.app.global.exception.UncaughtExceptionSource import com.duckduckgo.app.statistics.store.OfflinePixelCountDataStore -import io.reactivex.exceptions.UndeliverableException import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.GlobalScope import kotlinx.coroutines.NonCancellable @@ -54,7 +53,7 @@ class AlertingUncaughtExceptionHandler( */ private fun shouldRecordExceptionAndCrashApp(exception: Throwable?): Boolean { return when (exception) { - is UndeliverableException, is InterruptedException, is InterruptedIOException -> false + is InterruptedException, is InterruptedIOException -> false else -> true } }