diff --git a/components/feature/accounts-push/src/main/java/mozilla/components/feature/accounts/push/FxaPushSupportFeature.kt b/components/feature/accounts-push/src/main/java/mozilla/components/feature/accounts/push/FxaPushSupportFeature.kt index 712d9538dab..f8c7df3501a 100644 --- a/components/feature/accounts-push/src/main/java/mozilla/components/feature/accounts/push/FxaPushSupportFeature.kt +++ b/components/feature/accounts-push/src/main/java/mozilla/components/feature/accounts/push/FxaPushSupportFeature.kt @@ -2,6 +2,8 @@ * 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/. */ +@file:Suppress("LongParameterList") + package mozilla.components.feature.accounts.push import android.content.Context @@ -11,7 +13,8 @@ import androidx.lifecycle.ProcessLifecycleOwner import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.launch -import mozilla.components.concept.push.PushProcessor +import mozilla.components.concept.base.crash.Breadcrumb +import mozilla.components.concept.base.crash.CrashReporting import mozilla.components.concept.sync.AuthType import mozilla.components.concept.sync.ConstellationState import mozilla.components.concept.sync.Device @@ -42,6 +45,7 @@ internal const val PREF_FXA_SCOPE = "fxa_push_scope" * @param context The application Android context. * @param accountManager The FxaAccountManager. * @param pushFeature The [AutoPushFeature] if that is setup for observing push events. + * @param crashReporter Instance of `CrashReporting` to record unexpected caught exceptions. * @param owner the lifecycle owner for the observer. Defaults to [ProcessLifecycleOwner]. * @param autoPause whether to stop notifying the observer during onPause lifecycle events. * Defaults to false so that observers are always notified. @@ -50,6 +54,7 @@ class FxaPushSupportFeature( private val context: Context, accountManager: FxaAccountManager, pushFeature: AutoPushFeature, + private val crashReporter: CrashReporting? = null, owner: LifecycleOwner = ProcessLifecycleOwner.get(), autoPause: Boolean = false ) { @@ -85,6 +90,7 @@ class FxaPushSupportFeature( context, pushFeature, fxaPushScope, + crashReporter, owner, autoPause ) @@ -107,25 +113,42 @@ internal class AccountObserver( private val context: Context, private val push: AutoPushFeature, private val fxaPushScope: String, + private val crashReporter: CrashReporting?, private val lifecycleOwner: LifecycleOwner, private val autoPause: Boolean ) : SyncAccountObserver { private val logger = Logger(AccountObserver::class.java.simpleName) private val verificationDelegate = VerificationDelegate(context, push.config.disableRateLimit) - private val constellationObserver = ConstellationObserver(push, verificationDelegate) override fun onAuthenticated(account: OAuthAccount, authType: AuthType) { + + val constellationObserver = ConstellationObserver( + context = context, + push = push, + scope = fxaPushScope, + account = account, + verifier = verificationDelegate, + crashReporter = crashReporter + ) + // We need a new subscription only when we have a new account. // The subscription is removed when an account logs out. if (authType != AuthType.Existing && authType != AuthType.Recovered) { logger.debug("Subscribing for FxaPushScope ($fxaPushScope) events.") - push.subscribe(fxaPushScope) { subscription -> - CoroutineScope(Dispatchers.Main).launch { - account.deviceConstellation().setDevicePushSubscription(subscription.into()) - } - } + push.subscribe( + scope = fxaPushScope, + onSubscribeError = { e -> + crashReporter?.recordCrashBreadcrumb(Breadcrumb("Subscribing to FxA push failed at login.")) + logger.info("Subscribing to FxA push failed at login.", e) + }, + onSubscribe = { subscription -> + logger.info("Created a new subscription: $subscription") + CoroutineScope(Dispatchers.Main).launch { + account.deviceConstellation().setDevicePushSubscription(subscription.into()) + } + }) } // NB: can we just expose registerDeviceObserver on account manager? @@ -152,8 +175,12 @@ internal class AccountObserver( * when notified by the FxA server. See [Device.subscriptionExpired]. */ internal class ConstellationObserver( - private val push: PushProcessor, - private val verifier: VerificationDelegate + context: Context, + private val push: AutoPushFeature, + private val scope: String, + private val account: OAuthAccount, + private val verifier: VerificationDelegate = VerificationDelegate(context), + private val crashReporter: CrashReporting? ) : DeviceConstellationObserver { private val logger = Logger(ConstellationObserver::class.java.simpleName) @@ -166,21 +193,59 @@ internal class ConstellationObserver( // If our last check was recent (see: PERIODIC_INTERVAL_MILLISECONDS), we do nothing. val allowedToRenew = verifier.allowedToRenew() if (!updateSubscription || !allowedToRenew) { - logger.info("Short-circuiting onDevicesUpdate: " + - "updateSubscription($updateSubscription), allowedToRenew($allowedToRenew)") + logger.info( + "Short-circuiting onDevicesUpdate: " + + "updateSubscription($updateSubscription), allowedToRenew($allowedToRenew)" + ) return } else { logger.info("Proceeding to renew registration") } - logger.info("Renewing registration") - push.renewRegistration() + logger.info("We have been notified that our push subscription has expired; re-subscribing.") + push.subscribe( + scope = scope, + onSubscribeError = ::onSubscribeError, + onSubscribe = { onSubscribe(constellation, it) } + ) logger.info("Incrementing verifier") logger.info("Verifier state before: timestamp=${verifier.innerTimestamp}, count=${verifier.innerCount}") verifier.increment() logger.info("Verifier state after: timestamp=${verifier.innerTimestamp}, count=${verifier.innerCount}") } + + internal fun onSubscribe(constellation: ConstellationState, subscription: AutoPushSubscription) { + + logger.info("Created a new subscription: $subscription") + + val oldEndpoint = constellation.currentDevice?.subscription?.endpoint + if (subscription.endpoint == oldEndpoint) { + val exception = IllegalStateException( + "New push endpoint matches existing one", + Throwable("New endpoint: ${subscription.endpoint}\nOld endpoint: $oldEndpoint") + ) + + crashReporter?.submitCaughtException(exception) + + logger.warn("Push endpoints match!", exception) + } + + CoroutineScope(Dispatchers.Main).launch { + account.deviceConstellation().setDevicePushSubscription(subscription.into()) + } + } + + internal fun onSubscribeError(e: Exception) { + fun Exception.toBreadcrumb() = Breadcrumb( + message = "Re-subscribing to FxA push failed after subscriptionExpired", + data = mapOf( + "exception" to javaClass.name, + "message" to message.orEmpty() + ) + ) + crashReporter?.recordCrashBreadcrumb(e.toBreadcrumb()) + } } /** @@ -261,6 +326,7 @@ internal class VerificationDelegate( @VisibleForTesting internal var innerCount: Int = 0 + @VisibleForTesting internal var innerTimestamp: Long = System.currentTimeMillis() diff --git a/components/feature/accounts-push/src/test/java/mozilla/components/feature/accounts/push/AccountObserverTest.kt b/components/feature/accounts-push/src/test/java/mozilla/components/feature/accounts/push/AccountObserverTest.kt index c82c1347bef..3b01d76b78e 100644 --- a/components/feature/accounts-push/src/test/java/mozilla/components/feature/accounts/push/AccountObserverTest.kt +++ b/components/feature/accounts-push/src/test/java/mozilla/components/feature/accounts/push/AccountObserverTest.kt @@ -7,6 +7,7 @@ package mozilla.components.feature.accounts.push import androidx.lifecycle.Lifecycle import androidx.lifecycle.LifecycleOwner import kotlinx.coroutines.runBlocking +import mozilla.components.concept.base.crash.CrashReporting import mozilla.components.concept.sync.AuthType import mozilla.components.concept.sync.DeviceConstellation import mozilla.components.concept.sync.OAuthAccount @@ -42,6 +43,7 @@ class AccountObserverTest { private val account: OAuthAccount = mock() private val constellation: DeviceConstellation = mock() private val config: PushConfig = mock() + private val crashReporter: CrashReporting = mock() @Before fun setup() { @@ -58,6 +60,7 @@ class AccountObserverTest { testContext, pushFeature, pushScope, + crashReporter, lifecycleOwner, false ) @@ -81,6 +84,7 @@ class AccountObserverTest { testContext, pushFeature, pushScope, + crashReporter, mock(), false ) @@ -104,6 +108,7 @@ class AccountObserverTest { testContext, pushFeature, pushScope, + crashReporter, mock(), false ) @@ -123,6 +128,7 @@ class AccountObserverTest { testContext, pushFeature, pushScope, + crashReporter, mock(), false ) @@ -140,6 +146,7 @@ class AccountObserverTest { testContext, pushFeature, pushScope, + crashReporter, mock(), false ) @@ -161,6 +168,7 @@ class AccountObserverTest { testContext, pushFeature, pushScope, + crashReporter, mock(), false ) @@ -173,12 +181,32 @@ class AccountObserverTest { Unit } + @Test + fun `notify crash reporter on subscription error`() = runBlocking { + val observer = AccountObserver( + testContext, + pushFeature, + pushScope, + crashReporter, + mock(), + false + ) + + whenSubscribeError() + + observer.onAuthenticated(account, AuthType.Signin) + + verify(crashReporter).recordCrashBreadcrumb(any()) + Unit + } + @Test fun `feature and service invoked on logout`() { val observer = AccountObserver( testContext, pushFeature, pushScope, + crashReporter, mock(), false ) @@ -194,6 +222,7 @@ class AccountObserverTest { testContext, pushFeature, pushScope, + crashReporter, mock(), false ) @@ -221,4 +250,15 @@ class AccountObserverTest { ) } } + + @Suppress("UNCHECKED_CAST") + private fun whenSubscribeError(): OngoingStubbing? { + return `when`(pushFeature.subscribe(any(), nullable(), any(), any())).thenAnswer { + + // Invoke the `onSubscribe` lambda with a fake subscription. + (it.arguments[2] as ((Exception) -> Unit)).invoke( + IllegalStateException("test") + ) + } + } } \ No newline at end of file diff --git a/components/feature/accounts-push/src/test/java/mozilla/components/feature/accounts/push/ConstellationObserverTest.kt b/components/feature/accounts-push/src/test/java/mozilla/components/feature/accounts/push/ConstellationObserverTest.kt index 7cefcef13e0..d63a9be3d2e 100644 --- a/components/feature/accounts-push/src/test/java/mozilla/components/feature/accounts/push/ConstellationObserverTest.kt +++ b/components/feature/accounts-push/src/test/java/mozilla/components/feature/accounts/push/ConstellationObserverTest.kt @@ -7,10 +7,24 @@ package mozilla.components.feature.accounts.push import android.content.Context -import mozilla.components.concept.push.PushProcessor +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.test.TestCoroutineDispatcher +import kotlinx.coroutines.test.setMain +import mozilla.components.concept.base.crash.CrashReporting import mozilla.components.concept.sync.ConstellationState import mozilla.components.concept.sync.Device +import mozilla.components.concept.sync.DeviceConstellation +import mozilla.components.concept.sync.DevicePushSubscription +import mozilla.components.concept.sync.OAuthAccount +import mozilla.components.feature.push.AutoPushFeature +import mozilla.components.feature.push.AutoPushSubscription +import mozilla.components.support.test.any +import mozilla.components.support.test.eq import mozilla.components.support.test.mock +import mozilla.components.support.test.rule.MainCoroutineRule +import org.junit.Before +import org.junit.Ignore +import org.junit.Rule import org.junit.Test import org.mockito.Mockito.`when` import org.mockito.Mockito.verify @@ -18,15 +32,26 @@ import org.mockito.Mockito.verifyZeroInteractions class ConstellationObserverTest { - private val push: PushProcessor = mock() + private val push: AutoPushFeature = mock() private val verifier: VerificationDelegate = mock() private val state: ConstellationState = mock() private val device: Device = mock() private val context: Context = mock() + private val account: OAuthAccount = mock() + private val crashReporter: CrashReporting = mock() + private val testDispatcher = TestCoroutineDispatcher() + + @get:Rule + val coroutinesTestRule = MainCoroutineRule(testDispatcher) + + @Before + fun setup() { + Dispatchers.setMain(testDispatcher) + } @Test fun `do nothing if subscription has not expired`() { - val observer = ConstellationObserver(push, verifier) + val observer = ConstellationObserver(context, push, "testScope", account, verifier, crashReporter) observer.onDevicesUpdate(state) @@ -42,7 +67,7 @@ class ConstellationObserverTest { @Test fun `do nothing if verifier is false`() { - val observer = ConstellationObserver(push, verifier) + val observer = ConstellationObserver(context, push, "testScope", account, verifier, crashReporter) observer.onDevicesUpdate(state) @@ -62,8 +87,9 @@ class ConstellationObserverTest { } @Test + @Ignore("Disabling the test until we revert the changes from #8846 and fix #7143") fun `invoke registration renewal`() { - val observer = ConstellationObserver(push, verifier) + val observer = ConstellationObserver(context, push, "testScope", account, verifier, crashReporter) `when`(state.currentDevice).thenReturn(device) `when`(device.subscriptionExpired).thenReturn(true) @@ -74,4 +100,50 @@ class ConstellationObserverTest { verify(push).renewRegistration() verify(verifier).increment() } -} \ No newline at end of file + + /** + * Remove this test in the future. See [invoke registration renewal] test. + */ + @Test + fun `re-subscribe for push in onDevicesUpdate`() { + val observer = ConstellationObserver(context, push, "testScope", account, verifier, crashReporter) + + `when`(state.currentDevice).thenReturn(device) + `when`(device.subscriptionExpired).thenReturn(true) + `when`(verifier.allowedToRenew()).thenReturn(true) + + observer.onDevicesUpdate(state) + + verify(push).subscribe(eq("testScope"), any(), any(), any()) + verify(verifier).increment() + } + + @Test + fun `notify crash reporter if old and new subscription matches`() { + val observer = ConstellationObserver(context, push, "testScope", account, verifier, crashReporter) + val constellation: DeviceConstellation = mock() + val state: ConstellationState = mock() + val device: Device = mock() + val subscription: DevicePushSubscription = mock() + + `when`(account.deviceConstellation()).thenReturn(constellation) + `when`(state.currentDevice).thenReturn(device) + `when`(device.subscription).thenReturn(subscription) + `when`(subscription.endpoint).thenReturn("https://example.com") + + observer.onSubscribe(state, testSubscription()) + + verify(crashReporter).submitCaughtException(any()) + } + + @Test + fun `notify crash reporter if re-subscribe error occurs`() { + val observer = ConstellationObserver(context, push, "testScope", account, verifier, crashReporter) + + observer.onSubscribeError(mock()) + + verify(crashReporter).recordCrashBreadcrumb(any()) + } + + private fun testSubscription() = AutoPushSubscription(scope = "testScope", endpoint = "https://example.com", publicKey = "", authKey = "", appServerKey = null) +} diff --git a/docs/changelog.md b/docs/changelog.md index c5aa895d20a..2086271ff98 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -25,6 +25,10 @@ permalink: /changelog/ * 🚒 Bug fixed [issue #8967](https://github.com/mozilla-mobile/android-components/issues/8967) Crash when trying to upload a file see [fenix#16537](https://github.com/mozilla-mobile/fenix/issues/16537), for more information. * 🚒 Bug fixed [issue #8953](https://github.com/mozilla-mobile/android-components/issues/8953) - Scroll to selected prompt choice if one exists. +* **feature-accounts-push** + * ⚠️ `FxaPushSupportFeature` now re-subscribes to push instead of triggering the registration renewal process - this is a temporary workaround and will be removed in the future, see [#7143](https://github.com/mozilla-mobile/android-components/issues/7143). + * `FxaPushSupportFeature` now takes an optional crash reporter in the constructor. + # 66.0.0 * [Commits](https://github.com/mozilla-mobile/android-components/compare/v65.0.0...v66.0.0)