Commit 78b1263b authored by MozLando's avatar MozLando
Browse files

Merge #8090



8090: Improved result/error handling during migration r=csadilek,pocmo a=grigoryk

Best reviewed commit by commit.

Changes should be mostly non-functional - just adding more coverage for exception handling.

In one narrow case we were counting potential failures as successes, swallowing the problem. The problem would still get reported as a caught exception via Sentry, and we haven't seen it happen.



Co-authored-by: default avatarGrisha Kruglov <gkruglov@mozilla.com>
parents a0375b26 b24a7318
Loading
Loading
Loading
Loading
+314 −256
Original line number Diff line number Diff line
@@ -47,10 +47,10 @@ import mozilla.components.support.migration.GleanMetrics.MigrationSearch
import mozilla.components.support.migration.GleanMetrics.MigrationSettings
import java.io.File
import java.lang.AssertionError
import java.lang.Exception
import java.util.Date
import java.util.UUID
import java.util.concurrent.Executors
import kotlin.Exception
import kotlin.IllegalStateException
import kotlin.coroutines.CoroutineContext

@@ -135,6 +135,12 @@ data class VersionedMigration(val migration: Migration, val version: Int = migra
 * exceptions.
 */
sealed class FennecMigratorException(cause: Exception) : Exception(cause) {
    /**
     * Unexpected exception during high level migration processing.
     * @param cause Original exception which caused the problem.
     */
    class HighLevel(cause: Exception) : FennecMigratorException(cause)

    /**
     * Unexpected exception while migrating history.
     * @param cause Original exception which caused the problem.
@@ -177,6 +183,12 @@ sealed class FennecMigratorException(cause: Exception) : Exception(cause) {
     */
    class MigrateAddonsException(cause: Exception) : FennecMigratorException(cause)

    /**
     * Unexpected exception while migrating FxA.
     * @param cause Original exception which caused the problem
     */
    class MigrateFxaException(cause: Exception) : FennecMigratorException(cause)

    /**
     * Unexpected exception while migrating telemetry identifiers.
     * @param cause Original exception which caused the problem.
@@ -576,7 +588,7 @@ class FennecMigrator private constructor(
        }
    }

    @Suppress("ComplexMethod")
    @Suppress("ComplexMethod", "TooGenericExceptionCaught")
    private fun runMigrationsAsync(
        store: MigrationStore,
        migrations: List<VersionedMigration>
@@ -597,7 +609,8 @@ class FennecMigrator private constructor(

            versionedMigration.migration.metricTotalDuration().start()

            val migrationResult = when (versionedMigration.migration) {
            val migrationResult: Result<*> = try {
                when (versionedMigration.migration) {
                    Migration.History -> migrateHistory()
                    Migration.Bookmarks -> migrateBookmarks()
                    Migration.OpenTabs -> migrateOpenTabs()
@@ -610,8 +623,13 @@ class FennecMigrator private constructor(
                    Migration.SearchEngine -> migrateSearchEngine()
                    Migration.PinnedSites -> migratePinnedSites()
                }

            } catch (e: Exception) {
                logger.error("Unexpected error while migrating $versionedMigration", e)
                crashReporter.submitCaughtException(FennecMigratorException.HighLevel(e))
                Result.Failure<Any>(e)
            } finally {
                versionedMigration.migration.metricTotalDuration().stop()
            }

            val migrationRun = when (migrationResult) {
                is Result.Failure<*> -> {
@@ -676,24 +694,23 @@ class FennecMigrator private constructor(

        // Process migration metrics. Here and elsewhere, we're assuming and hard-coding metrics schema.
        // See application-services repository: https://github.com/mozilla/application-services/commit/a7d5ff1903fb0f904785a1645cb7ae1d6c313f10
        try {
        return try {
            MigrationHistory.detected.add(migrationMetrics.getInt("num_total"))
            MigrationHistory.migrated["succeeded"].add(migrationMetrics.getInt("num_succeeded"))
            MigrationHistory.migrated["failed"].add(migrationMetrics.getInt("num_failed"))
            // Assuming that 'total_duration' is in milliseconds.
            MigrationHistory.duration.setRawNanos(migrationMetrics.getLong("total_duration") * 1000000)

            MigrationHistory.successReason.add(SuccessReasonTelemetryCodes.HISTORY_MIGRATED.code)
            logger.debug("Migrated history.")
            Result.Success(Unit)
        } catch (e: Exception) {
            MigrationHistory.anyFailures.set(true)
            MigrationHistory.failureReason.add(FailureReasonTelemetryCodes.HISTORY_TELEMETRY_EXCEPTION.code)
            crashReporter.submitCaughtException(
                FennecMigratorException.MigrateHistoryException(e)
            )
            Result.Failure(e)
        }

        MigrationHistory.successReason.add(SuccessReasonTelemetryCodes.HISTORY_MIGRATED.code)

        logger.debug("Migrated history.")
        return Result.Success(Unit)
    }

    @SuppressWarnings("TooGenericExceptionCaught", "MagicNumber", "ReturnCount")
@@ -723,23 +740,23 @@ class FennecMigrator private constructor(

        // Process migration metrics. Here and elsewhere, we're assuming and hard-coding metrics schema.
        // See application-services repository: https://github.com/mozilla/application-services/commit/b2e2edcc06a04503d493e1733b0d566815feac7c#diff-216f62325632ae6549587b038b21cfe0
        try {
        return try {
            MigrationBookmarks.detected.add(migrationMetrics.getInt("num_total"))
            MigrationBookmarks.migrated["succeeded"].add(migrationMetrics.getInt("num_succeeded"))
            MigrationBookmarks.migrated["failed"].add(migrationMetrics.getInt("num_failed"))
            // Assuming that 'total_duration' is in milliseconds.
            MigrationBookmarks.duration.setRawNanos(migrationMetrics.getLong("total_duration") * 1000000)

            logger.debug("Migrated bookmarks.")
            MigrationBookmarks.successReason.add(SuccessReasonTelemetryCodes.BOOKMARKS_MIGRATED.code)
            Result.Success(Unit)
        } catch (e: Exception) {
            MigrationBookmarks.failureReason.add(FailureReasonTelemetryCodes.BOOKMARKS_TELEMETRY_EXCEPTION.code)
            MigrationBookmarks.anyFailures.set(true)
            crashReporter.submitCaughtException(
                FennecMigratorException.MigrateBookmarksException(e)
            )
            Result.Failure(e)
        }

        logger.debug("Migrated history.")
        MigrationBookmarks.successReason.add(SuccessReasonTelemetryCodes.BOOKMARKS_MIGRATED.code)
        return Result.Success(Unit)
    }

    @Suppress("ComplexMethod", "TooGenericExceptionCaught", "LongMethod", "ReturnCount")
@@ -750,23 +767,18 @@ class FennecMigrator private constructor(
            return Result.Failure(IllegalStateException("Missing Profile path"))
        }

        val result = try {
        return try {
            logger.debug("Migrating logins...")
            FennecLoginsMigration.migrate(
            val result = FennecLoginsMigration.migrate(
                crashReporter,
                signonsDbPath = "${profile.path}/$signonsDbName",
                key4DbPath = "${profile.path}/$key4DbName",
                loginsStorage = loginsStorage!!.value
            )
        } catch (e: Exception) {
            crashReporter.submitCaughtException(FennecMigratorException.MigrateLoginsException(e))
            MigrationLogins.failureReason.add(FailureReasonTelemetryCodes.LOGINS_UNEXPECTED_EXCEPTION.code)
            return Result.Failure(e)
        }

            if (result is Result.Failure<LoginsMigrationResult>) {
                val migrationFailureWrapper = result.throwables.first() as LoginMigrationException
            return when (val failure = migrationFailureWrapper.failure) {
                when (val failure = migrationFailureWrapper.failure) {
                    is LoginsMigrationResult.Failure.FailedToCheckMasterPassword -> {
                        logger.error("Failed to check master password: $failure")
                        MigrationLogins.failureReason.add(FailureReasonTelemetryCodes.LOGINS_MP_CHECK.code)
@@ -803,10 +815,9 @@ class FennecMigrator private constructor(
                        result
                    }
                }
        }

            } else {
                val loginMigrationSuccess = result as Result.Success<LoginsMigrationResult>
        return when (val success = loginMigrationSuccess.value as LoginsMigrationResult.Success) {
                when (val success = loginMigrationSuccess.value as LoginsMigrationResult.Success) {
                    is LoginsMigrationResult.Success.MasterPasswordIsSet -> {
                        logger.debug("Could not migrate logins - master password is set")
                        MigrationLogins.successReason.add(SuccessReasonTelemetryCodes.LOGINS_MP_SET.code)
@@ -827,6 +838,12 @@ class FennecMigrator private constructor(
                    }
                }
            }
        } catch (e: Exception) {
            crashReporter.submitCaughtException(FennecMigratorException.MigrateLoginsException(e))
            MigrationLogins.failureReason.add(FailureReasonTelemetryCodes.LOGINS_UNEXPECTED_EXCEPTION.code)
            Result.Failure(e)
        }
    }

    @SuppressWarnings("TooGenericExceptionCaught")
    private suspend fun migrateOpenTabs(): Result<SessionManager.Snapshot> {
@@ -858,11 +875,11 @@ class FennecMigrator private constructor(
                    MigrationOpenTabs.migrated.add(sessionManager.all.size)
                    MigrationOpenTabs.successReason.add(SuccessReasonTelemetryCodes.OPEN_TABS_MIGRATED.code)
                }
            } else if (result is Result.Failure<*>) {
                MigrationOpenTabs.anyFailures.set(result.throwables.isNotEmpty())
                result
            } else {
                MigrationOpenTabs.failureReason.add(FailureReasonTelemetryCodes.OPEN_TABS_NO_SNAPSHOT.code)
            }
                result
            }
        } catch (e: Exception) {
            MigrationOpenTabs.failureReason.add(FailureReasonTelemetryCodes.OPEN_TABS_RESTORE_EXCEPTION.code)
            crashReporter.submitCaughtException(
@@ -872,13 +889,15 @@ class FennecMigrator private constructor(
        }
    }

    @Suppress("ComplexMethod", "LongMethod")
    @Suppress("ComplexMethod", "LongMethod", "TooGenericExceptionCaught", "NestedBlockDepth")
    private suspend fun migrateFxA(): Result<FxaMigrationResult> {
        return try {
            val result = FennecFxaMigration.migrate(fxaState!!, context, accountManager!!.value)

            if (result is Result.Failure<FxaMigrationResult>) {
            val migrationFailureWrapper = result.throwables.first() as FxaMigrationException
            return when (val failure = migrationFailureWrapper.failure) {
                val migrationFailureWrapper = result.throwables.first()
                if (migrationFailureWrapper is FxaMigrationException) {
                    when (val failure = migrationFailureWrapper.failure) {
                        is FxaMigrationResult.Failure.CorruptAccountState -> {
                            logger.error("Detected a corrupt account state: $failure")
                            MigrationFxa.failureReason.add(FailureReasonTelemetryCodes.FXA_CORRUPT_ACCOUNT_STATE.code)
@@ -907,10 +926,14 @@ class FennecMigrator private constructor(
                            result
                        }
                    }
                } else {
                    logger.error("Unexpected FxA migration exception", migrationFailureWrapper.cause)
                    MigrationFxa.failureReason.add(FailureReasonTelemetryCodes.FXA_MIGRATE_EXCEPTION.code)
                    result
                }

            } else {
                val migrationSuccess = result as Result.Success<FxaMigrationResult>
        return when (val success = migrationSuccess.value as FxaMigrationResult.Success) {
                when (val success = migrationSuccess.value as FxaMigrationResult.Success) {
                    // The rest are all successful migrations.
                    is FxaMigrationResult.Success.NoAccount -> {
                        logger.debug("No Fennec account detected")
@@ -919,7 +942,8 @@ class FennecMigrator private constructor(
                    }
                    is FxaMigrationResult.Success.UnauthenticatedAccount -> {
                        // Here we have an 'email' and a state label.
                // "Bad auth state" could be a few things - unverified account, bad credentials detected by Fennec, etc.
                        // "Bad auth state" could be a few things - unverified account, bad credentials
                        // detected by Fennec, etc
                        // We could try using the 'email' address as a starting point in the authentication flow.
                        logger.debug("Detected a Fennec account in a bad authentication state: ${success.stateLabel}")
                        MigrationFxa.successReason.add(SuccessReasonTelemetryCodes.FXA_BAD_AUTH.code)
@@ -938,6 +962,13 @@ class FennecMigrator private constructor(
                    }
                }
            }
        } catch (e: Exception) {
            logger.error("Unexpected FxA migration exception", e)
            MigrationFxa.failureReason.add(FailureReasonTelemetryCodes.FXA_MIGRATE_EXCEPTION.code)
            crashReporter.submitCaughtException(FennecMigratorException.MigrateFxaException(e))
            Result.Failure(e)
        }
    }

    @Suppress("ComplexMethod", "LongMethod", "TooGenericExceptionCaught", "ReturnCount")
    private fun migrateGecko(migrationVersion: Int): Result<GeckoMigrationResult> {
@@ -953,7 +984,7 @@ class FennecMigrator private constructor(

            if (result is Result.Failure<GeckoMigrationResult>) {
                val geckoFailureWrapper = result.throwables.first() as GeckoMigrationException
                return when (val failure = geckoFailureWrapper.failure) {
                when (val failure = geckoFailureWrapper.failure) {
                    is GeckoMigrationResult.Failure.FailedToDeleteFile -> {
                        logger.error("Failed to delete prefs.js file: $failure")
                        MigrationGecko.failureReason.add(FailureReasonTelemetryCodes.GECKO_FAILED_TO_DELETE_PREFS.code)
@@ -970,10 +1001,9 @@ class FennecMigrator private constructor(
                        result
                    }
                }
            }

            } else {
                val migrationSuccess = result as Result.Success<GeckoMigrationResult>
            return when (migrationSuccess.value as GeckoMigrationResult.Success) {
                when (migrationSuccess.value as GeckoMigrationResult.Success) {
                    is GeckoMigrationResult.Success.NoPrefsFile -> {
                        logger.debug("No prefs.js file found")
                        MigrationGecko.successReason.add(
@@ -1003,6 +1033,7 @@ class FennecMigrator private constructor(
                        result
                    }
                }
            }
        } catch (e: Exception) {
            MigrationGecko.failureReason.add(FailureReasonTelemetryCodes.GECKO_UNEXPECTED_EXCEPTION.code)
            crashReporter.submitCaughtException(
@@ -1072,17 +1103,17 @@ class FennecMigrator private constructor(
        }
    }

    @SuppressWarnings("TooGenericExceptionCaught")
    @SuppressWarnings("TooGenericExceptionCaught", "NestedBlockDepth")
    private suspend fun migrateSearchEngine(): Result<SearchEngineMigrationResult> {
        val manager = searchEngineManager
            ?: throw AssertionError("Migrating search engines without search engine manager set")

        try {
        return try {
            val result = SearchEngineMigration.migrate(context, manager)

            if (result is Result.Failure<SearchEngineMigrationResult>) {
                val migrationFailureWrapper = result.throwables.first() as SearchEngineMigrationException
                return when (val failure = migrationFailureWrapper.failure) {
                when (val failure = migrationFailureWrapper.failure) {
                    is SearchEngineMigrationResult.Failure.NoDefault -> {
                        logger.error("Missing search engine default: $failure")
                        MigrationSearch.failureReason.add(FailureReasonTelemetryCodes.SEARCH_NO_DEFAULT.code)
@@ -1095,22 +1126,22 @@ class FennecMigrator private constructor(
                        result
                    }
                }
            }

            } else {
                val migrationSuccess = result as Result.Success<SearchEngineMigrationResult>
            return when (migrationSuccess.value as SearchEngineMigrationResult.Success) {
                when (migrationSuccess.value as SearchEngineMigrationResult.Success) {
                    is SearchEngineMigrationResult.Success.SearchEngineMigrated -> {
                        logger.debug("Migrated default search engine")
                        MigrationSearch.successReason.add(SuccessReasonTelemetryCodes.SEARCH_MIGRATED.code)
                        result
                    }
                }
            }
        } catch (e: Exception) {
            crashReporter.submitCaughtException(
                FennecMigratorException.MigrateSearchEngineException(e)
            )
            MigrationSearch.failureReason.add(FailureReasonTelemetryCodes.SEARCH_EXCEPTION.code)
            return Result.Failure(e)
            Result.Failure(e)
        }
    }

@@ -1121,7 +1152,7 @@ class FennecMigrator private constructor(
            val result = AddonMigration.migrate(engine!!, addonCollectionProvider!!, addonUpdater!!)
            if (result is Result.Failure<AddonMigrationResult>) {
                val migrationFailureWrapper = result.throwables.first() as AddonMigrationException
                return when (val failure = migrationFailureWrapper.failure) {
                when (val failure = migrationFailureWrapper.failure) {
                    is AddonMigrationResult.Failure.FailedToQueryInstalledAddons -> {
                        logger.error("Failed to query installed add-ons: $failure")
                        MigrationAddons.failureReason.add(FailureReasonTelemetryCodes.ADDON_QUERY.code)
@@ -1144,10 +1175,9 @@ class FennecMigrator private constructor(
                        result
                    }
                }
            }

            } else {
                val migrationSuccess = result as Result.Success<AddonMigrationResult>
            return when (val success = migrationSuccess.value as AddonMigrationResult.Success) {
                when (val success = migrationSuccess.value as AddonMigrationResult.Success) {
                    is AddonMigrationResult.Success.NoAddons -> {
                        logger.debug("No add-ons to migrate")
                        MigrationAddons.successReason.add(SuccessReasonTelemetryCodes.ADDONS_NO.code)
@@ -1160,6 +1190,7 @@ class FennecMigrator private constructor(
                        result
                    }
                }
            }
        } catch (e: Exception) {
            crashReporter.submitCaughtException(
                FennecMigratorException.MigrateAddonsException(e)
@@ -1169,7 +1200,7 @@ class FennecMigrator private constructor(
        }
    }

    @SuppressWarnings("TooGenericExceptionCaught")
    @SuppressWarnings("TooGenericExceptionCaught", "NestedBlockDepth")
    private fun migrateTelemetryIdentifiers(): Result<TelemetryIdentifiersResult> {
        if (profile == null) {
            crashReporter.submitCaughtException(IllegalStateException("Missing Profile path"))
@@ -1179,25 +1210,27 @@ class FennecMigrator private constructor(
            return Result.Failure(IllegalStateException("Missing Profile path"))
        }

        val result = try {
            // Will submit unexpected errors via crashReporter.
            TelemetryIdentifiersMigration.migrate(profile.path, crashReporter)
        } catch (e: Exception) {
            crashReporter.submitCaughtException(
                FennecMigratorException.TelemetryIdentifierException(e)
            )
        return try {
            val result = TelemetryIdentifiersMigration.migrate(profile.path, crashReporter)

            if (result is Result.Failure<TelemetryIdentifiersResult>) {
                // TelemetryIdentifiersMigration.migrate should not report a Result.Failure,
                // but in case it does, record it.
                crashReporter.submitCaughtException(result.throwables.first())
                MigrationTelemetryIdentifiers.failureReason.add(
                    FailureReasonTelemetryCodes.TELEMETRY_IDENTIFIERS_MIGRATE_EXCEPTION.code
                )
            return Result.Failure(e)
        }

        if (result is Result.Success<TelemetryIdentifiersResult>) {
                result
            } else {
                result as Result.Success<TelemetryIdentifiersResult>
                when (val success = result.value as TelemetryIdentifiersResult.Success) {
                    is TelemetryIdentifiersResult.Success.Identifiers -> {
                        try {
                            // It's important that we're aware, during telemetry analysis, that these values
                            // are missing or present. Absence of a set value should be enough.
                    success.clientId?.let { MigrationTelemetryIdentifiers.fennecClientId.set(UUID.fromString(it)) }
                            success.clientId?.let {
                                MigrationTelemetryIdentifiers.fennecClientId.set(UUID.fromString(it))
                            }
                            // profileCreationDate is a unix timestamp.
                            success.profileCreationDate?.let {
                                MigrationTelemetryIdentifiers.fennecProfileCreationDate.set(Date(it))
@@ -1205,14 +1238,29 @@ class FennecMigrator private constructor(
                            MigrationTelemetryIdentifiers.successReason.add(
                                SuccessReasonTelemetryCodes.TELEMETRY_IDENTIFIERS_MIGRATED.code
                            )
                            result
                        } catch (e: Exception) {
                            crashReporter.submitCaughtException(FennecMigratorException.TelemetryIdentifierException(e))
                            MigrationTelemetryIdentifiers.failureReason.add(
                                FailureReasonTelemetryCodes.TELEMETRY_IDENTIFIERS_PARSE_SET_EXCEPTION.code
                            )
                            Result.Failure<TelemetryIdentifiersResult>(e)
                        }
                    }
                }

        return result
            }
        } catch (e: Exception) {
            crashReporter.submitCaughtException(
                FennecMigratorException.TelemetryIdentifierException(e)
            )
            MigrationTelemetryIdentifiers.failureReason.add(
                FailureReasonTelemetryCodes.TELEMETRY_IDENTIFIERS_MIGRATE_EXCEPTION.code
            )
            Result.Failure(e)
        }
    }

    @Suppress("ComplexMethod", "TooGenericExceptionCaught", "ReturnCount")
    @Suppress("ComplexMethod", "TooGenericExceptionCaught", "ReturnCount", "NestedBlockDepth")
    private fun migratePinnedSites(): Result<Unit> {
        checkNotNull(bookmarksStorage) { "Bookmarks storage must be configured to migrate pinned sites" }
        checkNotNull(topSiteStorage) { "TopSiteStorage must be configured to migrate pinned sites" }
@@ -1248,6 +1296,7 @@ class FennecMigrator private constructor(
            MigrationPinnedSites.detectedPinnedSites.add(importedPinnedSites.size)
        }

        return try {
            // We can't import pinned sites that do not have a url.
            val pinnedSitesWithUrl = importedPinnedSites.filter { it.url != null }
            var failedToImport = importedPinnedSites.size - pinnedSitesWithUrl.size
@@ -1271,6 +1320,15 @@ class FennecMigrator private constructor(
            MigrationPinnedSites.migratedPinnedSites.add(importedPinnedSites.size - failedToImport)
            MigrationPinnedSites.successReason.add(SuccessReasonTelemetryCodes.PINNED_SITES_MIGRATED.code)

        return Result.Success(Unit)
            Result.Success(Unit)
        } catch (e: Exception) {
            crashReporter.submitCaughtException(
                FennecMigratorException.MigratePinnedSitesException(e)
            )
            MigrationPinnedSites.failureReason.add(
                FailureReasonTelemetryCodes.PINNED_SITES_EXCEPTION.code
            )
            Result.Failure(e)
        }
    }
}
+5 −1
Original line number Diff line number Diff line
@@ -116,7 +116,11 @@ internal enum class FailureReasonTelemetryCodes(val code: Int) {

    GECKO_FAILED_TO_DELETE_PREFS(36),
    GECKO_FAILED_TO_WRITE_BACKUP(37),
    GECKO_FAILED_TO_WRITE_PREFS(38)
    GECKO_FAILED_TO_WRITE_PREFS(38),

    FXA_MIGRATE_EXCEPTION(39),
    TELEMETRY_IDENTIFIERS_PARSE_SET_EXCEPTION(40),
    PINNED_SITES_EXCEPTION(41),
}

@SuppressWarnings("MagicNumber")
+8 −0
Original line number Diff line number Diff line
@@ -42,6 +42,7 @@ import mozilla.components.support.base.crash.CrashReporting
import mozilla.components.support.test.argumentCaptor
import mozilla.components.support.test.whenever
import mozilla.components.support.test.eq
import org.json.JSONObject
import org.mockito.Mockito.never
import org.mockito.Mockito.reset
import java.lang.IllegalArgumentException
@@ -373,6 +374,13 @@ class FennecMigratorTest {
        // Fail during history migration.
        `when`(historyStorage.importFromFennec(any())).thenThrow(PlacesException("test exception"))

        `when`(bookmarkStorage.importFromFennec(any())).thenReturn(JSONObject().also {
            it.put("num_total", 25)
            it.put("num_succeeded", 23)
            it.put("num_failed", 2)
            it.put("total_duration", 1245L)
        })

        // DB path is configured, partial success (only history failed).
        val migrator = FennecMigrator.Builder(testContext, mock())
            .migrateHistory(lazy { historyStorage })