Merge mozilla-mobile#5751

5751: update/subscribe push race workaround r=jonalmeida a=grigoryk



Co-authored-by: Grisha Kruglov <[email protected]>

Author: MozLando

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

@@ -99,8 +99,7 @@ class AutoPushFeature(
         get() = preferences(context).getLong(LAST_VERIFIED, System.currentTimeMillis())
         set(value) = preferences(context).edit().putLong(LAST_VERIFIED, value).apply()
 
-    internal var job: Job = SupervisorJob()
-    private val scope = CoroutineScope(coroutineContext) + job
+    private val scope = CoroutineScope(coroutineContext) + SupervisorJob()
 
     init {
         // If we have a token, initialize the rust component first.
@@ -132,9 +131,8 @@ class AutoPushFeature(
         service.stop()
 
         DeliveryManager.with(connection) {
-            job = scope.launch {
+            scope.launch {
                 unsubscribeAll()
-                job.cancel()
             }
         }
 
@@ -147,7 +145,7 @@ class AutoPushFeature(
      * each push type and notifies the subscribers.
      */
     override fun onNewToken(newToken: String) {
-        job = scope.launchAndTry {
+        scope.launchAndTry {
             logger.info("Received a new registration token from push service.")
 
             connection.updateToken(newToken)
@@ -173,7 +171,7 @@ class AutoPushFeature(
             logger.error("Unknown channelID: ${message.channelId}")
             return
         }
-        job = scope.launchAndTry {
+        scope.launchAndTry {
             DeliveryManager.with(connection) {
                 logger.info("New push message decrypted.")
                 val decrypted = decrypt(
@@ -229,7 +227,7 @@ class AutoPushFeature(
      */
     fun subscribeForType(type: PushType) {
         DeliveryManager.with(connection) {
-            job = scope.launchAndTry {
+            scope.launchAndTry {
                 val sub = subscribe(type.toChannelId()).toPushSubscription()
                 subscriptionObservers.notifyObservers { onSubscriptionAvailable(sub) }
             }
@@ -246,7 +244,7 @@ class AutoPushFeature(
      */
     fun unsubscribeForType(type: PushType) {
         DeliveryManager.with(connection) {
-            job = scope.launchAndTry {
+            scope.launchAndTry {
                 unsubscribe(type.toChannelId())
             }
         }
@@ -257,7 +255,7 @@ class AutoPushFeature(
      */
     fun subscribeAll() {
         DeliveryManager.with(connection) {
-            job = scope.launchAndTry {
+            scope.launchAndTry {
                 PushType.values().forEach { type ->
                     val sub = subscribe(type.toChannelId()).toPushSubscription()
                     subscriptionObservers.notifyObservers { onSubscriptionAvailable(sub) }
@@ -290,7 +288,7 @@ class AutoPushFeature(
     @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
     internal fun verifyActiveSubscriptions() {
         DeliveryManager.with(connection) {
-            job = scope.launchAndTry {
+            scope.launchAndTry {
                 val notifyObservers = verifyConnection()
 
                 if (notifyObservers) {

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

@@ -8,10 +8,12 @@ import androidx.annotation.GuardedBy
 import androidx.annotation.VisibleForTesting
 import mozilla.appservices.push.BridgeType
 import mozilla.appservices.push.PushAPI
+import mozilla.appservices.push.PushError
 import mozilla.appservices.push.PushManager
 import mozilla.appservices.push.SubscriptionResponse
 import java.io.Closeable
 import java.util.Locale
+import java.util.UUID
 
 /**
  * An interface that wraps the [PushAPI].
@@ -81,7 +83,23 @@ internal class RustPushConnection(
             )
             return true
         }
-        return pushApi.update(token)
+        // This call will fail if we haven't 'subscribed' yet.
+        return try {
+            pushApi.update(token)
+        } catch (e: PushError) {
+            // Once we get GeneralError, let's catch that instead:
+            // https://github.com/mozilla/application-services/issues/2541
+            val fakeChannelId = UUID.nameUUIDFromBytes("fake".toByteArray()).toString().replace("-", "")
+            // It's possible that we have a race (on a first run) between 'subscribing' and setting a token.
+            // 'update' expects that we've called 'subscribe' (which would obtain a 'uaid' from an autopush
+            // server), which we need to have in order to call 'update' on the library.
+            // In https://github.com/mozilla/application-services/issues/2490 this will be fixed, and we
+            // can clean up this work-around.
+            pushApi.subscribe(fakeChannelId)
+
+            // If this fails again, give up - it seems like a legit failure that we should re-throw.
+            pushApi.update(token)
+        }
     }
 
     @GuardedBy("this")

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

@@ -30,7 +30,6 @@ import mozilla.components.support.test.whenever
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertNotNull
 import org.junit.Assert.assertNull
-import org.junit.Assert.assertTrue
 import org.junit.Before
 import org.junit.Test
 import org.junit.runner.RunWith
@@ -98,13 +97,12 @@ class AutoPushFeatureTest {
         val connection: PushConnection = mock()
         whenever(connection.isInitialized()).thenReturn(true)
 
-        val feature = spy(AutoPushFeature(testContext, service, mock(), coroutineContext, connection)).also {
+        AutoPushFeature(testContext, service, mock(), coroutineContext, connection).also {
             it.shutdown()
         }
 
         verify(service).stop()
         verify(connection).unsubscribeAll()
-        assertTrue(feature.job.isCancelled)
     }
 
     @Test

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

@@ -6,6 +6,7 @@ package mozilla.components.feature.push
 
 import kotlinx.coroutines.runBlocking
 import mozilla.appservices.push.PushAPI
+import mozilla.components.support.test.any
 import mozilla.components.support.test.eq
 import mozilla.components.support.test.mock
 import org.junit.Assert.assertFalse
@@ -16,6 +17,7 @@ import org.junit.Ignore
 import org.junit.Test
 import org.mockito.ArgumentMatchers.nullable
 import org.mockito.Mockito.anyString
+import org.mockito.Mockito.never
 import org.mockito.Mockito.verify
 
 class RustPushConnectionTest {
@@ -44,6 +46,7 @@ class RustPushConnectionTest {
             connection.updateToken("123")
         }
 
+        verify(api, never()).subscribe(any(), any(), any())
         verify(api).update(anyString())
     }