[components] Closes https://github.com/mozilla-mobile/android-components/issues/5677: Catch all known non-fatal push errors

Previously, we wanted to throw on all unknown push errors so that we were notified on them. Since this seems to be more common than originally expected, we should just catch them and in a future version, we should log them without crashing. All of these push errors can be considered recoverable except for InternalPanic.
2020-01-22 12:28:47 -05:00
parent 7f11f02e1d
commit 81640524f0
7 changed files with 113 additions and 32 deletions
--- a/mobile/android/android-components/components/concept/push/src/main/java/mozilla/components/concept/push/PushProcessor.kt
+++ b/mobile/android/android-components/components/concept/push/src/main/java/mozilla/components/concept/push/PushProcessor.kt
@@ -95,13 +95,16 @@ data class EncryptedPushMessage(
 /**
 *  Various error types.
 */
-sealed class PushError(open val desc: String) {
-    data class Registration(override val desc: String) : PushError(desc)
-    data class Network(override val desc: String) : PushError(desc)
+sealed class PushError(override val message: String) : Exception() {
+    data class Registration(override val message: String) : PushError(message)
+    data class Network(override val message: String) : PushError(message)
    /**
     * @property cause Original exception from Rust code.
     */
-    data class Rust(val cause: Exception) : PushError(cause.toString())
-    data class MalformedMessage(override val desc: String) : PushError(desc)
-    data class ServiceUnavailable(override val desc: String) : PushError(desc)
+    data class Rust(
+        override val cause: Throwable?,
+        override val message: String = cause?.message.orEmpty()
+    ) : PushError(message)
+    data class MalformedMessage(override val message: String) : PushError(message)
+    data class ServiceUnavailable(override val message: String) : PushError(message)
 }
--- a/mobile/android/android-components/components/concept/push/src/test/java/mozilla/components/concept/push/PushErrorTest.kt
+++ b/mobile/android/android-components/components/concept/push/src/test/java/mozilla/components/concept/push/PushErrorTest.kt
@@ -6,7 +6,6 @@ package mozilla.components.concept.push

 import org.junit.Assert.assertEquals
 import org.junit.Test
-import java.lang.IllegalStateException

 class PushErrorTest {
    @Test
@@ -14,18 +13,20 @@ class PushErrorTest {
        // This test is mostly to satisfy coverage.

        var error: PushError = PushError.MalformedMessage("message")
-        assertEquals("message", error.desc)
+        assertEquals("message", error.message)

        error = PushError.Network("network")
-        assertEquals("network", error.desc)
+        assertEquals("network", error.message)

        error = PushError.Registration("reg")
-        assertEquals("reg", error.desc)
+        assertEquals("reg", error.message)

-        error = PushError.Rust(IllegalStateException("boo"))
-        assertEquals("java.lang.IllegalStateException: boo", error.desc)
+        val exception = IllegalStateException()
+        val rustError = PushError.Rust(exception, "rust")
+        assertEquals("rust", rustError.message)
+        assertEquals(exception, rustError.cause)

        error = PushError.ServiceUnavailable("service")
-        assertEquals("service", error.desc)
+        assertEquals("service", error.message)
    }
 }
--- a/mobile/android/android-components/components/feature/push/src/main/java/mozilla/components/feature/push/AutoPushFeature.kt
+++ b/mobile/android/android-components/components/feature/push/src/main/java/mozilla/components/feature/push/AutoPushFeature.kt
@@ -15,10 +15,17 @@ import kotlinx.coroutines.SupervisorJob
 import kotlinx.coroutines.asCoroutineDispatcher
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.plus
+import mozilla.appservices.push.AlreadyRegisteredError
 import mozilla.appservices.push.CommunicationError
 import mozilla.appservices.push.CommunicationServerError
+import mozilla.appservices.push.CryptoError
+import mozilla.appservices.push.MissingRegistrationTokenError
 import mozilla.appservices.push.RecordNotFoundError
+import mozilla.appservices.push.StorageError
+import mozilla.appservices.push.StorageSqlError
 import mozilla.appservices.push.SubscriptionResponse
+import mozilla.appservices.push.TranscodingError
+import mozilla.appservices.push.UrlParseError
 import mozilla.components.concept.push.Bus
 import mozilla.components.concept.push.EncryptedPushMessage
 import mozilla.components.concept.push.PushError
@@ -182,17 +189,9 @@ class AutoPushFeature(
    }

    override fun onError(error: PushError) {
-        logger.error("${error.javaClass.simpleName} error: ${error.desc}")
+        logger.error("${error.javaClass.simpleName} error: ${error.message}")

-        // Submit via our configured CrashReporter, as well.
-        when (error) {
-            is PushError.Rust -> {
-                crashReporter?.submitCaughtException(error.cause)
-            }
-            else -> {
-                crashReporter?.submitCaughtException(GenericPushError(error.desc))
-            }
-        }
+        crashReporter?.submitCaughtException(error)
    }

    /**
@@ -319,7 +318,7 @@ class AutoPushFeature(

    private fun CoroutineScope.launchAndTry(block: suspend CoroutineScope.() -> Unit): Job {
        return launchAndTry(block, { e ->
-            onError(PushError.Rust(e))
+            onError(PushError.Rust(e, e.message.orEmpty()))
        })
    }

@@ -351,6 +350,9 @@ class AutoPushFeature(
    }
 }

+/**
+ * Catches all known non-fatal push errors logs.
+ */
 internal fun CoroutineScope.launchAndTry(
    block: suspend CoroutineScope.() -> Unit,
    errorBlock: (Exception) -> Unit
@@ -359,7 +361,21 @@ internal fun CoroutineScope.launchAndTry(
        try {
            block()
        } catch (e: RustPushError) {
-            if (e !is CommunicationServerError && e !is CommunicationError && e !is RecordNotFoundError) {
+            val result = when (e) {
+                is CryptoError,
+                is CommunicationError,
+                is CommunicationServerError,
+                is AlreadyRegisteredError,
+                is StorageError,
+                is MissingRegistrationTokenError,
+                is StorageSqlError,
+                is TranscodingError,
+                is RecordNotFoundError,
+                is UrlParseError -> false
+                else -> true
+            }
+
+            if (result) {
                throw e
            }

@@ -368,8 +384,6 @@ internal fun CoroutineScope.launchAndTry(
    }
 }

-private class GenericPushError(desc: String) : Exception(desc)
-
 /**
 * The different kind of message types that a [EncryptedPushMessage] can be:
 *  - Application Services (e.g. FxA/Send Tab)
--- a/mobile/android/android-components/components/feature/push/src/test/java/mozilla/components/feature/push/AutoPushFeatureKtTest.kt
+++ b/mobile/android/android-components/components/feature/push/src/test/java/mozilla/components/feature/push/AutoPushFeatureKtTest.kt
@@ -7,13 +7,20 @@ package mozilla.components.feature.push
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.test.runBlockingTest
+import mozilla.appservices.push.AlreadyRegisteredError
 import mozilla.appservices.push.CommunicationError
 import mozilla.appservices.push.CommunicationServerError
 import mozilla.appservices.push.CryptoError
 import mozilla.appservices.push.KeyInfo
+import mozilla.appservices.push.InternalPanic
+import mozilla.appservices.push.MissingRegistrationTokenError
 import mozilla.appservices.push.RecordNotFoundError
+import mozilla.appservices.push.StorageError
+import mozilla.appservices.push.StorageSqlError
 import mozilla.appservices.push.SubscriptionInfo
 import mozilla.appservices.push.SubscriptionResponse
+import mozilla.appservices.push.TranscodingError
+import mozilla.appservices.push.UrlParseError
 import org.junit.Assert.assertEquals
 import org.junit.Test

@@ -48,13 +55,18 @@ class AutoPushFeatureKtTest {
        assertEquals(ServiceType.ADM, config2.serviceType)
    }

-    @Test(expected = CryptoError::class)
+    @Test(expected = InternalPanic::class)
    fun `launchAndTry throws on unrecoverable Rust exceptions`() = runBlockingTest {
-        CoroutineScope(coroutineContext).launchAndTry({ throw CryptoError("unit test") }, { assert(false) })
+        CoroutineScope(coroutineContext).launchAndTry({ throw InternalPanic("unit test") }, { assert(false) })
    }

    @Test
    fun `launchAndTry should NOT throw on recoverable Rust exceptions`() = runBlockingTest {
+        CoroutineScope(coroutineContext).launchAndTry(
+            { throw CryptoError("should not fail test") },
+            { assert(true) }
+        )
+
        CoroutineScope(coroutineContext).launchAndTry(
            { throw CommunicationServerError("should not fail test") },
            { assert(true) }
@@ -65,9 +77,39 @@ class AutoPushFeatureKtTest {
            { assert(true) }
        )

+        CoroutineScope(coroutineContext).launchAndTry(
+            { throw AlreadyRegisteredError() },
+            { assert(true) }
+        )
+
+        CoroutineScope(coroutineContext).launchAndTry(
+            { throw StorageError("should not fail test") },
+            { assert(true) }
+        )
+
+        CoroutineScope(coroutineContext).launchAndTry(
+            { throw MissingRegistrationTokenError() },
+            { assert(true) }
+        )
+
+        CoroutineScope(coroutineContext).launchAndTry(
+            { throw StorageSqlError("should not fail test") },
+            { assert(true) }
+        )
+
+        CoroutineScope(coroutineContext).launchAndTry(
+            { throw TranscodingError("should not fail test") },
+            { assert(true) }
+        )
+
        CoroutineScope(coroutineContext).launchAndTry(
            { throw RecordNotFoundError("should not fail test") },
            { assert(true) }
        )
+
+        CoroutineScope(coroutineContext).launchAndTry(
+            { throw UrlParseError("should not fail test") },
+            { assert(true) }
+        )
    }
 }
--- a/mobile/android/android-components/components/feature/push/src/test/java/mozilla/components/feature/push/AutoPushFeatureTest.kt
+++ b/mobile/android/android-components/components/feature/push/src/test/java/mozilla/components/feature/push/AutoPushFeatureTest.kt
@@ -16,11 +16,13 @@ import mozilla.appservices.push.SubscriptionInfo
 import mozilla.appservices.push.SubscriptionResponse
 import mozilla.components.concept.push.Bus
 import mozilla.components.concept.push.EncryptedPushMessage
+import mozilla.components.concept.push.PushError
 import mozilla.components.concept.push.PushService
 import mozilla.components.feature.push.AutoPushFeature.Companion.LAST_VERIFIED
 import mozilla.components.feature.push.AutoPushFeature.Companion.PERIODIC_INTERVAL_MILLISECONDS
 import mozilla.components.feature.push.AutoPushFeature.Companion.PREFERENCE_NAME
 import mozilla.components.feature.push.AutoPushFeature.Companion.PREF_TOKEN
+import mozilla.components.lib.crash.CrashReporter
 import mozilla.components.support.test.any
 import mozilla.components.support.test.mock
 import mozilla.components.support.test.robolectric.testContext
@@ -338,6 +340,25 @@ class AutoPushFeatureTest {
        feature.verifyActiveSubscriptions()
    }

+    @Test
+    fun `crash reporter is notified of errors`() = runBlockingTest {
+        val native: PushConnection = TestPushConnection(true)
+        val crashReporter: CrashReporter = mock()
+        val feature = spy(
+            AutoPushFeature(
+                context = testContext,
+                service = mock(),
+                config = mock(),
+                coroutineContext = coroutineContext,
+                connection = native,
+                crashReporter = crashReporter
+            )
+        )
+        feature.onError(PushError.Rust(PushError.MalformedMessage("Bad things happened!")))
+
+        verify(crashReporter).submitCaughtException(any<PushError.Rust>())
+    }
+
    companion object {
        private fun preference(context: Context): SharedPreferences {
            return context.getSharedPreferences(PREFERENCE_NAME, Context.MODE_PRIVATE)
--- a/mobile/android/android-components/components/lib/push-amazon/src/test/java/mozilla/components/lib/push/amazon/AbstractAmazonPushServiceTest.kt
+++ b/mobile/android/android-components/components/lib/push-amazon/src/test/java/mozilla/components/lib/push/amazon/AbstractAmazonPushServiceTest.kt
@@ -95,7 +95,7 @@ class AbstractAmazonPushServiceTest {
        verify(processor).onError(captor.capture())

        assertTrue(captor.value is PushError.Registration)
-        assertTrue(captor.value.desc.contains("registration failed"))
+        assertTrue(captor.value.message.contains("registration failed"))
    }

    @Test
@@ -111,7 +111,7 @@ class AbstractAmazonPushServiceTest {
        verify(processor).onError(captor.capture())

        assertTrue(captor.value is PushError.MalformedMessage)
-        assertTrue(captor.value.desc.contains("NoSuchElementException"))
+        assertTrue(captor.value.message.contains("NoSuchElementException"))
    }

    @Test
--- a/mobile/android/android-components/components/lib/push-firebase/src/test/java/mozilla/components/lib/push/firebase/AbstractFirebasePushServiceTest.kt
+++ b/mobile/android/android-components/components/lib/push-firebase/src/test/java/mozilla/components/lib/push/firebase/AbstractFirebasePushServiceTest.kt
@@ -88,7 +88,7 @@ class AbstractFirebasePushServiceTest {
        verify(processor).onError(captor.capture())

        assertTrue(captor.value is PushError.MalformedMessage)
-        assertTrue(captor.value.desc.contains("NoSuchElementException"))
+        assertTrue(captor.value.message.contains("NoSuchElementException"))
    }

    @Test