From a04a82f84b7fd688e00594683d7d7a193f78aa46 Mon Sep 17 00:00:00 2001 From: Jonathan Almeida Date: Fri, 29 May 2020 03:27:49 -0400 Subject: [PATCH 1/2] Issue #7164: Add getSubscription to AutoPushFeature if one exists We added the ability to check if a subscription exists first to match the API requirement for WebPush support in GeckoView. We do this because the call to `getSubscription` on the engine side expects it to work as a check before creating a subscription, where-as our implementation used a "get or create" pattern instead. The fallout from this is that visiting sites that check for subscriptions were immediately given one without user permission. --- .../feature/push/AutoPushFeature.kt | 26 ++++++ .../components/feature/push/Connection.kt | 13 +++ .../feature/push/AutoPushFeatureTest.kt | 90 ++++++++++++------- .../feature/push/RustPushConnectionTest.kt | 18 ++++ docs/changelog.md | 3 + 5 files changed, 120 insertions(+), 30 deletions(-) diff --git a/components/feature/push/src/main/java/mozilla/components/feature/push/AutoPushFeature.kt b/components/feature/push/src/main/java/mozilla/components/feature/push/AutoPushFeature.kt index a324389e76c..48e59dbe115 100644 --- a/components/feature/push/src/main/java/mozilla/components/feature/push/AutoPushFeature.kt +++ b/components/feature/push/src/main/java/mozilla/components/feature/push/AutoPushFeature.kt @@ -230,6 +230,32 @@ class AutoPushFeature( } } + /** + * Checks if a subscription for the [scope] already exists. + * + * @param scope The subscription identifier which usually represents the website's URI. + * @param appServerKey An optional key provided by the application server. + * @param block The callback invoked when a subscription for the [scope] is found, otherwise null. Note: this will + * not execute on the calls thread. + */ + fun getSubscription( + scope: String, + appServerKey: String? = null, + block: (AutoPushSubscription?) -> Unit + ) { + connection.ifInitialized { + coroutineScope.launchAndTry { + if (containsSubscription(scope)) { + // If we have a subscription, calling subscribe will give us the existing subscription. + // We do this because we do not have API symmetry across the different layers in this stack. + subscribe(scope, appServerKey, {}, block) + } else { + block(null) + } + } + } + } + /** * Deletes the registration token locally so that it forces the service to get a new one the * next time hits it's messaging server. diff --git a/components/feature/push/src/main/java/mozilla/components/feature/push/Connection.kt b/components/feature/push/src/main/java/mozilla/components/feature/push/Connection.kt index a20f5c7a819..756004e3fc6 100644 --- a/components/feature/push/src/main/java/mozilla/components/feature/push/Connection.kt +++ b/components/feature/push/src/main/java/mozilla/components/feature/push/Connection.kt @@ -47,6 +47,11 @@ interface PushConnection : Closeable { */ suspend fun unsubscribeAll(): Boolean + /** + * Returns `true` if the specified [scope] has a subscription. + */ + suspend fun containsSubscription(scope: PushScope): Boolean + /** * Updates the registration token to the native Push API if it changes. * @@ -154,6 +159,14 @@ internal class RustPushConnection( return pushApi.unsubscribeAll() } + @GuardedBy("this") + override suspend fun containsSubscription(scope: PushScope): Boolean = synchronized(this) { + val pushApi = api + check(pushApi != null) { "Rust API is not initiated; updateToken hasn't been called yet." } + + return pushApi.dispatchInfoForChid(scope.toChannelId()) != null + } + @GuardedBy("this") override suspend fun updateToken(token: String): Boolean = synchronized(this) { val pushApi = api diff --git a/components/feature/push/src/test/java/mozilla/components/feature/push/AutoPushFeatureTest.kt b/components/feature/push/src/test/java/mozilla/components/feature/push/AutoPushFeatureTest.kt index 4c8d744ec8c..894e8cb32a1 100644 --- a/components/feature/push/src/test/java/mozilla/components/feature/push/AutoPushFeatureTest.kt +++ b/components/feature/push/src/test/java/mozilla/components/feature/push/AutoPushFeatureTest.kt @@ -63,7 +63,7 @@ class AutoPushFeatureTest { fun `initialize starts push service`() { val service: PushService = mock() val config = PushConfig("push-test") - val feature = spy(AutoPushFeature(testContext, service, config)) + val feature = AutoPushFeature(testContext, service, config) feature.initialize() @@ -96,7 +96,7 @@ class AutoPushFeatureTest { val service: PushService = mock() whenever(connection.isInitialized()).thenReturn(true) - spy(AutoPushFeature(testContext, service, mock(), coroutineContext, connection)).also { + AutoPushFeature(testContext, service, mock(), coroutineContext, connection).also { it.shutdown() } @@ -105,7 +105,7 @@ class AutoPushFeatureTest { @Test fun `onNewToken updates connection and saves pref`() = runBlockingTest { - val feature = spy(AutoPushFeature(testContext, mock(), mock(), coroutineContext, connection)) + val feature = AutoPushFeature(testContext, mock(), mock(), coroutineContext, connection) whenever(connection.subscribe(anyString(), nullable())).thenReturn(mock()) @@ -143,7 +143,7 @@ class AutoPushFeatureTest { .thenReturn(null) // If we get null, we shouldn't notify observers. .thenReturn(DecryptedMessage("testScope", "test".toByteArray())) - val feature = spy(AutoPushFeature(testContext, mock(), mock(), coroutineContext, connection)) + val feature = AutoPushFeature(testContext, mock(), mock(), coroutineContext, connection) feature.register(observer) @@ -161,7 +161,7 @@ class AutoPushFeatureTest { val connection: PushConnection = mock() val subscription: AutoPushSubscription = mock() var invoked = false - val feature = spy(AutoPushFeature(testContext, mock(), mock(), coroutineContext, connection)) + val feature = AutoPushFeature(testContext, mock(), mock(), coroutineContext, connection) feature.subscribe("testScope") { invoked = true @@ -186,7 +186,7 @@ class AutoPushFeatureTest { val subscription: AutoPushSubscription = mock() var invoked = false var errorInvoked = false - val feature = spy(AutoPushFeature(testContext, mock(), mock(), coroutineContext, connection)) + val feature = AutoPushFeature(testContext, mock(), mock(), coroutineContext, connection) feature.subscribe( scope = "testScope", @@ -223,7 +223,7 @@ class AutoPushFeatureTest { var invoked = false var errorInvoked = false - val feature = spy(AutoPushFeature(testContext, mock(), mock(), coroutineContext, connection)) + val feature = AutoPushFeature(testContext, mock(), mock(), coroutineContext, connection) feature.unsubscribe( scope = "testScope", @@ -292,10 +292,43 @@ class AutoPushFeatureTest { assertTrue(errorInvoked) } + @Test + fun `getSubscription returns null when there is no subscription`() = runBlockingTest { + val feature = AutoPushFeature(testContext, mock(), mock(), coroutineContext, connection) + var invoked = false + + whenever(connection.containsSubscription(anyString())).thenReturn(true) + + feature.getSubscription( + scope = "testScope", + appServerKey = null + ) { + invoked = it == null + } + + assertTrue(invoked) + } + + @Test + fun `getSubscription invokes subscribe when there is a subscription`() = runBlockingTest { + val connection = TestPushConnection(true) + val feature = AutoPushFeature(testContext, mock(), mock(), coroutineContext, connection) + var invoked = false + + feature.getSubscription( + scope = "testScope", + appServerKey = null + ) { + invoked = it != null + } + + assertTrue(invoked) + } + @Test fun `forceRegistrationRenewal deletes pref and calls service`() = runBlockingTest { val service: PushService = mock() - val feature = spy(AutoPushFeature(testContext, service, mock(), coroutineContext, mock())) + val feature = AutoPushFeature(testContext, service, mock(), coroutineContext, mock()) feature.renewRegistration() @@ -312,7 +345,7 @@ class AutoPushFeatureTest { val owner: LifecycleOwner = mock() val lifecycle: Lifecycle = mock() val observers: AutoPushFeature.Observer = mock() - val feature = spy(AutoPushFeature(testContext, mock(), mock(), coroutineContext, connection)) + val feature = AutoPushFeature(testContext, mock(), mock(), coroutineContext, connection) whenever(owner.lifecycle).thenReturn(lifecycle) whenever(lifecycle.currentState).thenReturn(Lifecycle.State.STARTED) @@ -378,8 +411,7 @@ class AutoPushFeatureTest { fun `crash reporter is notified of errors`() = runBlockingTest { val native: PushConnection = TestPushConnection(true) val crashReporter: CrashReporting = mock() - val feature = spy( - AutoPushFeature( + val feature = AutoPushFeature( context = testContext, service = mock(), config = mock(), @@ -387,7 +419,7 @@ class AutoPushFeatureTest { connection = native, crashReporter = crashReporter ) - ) + feature.onError(PushError.Rust(PushError.MalformedMessage("Bad things happened!"))) verify(crashReporter).submitCaughtException(any()) @@ -396,15 +428,13 @@ class AutoPushFeatureTest { @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 - ) + val feature = AutoPushFeature( + context = testContext, + service = mock(), + config = mock(), + coroutineContext = coroutineContext, + connection = connection, + crashReporter = crashReporter ) whenever(connection.unsubscribe(any())).thenAnswer { throw GeneralError("test") } @@ -417,15 +447,13 @@ class AutoPushFeatureTest { @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 - ) + val feature = AutoPushFeature( + context = testContext, + service = mock(), + config = mock(), + coroutineContext = coroutineContext, + connection = connection, + crashReporter = crashReporter ) whenever(connection.unsubscribe(any())).thenAnswer { throw MissingRegistrationTokenError() } @@ -457,6 +485,8 @@ class AutoPushFeatureTest { override suspend fun unsubscribeAll(): Boolean = true + override suspend fun containsSubscription(scope: PushScope) = true + override suspend fun updateToken(token: String) = true override suspend fun verifyConnection(): List = emptyList() diff --git a/components/feature/push/src/test/java/mozilla/components/feature/push/RustPushConnectionTest.kt b/components/feature/push/src/test/java/mozilla/components/feature/push/RustPushConnectionTest.kt index 24936ebe7e2..60b47e6792d 100644 --- a/components/feature/push/src/test/java/mozilla/components/feature/push/RustPushConnectionTest.kt +++ b/components/feature/push/src/test/java/mozilla/components/feature/push/RustPushConnectionTest.kt @@ -21,6 +21,7 @@ import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Ignore import org.junit.Test +import org.mockito.ArgumentMatchers import org.mockito.Mockito.`when` import org.mockito.Mockito.anyString import org.mockito.Mockito.never @@ -140,6 +141,23 @@ class RustPushConnectionTest { verify(api).unsubscribeAll() } + @Test + fun `containsSubscription returns true if a subscription exists`() { + val connection = createConnection() + val api: PushAPI = mock() + connection.api = api + + `when`(api.dispatchInfoForChid(ArgumentMatchers.anyString())) + .thenReturn(mock()) + .thenReturn(null) + + runBlocking { + assertTrue(connection.containsSubscription("validSubscription")) + + assertFalse(connection.containsSubscription("invalidSubscription")) + } + } + @Test(expected = IllegalStateException::class) fun `verifyConnection throws if API is not initialized first`() { val connection = createConnection() diff --git a/docs/changelog.md b/docs/changelog.md index d04306c86f4..2f4ffe76ce6 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -24,6 +24,9 @@ permalink: /changelog/ * Adds `ThumbnailStorage` as a storage layer for handling saving and loading a thumbnail from the disk cache. +* **feature-push** + * Adds the `getSubscription` call to check if a subscription exists. + # 43.0.0 * [Commits](https://github.com/mozilla-mobile/android-components/compare/v42.0.0...v43.0.0) From aa4e35debf63a50ffff976c049f5aaccfb43291a Mon Sep 17 00:00:00 2001 From: Jonathan Almeida Date: Fri, 29 May 2020 03:29:12 -0400 Subject: [PATCH 2/2] Issue #7164: Do not complete WebPush responses exceptionally With the previous patch, we fixed a bug where we need to check if a subscription exists. This new (correct) behaviour broke our original implementation of how to handle the null case in the engine. --- .../engine/gecko/webpush/GeckoWebPushDelegate.kt | 12 ++---------- .../engine/gecko/webpush/GeckoWebPushDelegateTest.kt | 10 ++++------ .../engine/gecko/webpush/GeckoWebPushDelegate.kt | 12 ++---------- .../engine/gecko/webpush/GeckoWebPushDelegateTest.kt | 10 ++++------ .../engine/gecko/webpush/GeckoWebPushDelegate.kt | 12 ++---------- .../engine/gecko/webpush/GeckoWebPushDelegateTest.kt | 10 ++++------ docs/changelog.md | 3 +++ 7 files changed, 21 insertions(+), 48 deletions(-) diff --git a/components/browser/engine-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/webpush/GeckoWebPushDelegate.kt b/components/browser/engine-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/webpush/GeckoWebPushDelegate.kt index e421f844eb7..3facfc2de16 100644 --- a/components/browser/engine-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/webpush/GeckoWebPushDelegate.kt +++ b/components/browser/engine-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/webpush/GeckoWebPushDelegate.kt @@ -22,11 +22,7 @@ internal class GeckoWebPushDelegate(private val delegate: WebPushDelegate) : Gec val result: GeckoResult = GeckoResult() delegate.onGetSubscription(scope) { subscription -> - if (subscription != null) { - result.complete(subscription.toGeckoSubscription()) - } else { - result.completeExceptionally(WebPushException("Retrieving subscription failed.")) - } + result.complete(subscription?.toGeckoSubscription()) } return result @@ -39,11 +35,7 @@ internal class GeckoWebPushDelegate(private val delegate: WebPushDelegate) : Gec val result: GeckoResult = GeckoResult() delegate.onSubscribe(scope, appServerKey) { subscription -> - if (subscription != null) { - result.complete(subscription.toGeckoSubscription()) - } else { - result.completeExceptionally(WebPushException("Creating subscription failed.")) - } + result.complete(subscription?.toGeckoSubscription()) } return result diff --git a/components/browser/engine-gecko-beta/src/test/java/mozilla/components/browser/engine/gecko/webpush/GeckoWebPushDelegateTest.kt b/components/browser/engine-gecko-beta/src/test/java/mozilla/components/browser/engine/gecko/webpush/GeckoWebPushDelegateTest.kt index c9494fdf75e..446e9184e80 100644 --- a/components/browser/engine-gecko-beta/src/test/java/mozilla/components/browser/engine/gecko/webpush/GeckoWebPushDelegateTest.kt +++ b/components/browser/engine-gecko-beta/src/test/java/mozilla/components/browser/engine/gecko/webpush/GeckoWebPushDelegateTest.kt @@ -72,9 +72,8 @@ class GeckoWebPushDelegateTest { val nullResult = geckoDelegate.onGetSubscription("test") - nullResult?.exceptionally { throwable: Throwable -> - assertTrue(throwable.localizedMessage == "Retrieving subscription failed.") - GeckoResult.fromValue(null) + nullResult?.accept { sub -> + assertNull(sub) } } @@ -108,9 +107,8 @@ class GeckoWebPushDelegateTest { subscription = null val nullResult = geckoDelegate.onSubscribe("test", null) - nullResult?.exceptionally { throwable: Throwable -> - assertTrue(throwable.localizedMessage == "Creating subscription failed.") - GeckoResult.fromValue(null) + nullResult?.accept { sub -> + assertNull(sub) } } diff --git a/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/webpush/GeckoWebPushDelegate.kt b/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/webpush/GeckoWebPushDelegate.kt index e421f844eb7..3facfc2de16 100644 --- a/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/webpush/GeckoWebPushDelegate.kt +++ b/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/webpush/GeckoWebPushDelegate.kt @@ -22,11 +22,7 @@ internal class GeckoWebPushDelegate(private val delegate: WebPushDelegate) : Gec val result: GeckoResult = GeckoResult() delegate.onGetSubscription(scope) { subscription -> - if (subscription != null) { - result.complete(subscription.toGeckoSubscription()) - } else { - result.completeExceptionally(WebPushException("Retrieving subscription failed.")) - } + result.complete(subscription?.toGeckoSubscription()) } return result @@ -39,11 +35,7 @@ internal class GeckoWebPushDelegate(private val delegate: WebPushDelegate) : Gec val result: GeckoResult = GeckoResult() delegate.onSubscribe(scope, appServerKey) { subscription -> - if (subscription != null) { - result.complete(subscription.toGeckoSubscription()) - } else { - result.completeExceptionally(WebPushException("Creating subscription failed.")) - } + result.complete(subscription?.toGeckoSubscription()) } return result diff --git a/components/browser/engine-gecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/webpush/GeckoWebPushDelegateTest.kt b/components/browser/engine-gecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/webpush/GeckoWebPushDelegateTest.kt index c9494fdf75e..446e9184e80 100644 --- a/components/browser/engine-gecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/webpush/GeckoWebPushDelegateTest.kt +++ b/components/browser/engine-gecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/webpush/GeckoWebPushDelegateTest.kt @@ -72,9 +72,8 @@ class GeckoWebPushDelegateTest { val nullResult = geckoDelegate.onGetSubscription("test") - nullResult?.exceptionally { throwable: Throwable -> - assertTrue(throwable.localizedMessage == "Retrieving subscription failed.") - GeckoResult.fromValue(null) + nullResult?.accept { sub -> + assertNull(sub) } } @@ -108,9 +107,8 @@ class GeckoWebPushDelegateTest { subscription = null val nullResult = geckoDelegate.onSubscribe("test", null) - nullResult?.exceptionally { throwable: Throwable -> - assertTrue(throwable.localizedMessage == "Creating subscription failed.") - GeckoResult.fromValue(null) + nullResult?.accept { sub -> + assertNull(sub) } } diff --git a/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/webpush/GeckoWebPushDelegate.kt b/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/webpush/GeckoWebPushDelegate.kt index e421f844eb7..3facfc2de16 100644 --- a/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/webpush/GeckoWebPushDelegate.kt +++ b/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/webpush/GeckoWebPushDelegate.kt @@ -22,11 +22,7 @@ internal class GeckoWebPushDelegate(private val delegate: WebPushDelegate) : Gec val result: GeckoResult = GeckoResult() delegate.onGetSubscription(scope) { subscription -> - if (subscription != null) { - result.complete(subscription.toGeckoSubscription()) - } else { - result.completeExceptionally(WebPushException("Retrieving subscription failed.")) - } + result.complete(subscription?.toGeckoSubscription()) } return result @@ -39,11 +35,7 @@ internal class GeckoWebPushDelegate(private val delegate: WebPushDelegate) : Gec val result: GeckoResult = GeckoResult() delegate.onSubscribe(scope, appServerKey) { subscription -> - if (subscription != null) { - result.complete(subscription.toGeckoSubscription()) - } else { - result.completeExceptionally(WebPushException("Creating subscription failed.")) - } + result.complete(subscription?.toGeckoSubscription()) } return result diff --git a/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/webpush/GeckoWebPushDelegateTest.kt b/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/webpush/GeckoWebPushDelegateTest.kt index c9494fdf75e..446e9184e80 100644 --- a/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/webpush/GeckoWebPushDelegateTest.kt +++ b/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/webpush/GeckoWebPushDelegateTest.kt @@ -72,9 +72,8 @@ class GeckoWebPushDelegateTest { val nullResult = geckoDelegate.onGetSubscription("test") - nullResult?.exceptionally { throwable: Throwable -> - assertTrue(throwable.localizedMessage == "Retrieving subscription failed.") - GeckoResult.fromValue(null) + nullResult?.accept { sub -> + assertNull(sub) } } @@ -108,9 +107,8 @@ class GeckoWebPushDelegateTest { subscription = null val nullResult = geckoDelegate.onSubscribe("test", null) - nullResult?.exceptionally { throwable: Throwable -> - assertTrue(throwable.localizedMessage == "Creating subscription failed.") - GeckoResult.fromValue(null) + nullResult?.accept { sub -> + assertNull(sub) } } diff --git a/docs/changelog.md b/docs/changelog.md index 2f4ffe76ce6..76a042d86de 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -27,6 +27,9 @@ permalink: /changelog/ * **feature-push** * Adds the `getSubscription` call to check if a subscription exists. +* **browser-engine-gecko-*** + * Fixes GeckoWebPushDelegate to gracefully return when a subscription is not available. + # 43.0.0 * [Commits](https://github.com/mozilla-mobile/android-components/compare/v42.0.0...v43.0.0)