Unverified Commit cb03ee89 authored by Jonathan Almeida's avatar Jonathan Almeida
Browse files

Closes #6171: Simplify push error handling; reduce non-fatal reporting 

Based on the our crash reporting there are only a hand few of exceptions
that are non-fatal. For the rest, we can simply notify the crash
reporter.

 - Simplified state requirements for native invocation by replacing the
 `DeliveryManager.with` to an extension `PushConnection.ifInitialized`.
 - The new `CoroutineExceptionHandler` now notifies the crash reporter
 of fatal or unknown exceptions.
 - Moved extension functions to their equivalent files in an `.ext`
 package.
 - Added more tests to increase coverage. 🎉
parent edf84533

components/feature/push/src/main/java/mozilla/components/feature/push/AutoPushFeature.kt

+21 −73
Original line number Diff line number Diff line
@@ -7,35 +7,29 @@ package mozilla.components.feature.push 
import android.content.Context

import android.content.SharedPreferences

import androidx.annotation.VisibleForTesting

import kotlinx.coroutines.CoroutineExceptionHandler

import kotlinx.coroutines.CoroutineScope

import kotlinx.coroutines.Job

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.GeneralError

import mozilla.appservices.push.MissingRegistrationTokenError

import mozilla.appservices.push.RecordNotFoundError

import mozilla.appservices.push.StorageError

import mozilla.appservices.push.StorageSqlError

import mozilla.appservices.push.TranscodingError

import mozilla.appservices.push.UrlParseError

import mozilla.components.concept.push.EncryptedPushMessage

import mozilla.components.concept.push.PushError

import mozilla.components.concept.push.PushProcessor

import mozilla.components.concept.push.PushService

import mozilla.components.support.base.crash.CrashReporting

import mozilla.components.feature.push.ext.launchAndTry

import mozilla.components.feature.push.ext.ifInitialized

import mozilla.components.support.base.log.logger.Logger

import mozilla.components.support.base.observer.Observable

import mozilla.components.support.base.observer.ObserverRegistry

import java.io.File

import java.util.concurrent.Executors

import kotlin.coroutines.CoroutineContext

import mozilla.appservices.push.PushError as RustPushError



/**

 * A implementation of a [PushProcessor] that should live as a singleton by being installed
@@ -82,9 +76,8 @@ class AutoPushFeature( 
        serviceType = config.serviceType,

        databasePath = File(context.filesDir, DB_NAME).canonicalPath

    ),

    private val crashReporter: CrashReporting? = null,

    delegate: Observable<Observer> = ObserverRegistry()

) : PushProcessor, Observable<AutoPushFeature.Observer> by delegate {

    private val crashReporter: CrashReporting? = null

) : PushProcessor, Observable<AutoPushFeature.Observer> by ObserverRegistry() {



    private val logger = Logger("AutoPushFeature")
@@ -95,7 +88,7 @@ class AutoPushFeature( 
        get() = preferences(context).getLong(LAST_VERIFIED, System.currentTimeMillis())

        set(value) = preferences(context).edit().putLong(LAST_VERIFIED, value).apply()



    private val coroutineScope = CoroutineScope(coroutineContext) + SupervisorJob()

    private val coroutineScope = CoroutineScope(coroutineContext) + SupervisorJob() + exceptionHandler { onError(it) }



    init {

        // If we have a token, initialize the rust component first.
@@ -129,7 +122,7 @@ class AutoPushFeature( 
     * This should only be done on an account logout or app data deletion.

     */

    override fun shutdown() {

        DeliveryManager.runWithInitialized(connection) {

        connection.ifInitialized {

            coroutineScope.launch {

                unsubscribeAll()

            }
@@ -163,7 +156,7 @@ class AutoPushFeature( 
     * New encrypted messages received from a supported push messaging service.

     */

    override fun onMessageReceived(message: EncryptedPushMessage) {

        DeliveryManager.runWithInitialized(connection) {

        connection.ifInitialized {

            coroutineScope.launchAndTry {

                logger.info("New push message decrypted.")
@@ -200,7 +193,7 @@ class AutoPushFeature( 
        onSubscribeError: () -> Unit = {},

        onSubscribe: ((AutoPushSubscription) -> Unit) = {}

    ) {

        DeliveryManager.runWithInitialized(connection) {

        connection.ifInitialized {

            coroutineScope.launchAndTry(errorBlock = {

                onSubscribeError()

            }, block = {
@@ -222,7 +215,7 @@ class AutoPushFeature( 
        onUnsubscribeError: () -> Unit = {},

        onUnsubscribe: (Boolean) -> Unit = {}

    ) {

        DeliveryManager.runWithInitialized(connection) {

        connection.ifInitialized {

            coroutineScope.launchAndTry(errorBlock = {

                onUnsubscribeError()

            }, block = {
@@ -260,7 +253,7 @@ class AutoPushFeature( 
     */

    @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)

    internal fun verifyActiveSubscriptions() {

        DeliveryManager.runWithInitialized(connection) {

        connection.ifInitialized {

            coroutineScope.launchAndTry {

                val subscriptionChanges = verifyConnection()
@@ -286,16 +279,6 @@ class AutoPushFeature( 
        }

    }



    private fun CoroutineScope.launchAndTry(

        errorBlock: () -> Unit = {},

        block: suspend CoroutineScope.() -> Unit

    ): Job {

        return launchAndTry(block, { e ->

            errorBlock()

            onError(PushError.Rust(e, e.message.orEmpty()))

        })

    }



    private fun saveToken(context: Context, value: String) {

        preferences(context).edit().putString(PREF_TOKEN, value).apply()

    }
@@ -340,53 +323,18 @@ class AutoPushFeature( 
    }

}



/**

 * Catches all known non-fatal push errors logs.

 */

internal fun CoroutineScope.launchAndTry(

    block: suspend CoroutineScope.() -> Unit,

    errorBlock: (Exception) -> Unit

): Job {

    return launch {

        try {

            block()

        } catch (e: RustPushError) {

            val result = when (e) {

internal inline fun exceptionHandler(crossinline onError: (PushError) -> Unit) = CoroutineExceptionHandler { _, e ->

    val isFatal = when (e) {

        is PushError.MalformedMessage,

        is GeneralError,

        is CryptoError,

        is CommunicationError,

                is CommunicationServerError,

                is AlreadyRegisteredError,

                is StorageError,

                is MissingRegistrationTokenError,

                is StorageSqlError,

                is TranscodingError,

                is RecordNotFoundError,

                is UrlParseError -> false

        is CommunicationServerError -> false

        else -> true

    }



            if (result) {

                throw e

            }



            errorBlock(e)

        }

    }

}



/**

 * This is manager mapping of service type to channel ID, it will eventually be replaced by the

 * Application Service implementation.

 */

internal object DeliveryManager {

    /**

     * Executes the block if the Push Manager is initialized.

     */

    fun runWithInitialized(connection: PushConnection, block: PushConnection.() -> Unit) {

        if (connection.isInitialized()) {

            block(connection)

        }

    if (isFatal) {

        onError(PushError.Rust(e, e.message.orEmpty()))

    }

}

components/feature/push/src/main/java/mozilla/components/feature/push/ext/Connection.kt

0 → 100644
+19 −0
Original line number Diff line number Diff line 
/* This Source Code Form is subject to the terms of the Mozilla Public

 * License, v. 2.0. If a copy of the MPL was not distributed with this

 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */



package mozilla.components.feature.push.ext



import mozilla.components.feature.push.PushConnection

import mozilla.components.support.base.log.logger.Logger



/**

 * Executes the block if the Push Manager is initialized.

 */

internal inline fun PushConnection.ifInitialized(block: PushConnection.() -> Unit) {

    if (isInitialized()) {

        block()

    } else {

        Logger.error("Native push layer is not yet initialized.")

    }

}

components/feature/push/src/main/java/mozilla/components/feature/push/ext/CoroutineScope.kt

0 → 100644
+29 −0
Original line number Diff line number Diff line 
/* This Source Code Form is subject to the terms of the Mozilla Public

 * License, v. 2.0. If a copy of the MPL was not distributed with this

 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */



package mozilla.components.feature.push.ext



import kotlinx.coroutines.CoroutineScope

import kotlinx.coroutines.Job

import kotlinx.coroutines.launch

import mozilla.appservices.push.PushError



/**

 * Catches all fatal push errors to notify the callback before re-throwing.

 */

internal fun CoroutineScope.launchAndTry(

    errorBlock: (Exception) -> Unit = {},

    block: suspend CoroutineScope.() -> Unit

): Job {

    return launch {

        try {

            block()

        } catch (e: PushError) {

            errorBlock(e)



            // rethrow

            throw e

        }

    }

}

components/feature/push/src/test/java/mozilla/components/feature/push/AutoPushFeatureKtTest.kt

+21 −62
Original line number Diff line number Diff line
@@ -6,24 +6,21 @@ package mozilla.components.feature.push 


import kotlinx.coroutines.CoroutineScope

import kotlinx.coroutines.ExperimentalCoroutinesApi

import kotlinx.coroutines.launch

import kotlinx.coroutines.plus

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.GeneralError

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.TranscodingError

import mozilla.appservices.push.UrlParseError

import mozilla.components.concept.push.PushError

import org.junit.Assert.assertEquals

import org.junit.Assert.assertFalse

import org.junit.Assert.assertTrue

import org.junit.Test



@ExperimentalCoroutinesApi

@Suppress("Deprecation")

class AutoPushFeatureKtTest {



    @Test
@@ -41,66 +38,28 @@ class AutoPushFeatureKtTest { 
        assertEquals(ServiceType.ADM, config2.serviceType)

    }



    @Test(expected = InternalPanic::class)

    fun `launchAndTry throws on unrecoverable Rust exceptions`() = runBlockingTest {

        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) }

        )



        CoroutineScope(coroutineContext).launchAndTry(

            { throw CommunicationError("should not fail test") },

            { assert(true) }

        )



        CoroutineScope(coroutineContext).launchAndTry(

            { throw AlreadyRegisteredError() },

            { assert(true) }

        )



        CoroutineScope(coroutineContext).launchAndTry(

            { throw StorageError("should not fail test") },

            { assert(true) }

        )

    fun `exception handler handles exceptions`() = runBlockingTest {

        var invoked = false

        val scope = CoroutineScope(coroutineContext) + exceptionHandler { invoked = true }



        CoroutineScope(coroutineContext).launchAndTry(

            { throw MissingRegistrationTokenError() },

            { assert(true) }

        )

        scope.launch { throw PushError.MalformedMessage("test") }

        assertFalse(invoked)



        CoroutineScope(coroutineContext).launchAndTry(

            { throw StorageSqlError("should not fail test") },

            { assert(true) }

        )

        scope.launch { throw GeneralError("test") }

        assertFalse(invoked)



        CoroutineScope(coroutineContext).launchAndTry(

            { throw TranscodingError("should not fail test") },

            { assert(true) }

        )

        scope.launch { throw CryptoError("test") }

        assertFalse(invoked)



        CoroutineScope(coroutineContext).launchAndTry(

            { throw RecordNotFoundError("should not fail test") },

            { assert(true) }

        )

        scope.launch { throw CommunicationError("test") }

        assertFalse(invoked)



        CoroutineScope(coroutineContext).launchAndTry(

            { throw UrlParseError("should not fail test") },

            { assert(true) }

        )

        scope.launch { throw CommunicationServerError("test") }

        assertFalse(invoked)



        CoroutineScope(coroutineContext).launchAndTry(

            { throw GeneralError("should not fail test") },

            { assert(true) }

        )

        // An exception where we should invoke our callback.

        scope.launch { throw MissingRegistrationTokenError() }

        assertTrue(invoked)

    }

}

components/feature/push/src/test/java/mozilla/components/feature/push/AutoPushFeatureTest.kt

+141 −25
Original line number Diff line number Diff line
@@ -11,6 +11,8 @@ import androidx.lifecycle.LifecycleOwner 
import androidx.test.ext.junit.runners.AndroidJUnit4

import kotlinx.coroutines.ExperimentalCoroutinesApi

import kotlinx.coroutines.test.runBlockingTest

import mozilla.appservices.push.GeneralError

import mozilla.appservices.push.MissingRegistrationTokenError

import mozilla.components.concept.push.EncryptedPushMessage

import mozilla.components.concept.push.PushError

import mozilla.components.concept.push.PushService
@@ -21,6 +23,7 @@ import mozilla.components.feature.push.AutoPushFeature.Companion.PREF_TOKEN 
import mozilla.components.support.base.crash.CrashReporting

import mozilla.components.support.test.any

import mozilla.components.support.test.mock

import mozilla.components.support.test.nullable

import mozilla.components.support.test.robolectric.testContext

import mozilla.components.support.test.whenever

import org.junit.Assert.assertEquals
@@ -41,16 +44,19 @@ import org.mockito.Mockito.verifyNoMoreInteractions 


@ExperimentalCoroutinesApi

@RunWith(AndroidJUnit4::class)

@Suppress("Deprecation")

class AutoPushFeatureTest {



    private var lastVerified: Long

        get() = preference(testContext).getLong(LAST_VERIFIED, System.currentTimeMillis())

        set(value) = preference(testContext).edit().putLong(LAST_VERIFIED, value).apply()



    private val connection: PushConnection = mock()



    @Before

    fun setup() {

        lastVerified = 0L



        whenever(connection.isInitialized()).thenReturn(true)

    }



    @Test
@@ -68,17 +74,13 @@ class AutoPushFeatureTest { 


    @Test

    fun `updateToken not called if no token in prefs`() = runBlockingTest {

        val connection: PushConnection = spy(TestPushConnection())



        spy(AutoPushFeature(testContext, mock(), mock(), coroutineContext, connection))

        AutoPushFeature(testContext, mock(), mock(), coroutineContext, connection)



        verify(connection, never()).updateToken(anyString())

    }



    @Test

    fun `updateToken called if token is in prefs`() = runBlockingTest {

        val connection: PushConnection = spy(TestPushConnection())



        preference(testContext).edit().putString(PREF_TOKEN, "token").apply()



        AutoPushFeature(
@@ -92,7 +94,6 @@ class AutoPushFeatureTest { 
    @Test

    fun `shutdown stops service and unsubscribes all`() = runBlockingTest {

        val service: PushService = mock()

        val connection: PushConnection = mock()

        whenever(connection.isInitialized()).thenReturn(true)



        spy(AutoPushFeature(testContext, service, mock(), coroutineContext, connection)).also {
@@ -104,9 +105,10 @@ class AutoPushFeatureTest { 


    @Test

    fun `onNewToken updates connection and saves pref`() = runBlockingTest {

        val connection: PushConnection = mock()

        val feature = spy(AutoPushFeature(testContext, mock(), mock(), coroutineContext, connection))



        whenever(connection.subscribe(anyString(), nullable())).thenReturn(mock())



        feature.onNewToken("token")



        verify(connection).updateToken("token")
@@ -130,14 +132,12 @@ class AutoPushFeatureTest { 


    @Test

    fun `onMessageReceived decrypts message and notifies observers`() = runBlockingTest {

        val connection: PushConnection = mock()

        val encryptedMessage: EncryptedPushMessage = mock()

        val owner: LifecycleOwner = mock()

        val lifecycle: Lifecycle = mock()

        val observer: AutoPushFeature.Observer = mock()

        whenever(owner.lifecycle).thenReturn(lifecycle)

        whenever(lifecycle.currentState).thenReturn(Lifecycle.State.STARTED)

        whenever(connection.isInitialized()).thenReturn(true)

        whenever(encryptedMessage.channelId).thenReturn("992a0f0542383f1ea5ef51b7cf4ae6c4")

        whenever(connection.decryptMessage(any(), any(), any(), any(), any()))

            .thenReturn(null) // If we get null, we shouldn't notify observers.
@@ -159,12 +159,9 @@ class AutoPushFeatureTest { 
    @Test

    fun `subscribe calls native layer and notifies observers`() = runBlockingTest {

        val connection: PushConnection = mock()

        val observer: AutoPushFeature.Observer = mock()

        val subscription: AutoPushSubscription = mock()

        var invoked = false



        val feature = spy(AutoPushFeature(testContext, mock(), mock(), coroutineContext, connection))

        feature.register(observer)



        feature.subscribe("testScope") {

            invoked = true
@@ -173,7 +170,7 @@ class AutoPushFeatureTest { 
        assertFalse(invoked)



        whenever(connection.isInitialized()).thenReturn(true)

        whenever(connection.subscribe(anyString(), nullable(String::class.java))).thenReturn(subscription)

        whenever(connection.subscribe(anyString(), nullable())).thenReturn(subscription)

        whenever(subscription.scope).thenReturn("testScope")



        feature.subscribe("testScope") {
@@ -183,19 +180,60 @@ class AutoPushFeatureTest { 
        assertTrue(invoked)

    }



    @Test

    fun `subscribe invokes error callback`() = runBlockingTest {

        val connection: PushConnection = mock()

        val subscription: AutoPushSubscription = mock()

        var invoked = false

        var errorInvoked = false

        val feature = spy(AutoPushFeature(testContext, mock(), mock(), coroutineContext, connection))



        feature.subscribe(

            scope = "testScope",

            onSubscribeError = {

                errorInvoked = true

            }, onSubscribe = {

                invoked = true

            }

        )



        assertFalse(invoked)

        assertFalse(errorInvoked)



        whenever(connection.isInitialized()).thenReturn(true)

        whenever(connection.subscribe(anyString(), nullable())).thenAnswer { throw MissingRegistrationTokenError() }

        whenever(subscription.scope).thenReturn("testScope")



        feature.subscribe(

            scope = "testScope",

            onSubscribeError = {

                errorInvoked = true

            }, onSubscribe = {

                invoked = true

            }

        )



        assertFalse(invoked)

        assertTrue(errorInvoked)

    }



    @Test

    fun `unsubscribe calls native layer and notifies observers`() = runBlockingTest {

        val connection: PushConnection = mock()

        val observer: AutoPushFeature.Observer = mock()

        var invoked = false

        var errorInvoked = false



        val feature = spy(AutoPushFeature(testContext, mock(), mock(), coroutineContext, connection))

        feature.register(observer)



        feature.unsubscribe("testScope", { errorInvoked = true }) {

        feature.unsubscribe(

            scope = "testScope",

            onUnsubscribeError = {

                errorInvoked = true

            },

            onUnsubscribe = {

                invoked = true

            }

        )



        assertFalse(errorInvoked)

        assertFalse(invoked)
@@ -203,19 +241,55 @@ class AutoPushFeatureTest { 
        whenever(connection.unsubscribe(anyString())).thenReturn(false)

        whenever(connection.isInitialized()).thenReturn(true)



        feature.unsubscribe("testScope", { errorInvoked = true }) {

        feature.unsubscribe(

            scope = "testScope",

            onUnsubscribeError = {

                errorInvoked = true

            },

            onUnsubscribe = {

                invoked = true

            }

        )



        assertTrue(errorInvoked)

        errorInvoked = false



        whenever(connection.unsubscribe(anyString())).thenReturn(true)



        feature.unsubscribe("testScope") {

        feature.unsubscribe(

            scope = "testScope",

            onUnsubscribeError = {

                errorInvoked = true

            },

            onUnsubscribe = {

                invoked = true

            }

        )



        assertTrue(invoked)

        assertFalse(errorInvoked)

    }



    @Test

    fun `unsubscribe invokes error callback on native exception`() = runBlockingTest {

        val feature = AutoPushFeature(testContext, mock(), mock(), coroutineContext, connection)

        var invoked = false

        var errorInvoked = false



        whenever(connection.unsubscribe(anyString())).thenAnswer { throw MissingRegistrationTokenError() }



        feature.unsubscribe(

            scope = "testScope",

            onUnsubscribeError = {

                errorInvoked = true

            },

            onUnsubscribe = {

                invoked = true

            }

        )



        assertFalse(invoked)

        assertTrue(errorInvoked)

    }



    @Test
@@ -319,6 +393,48 @@ class AutoPushFeatureTest { 
        verify(crashReporter).submitCaughtException(any<PushError.Rust>())

    }



    @Test

    fun `non-fatal errors are ignored`() = runBlockingTest {

        val crashReporter: CrashReporting = mock()

        val feature = spy(

            AutoPushFeature(

                context = testContext,

                service = mock(),

                config = mock(),

                coroutineContext = coroutineContext,

                connection = connection,

                crashReporter = crashReporter

            )

        )



        whenever(connection.unsubscribe(any())).thenAnswer { throw GeneralError("test") }



        feature.unsubscribe("123") {}



        verify(crashReporter, never()).submitCaughtException(any<PushError.Rust>())

    }



    @Test

    fun `only fatal errors are reported`() = runBlockingTest {

        val crashReporter: CrashReporting = mock()

        val feature = spy(

            AutoPushFeature(

                context = testContext,

                service = mock(),

                config = mock(),

                coroutineContext = coroutineContext,

                connection = connection,

                crashReporter = crashReporter

            )

        )



        whenever(connection.unsubscribe(any())).thenAnswer { throw MissingRegistrationTokenError() }



        feature.unsubscribe("123") {}



        verify(crashReporter).submitCaughtException(any<PushError.Rust>())

    }



    companion object {

        private fun preference(context: Context): SharedPreferences {

            return context.getSharedPreferences(PREFERENCE_NAME, Context.MODE_PRIVATE)