Commit d74e20ae authored by MozLando's avatar MozLando
Browse files

Merge #5083



5083: Closes #5063: better exception reporting during migrations r=pocmo a=grigoryk

Patch 1:
>
    We are logging exceptions when no profile is found, which is actually a legit case.
    There will be no profile on a fresh install, for example.
    
    This patch makes these kinds of exceptions more precise/useful: we'll only send a report
    in case there's a profile but a missing dbPath. We'll now also send an exception in case
    of an IOException while parsing profiles.

Patch 2:
>
    Submit unexpected exceptions during migrations
Co-authored-by: default avatarGrisha Kruglov <gkruglov@mozilla.com>
parents 98092490 d119e81a
......@@ -67,6 +67,37 @@ data class VersionedMigration(val migration: Migration, val version: Int = migra
}
}
/**
* Exceptions related to Fennec migrations.
*
* See https://github.com/mozilla-mobile/android-components/issues/5095 for stripping any possible PII from these
* exceptions.
*/
sealed class FennecMigratorException(cause: Exception) : Exception(cause) {
/**
* Unexpected exception while migrating history.
* @param cause Original exception which caused the problem.
*/
class MigrateHistoryException(cause: Exception) : FennecMigratorException(cause)
/**
* Unexpected exception while migrating bookmarks.
* @param cause Original exception which caused the problem.
*/
class MigrateBookmarksException(cause: Exception) : FennecMigratorException(cause)
/**
* Unexpected exception while migrating open tabs.
* @param cause Original exception which caused the problem.
*/
class MigrateOpenTabsException(cause: Exception) : FennecMigratorException(cause)
/**
* Unexpected exception while migrating gecko profile.
* @param cause Original exception which caused the problem.
*/
class MigrateGeckoException(cause: Exception) : FennecMigratorException(cause)
}
/**
* Entrypoint for Fennec data migration. See [Builder] for public API.
*
......@@ -105,7 +136,7 @@ class FennecMigrator private constructor(
private var coroutineContext: CoroutineContext = Executors.newSingleThreadExecutor().asCoroutineDispatcher()
private var fxaState = File("${context.filesDir}", "fxa.account.json")
private var fennecProfile = FennecProfile.findDefault(context)
private var fennecProfile = FennecProfile.findDefault(context, crashReporter)
private var browserDbName = "browser.db"
/**
......@@ -306,8 +337,12 @@ class FennecMigrator private constructor(
private fun migrateHistory(): Result<Unit> {
checkNotNull(historyStorage) { "History storage must be configured to migrate history" }
if (dbPath == null) {
// There's no dbPath without a profile, but if a profile is present we expect dbPath to be also present.
if (profile != null && dbPath == null) {
crashReporter.submitCaughtException(IllegalStateException("Missing DB path during history migration"))
}
if (dbPath == null) {
return Result.Failure(IllegalStateException("Missing DB path during history migration"))
}
return try {
......@@ -316,6 +351,7 @@ class FennecMigrator private constructor(
logger.debug("Migrated history.")
Result.Success(Unit)
} catch (e: Exception) {
crashReporter.submitCaughtException(FennecMigratorException.MigrateHistoryException(e))
Result.Failure(e)
}
}
......@@ -324,8 +360,12 @@ class FennecMigrator private constructor(
private fun migrateBookmarks(): Result<Unit> {
checkNotNull(bookmarksStorage) { "Bookmarks storage must be configured to migrate bookmarks" }
if (dbPath == null) {
// There's no dbPath without a profile, but if a profile is present we expect dbPath to be also present.
if (profile != null && dbPath == null) {
crashReporter.submitCaughtException(IllegalStateException("Missing DB path during bookmark migration"))
}
if (dbPath == null) {
return Result.Failure(IllegalStateException("Missing DB path during bookmark migration"))
}
return try {
......@@ -334,6 +374,9 @@ class FennecMigrator private constructor(
logger.debug("Migrated bookmarks.")
Result.Success(Unit)
} catch (e: Exception) {
crashReporter.submitCaughtException(
FennecMigratorException.MigrateBookmarksException(e)
)
Result.Failure(e)
}
}
......@@ -354,6 +397,9 @@ class FennecMigrator private constructor(
}
result
} catch (e: Exception) {
crashReporter.submitCaughtException(
FennecMigratorException.MigrateOpenTabsException(e)
)
Result.Failure(e)
}
}
......@@ -381,7 +427,7 @@ class FennecMigrator private constructor(
}
val migrationSuccess = result as Result.Success<FxaMigrationResult>
return when (migrationSuccess.value as FxaMigrationResult.Success) {
return when (val success = migrationSuccess.value as FxaMigrationResult.Success) {
// The rest are all successful migrations.
is FxaMigrationResult.Success.NoAccount -> {
logger.debug("No Fennec account detected")
......@@ -391,7 +437,7 @@ class FennecMigrator private constructor(
// 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.
// 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: migrationResult")
logger.debug("Detected a Fennec account in a bad authentication state: ${success.stateLabel}")
result
}
is FxaMigrationResult.Success.SignedInIntoAuthenticatedAccount -> {
......@@ -404,7 +450,6 @@ class FennecMigrator private constructor(
@SuppressWarnings("TooGenericExceptionCaught")
private fun migrateGecko(): Result<Unit> {
if (profile == null) {
crashReporter.submitCaughtException(IllegalStateException("Missing Profile path"))
return Result.Failure(IllegalStateException("Missing Profile path"))
}
......@@ -414,6 +459,9 @@ class FennecMigrator private constructor(
logger.debug("Migrated gecko files.")
result
} catch (e: Exception) {
crashReporter.submitCaughtException(
FennecMigratorException.MigrateGeckoException(e)
)
Result.Failure(e)
}
}
......
......@@ -5,6 +5,7 @@
package mozilla.components.support.migration
import android.content.Context
import mozilla.components.lib.crash.CrashReporter
import mozilla.components.support.base.log.logger.Logger
import java.io.File
import java.io.IOException
......@@ -13,6 +14,16 @@ import java.util.regex.Pattern
private val logger = Logger("FennecProfile")
private val profilePattern = Pattern.compile("\\[(.+)]")
/**
* Exceptions related to Fennec profile migrations.
*/
sealed class FennecProfileException : Exception() {
/**
* IO exception while parsing profiles.
*/
class IOException : FennecProfileException()
}
/**
* A profile of "Fennec" (Firefox for Android).
*/
......@@ -33,10 +44,11 @@ data class FennecProfile(
*/
fun findDefault(
context: Context,
crashReporter: CrashReporter,
mozillaDirectory: File = getMozillaDirectory(context),
fileName: String = "profiles.ini"
): FennecProfile? {
return findDefaultProfile(mozillaDirectory, fileName)
return findDefaultProfile(crashReporter, mozillaDirectory, fileName)
}
}
}
......@@ -47,13 +59,16 @@ private fun getMozillaDirectory(context: Context): File {
@Suppress("ReturnCount")
private fun findDefaultProfile(
crashReporter: CrashReporter,
mozillaDirectory: File,
fileName: String
): FennecProfile? {
val profiles = try {
findProfiles(mozillaDirectory, fileName)
} catch (e: IOException) {
logger.debug("IOException when reading profile")
// We want to avoid either logging or exposing PII via the crashReporter, so we synthesize our own exception.
crashReporter.submitCaughtException(FennecProfileException.IOException())
logger.error("IOException when reading profile")
return null
}
......
......@@ -6,6 +6,8 @@ package mozilla.components.support.migration
import android.util.Log
import androidx.test.ext.junit.runners.AndroidJUnit4
import mozilla.components.lib.crash.CrashReporter
import mozilla.components.support.test.mock
import mozilla.components.support.test.robolectric.testContext
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
......@@ -14,92 +16,109 @@ import org.junit.Assert.assertNull
import org.junit.Assert.assertTrue
import org.junit.Test
import org.junit.runner.RunWith
import org.mockito.Mockito.verifyZeroInteractions
import java.io.File
@RunWith(AndroidJUnit4::class)
class FennecProfileTest {
@Test
fun `default fennec profile`() {
val crashReporter: CrashReporter = mock()
val profile = FennecProfile.findDefault(
testContext, getTestPath("profiles"), "fennec_default.txt")
testContext, crashReporter, getTestPath("profiles"), "fennec_default.txt")
assertNotNull(profile!!)
assertTrue(profile.default)
assertEquals("default", profile.name)
Log.w("SKDBG", profile.path)
assertTrue(profile.path.endsWith("/profiles/10aaayu4.default"))
verifyZeroInteractions(crashReporter)
}
@Test
fun `mozillazine default profile`() {
val crashReporter: CrashReporter = mock()
val profile = FennecProfile.findDefault(
testContext, getTestPath("profiles"), "mozillazine_default.txt")
testContext, crashReporter, getTestPath("profiles"), "mozillazine_default.txt")
assertNotNull(profile!!)
assertFalse(profile.default)
assertEquals("default", profile.name)
assertTrue(profile.path.endsWith("/profiles/Profiles/qioxtndq.default"))
verifyZeroInteractions(crashReporter)
}
@Test
fun `mozillazine multiple profiles`() {
val crashReporter: CrashReporter = mock()
val profile = FennecProfile.findDefault(
testContext, getTestPath("profiles"), "mozillazine_multiple.txt")
testContext, crashReporter, getTestPath("profiles"), "mozillazine_multiple.txt")
assertNotNull(profile!!)
assertTrue(profile.default)
assertEquals("alicew", profile.name)
assertEquals("D:\\Mozilla\\Firefox\\Profiles\\alicew", profile.path)
verifyZeroInteractions(crashReporter)
}
@Test
fun `desktop profiles`() {
val crashReporter: CrashReporter = mock()
val profile = FennecProfile.findDefault(
testContext, getTestPath("profiles"), "desktop.txt")
testContext, crashReporter, getTestPath("profiles"), "desktop.txt")
assertNotNull(profile!!)
assertTrue(profile.default)
assertEquals("default", profile.name)
assertTrue(profile.path.endsWith("/profiles/Profiles/xvcf5yup.default"))
verifyZeroInteractions(crashReporter)
}
@Test
fun `profiles-ini not existing in path`() {
val profile = FennecProfile.findDefault(testContext, getTestPath("profiles"))
val crashReporter: CrashReporter = mock()
val profile = FennecProfile.findDefault(testContext, crashReporter, getTestPath("profiles"))
assertNull(profile)
verifyZeroInteractions(crashReporter)
}
@Test
fun `with comments`() {
val crashReporter: CrashReporter = mock()
val profile = FennecProfile.findDefault(
testContext, getTestPath("profiles"), "with_comments.txt")
testContext, crashReporter, getTestPath("profiles"), "with_comments.txt")
assertNotNull(profile!!)
assertTrue(profile.default)
assertEquals("default", profile.name)
assertTrue(profile.path.endsWith("/profiles/10aaayu4.default"))
verifyZeroInteractions(crashReporter)
}
@Test
fun `weird broken`() {
val crashReporter: CrashReporter = mock()
val profile = FennecProfile.findDefault(
testContext, getTestPath("profiles"), "broken.txt")
testContext, crashReporter, getTestPath("profiles"), "broken.txt")
assertNotNull(profile!!)
assertEquals("Fennec", profile.name)
assertTrue(profile.default)
assertTrue(profile.path.endsWith("/profiles/fennec-default"))
verifyZeroInteractions(crashReporter)
}
@Test
fun `multiple profiles without default`() {
val crashReporter: CrashReporter = mock()
val profile = FennecProfile.findDefault(
testContext, getTestPath("profiles"), "no_default.txt")
testContext, crashReporter, getTestPath("profiles"), "no_default.txt")
assertNotNull(profile!!)
assertEquals("default", profile.name)
assertFalse(profile.default)
assertTrue(profile.path.endsWith("/profiles/Profiles/default"))
verifyZeroInteractions(crashReporter)
}
}
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment