Close #9829: Move IO work in FxaPushSupportFeature to a coroutine

[jonalmeida]

The FxaPushSupportFeature needs to generate, read and write a push
scope (identifier) to disk. This work was meant to be done lazily and
not during app startup. However, if a Sync account is setup then the IO
work is done immediately during the initial account manager
initialization at startup.

In this patch, we delegate this IO work to a new PushScopeProperty
that retrieves the value from disk using a provided CoroutineScope.

Pull Request checklist

• Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
• Tests: This PR includes thorough tests or an explanation of why it does not
• Changelog: This PR includes a changelog entry or does not need one
• Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

• Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
• Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

GitHub Automation

Fixes #9829

[bendk]

I tried testing this on Fenix using the following changes:

diff --git a/app/src/debug/res/values/fenix_firebase.xml b/app/src/debug/res/values/fenix_firebase.xml
new file mode 100644
index 000000000..d6f3f4dcd
--- /dev/null
+++ b/app/src/debug/res/values/fenix_firebase.xml
@@ -0,0 +1,17 @@
[firebase keys content]
diff --git a/app/src/main/java/org/mozilla/fenix/components/BackgroundServices.kt b/app/src/main/java/org/mozilla/fenix/components/BackgroundServices.kt
index b32d28408..22cf21923 100644
--- a/app/src/main/java/org/mozilla/fenix/components/BackgroundServices.kt
+++ b/app/src/main/java/org/mozilla/fenix/components/BackgroundServices.kt
@@ -182,7 +182,8 @@ class BackgroundServices(
 
         // Enable push if it's configured.
         push.feature?.let { autoPushFeature ->
-            FxaPushSupportFeature(context, accountManager, autoPushFeature, crashReporter)
+            val feature = FxaPushSupportFeature(context, accountManager, autoPushFeature, crashReporter)
+            feature.initialize()
         }
 
         SendTabFeature(accountManager) { device, tabs ->

However, Fenix crashed and I saw the following exception in the log:

08-24 15:49:24.005 15627 15682 E ExceptionHandler: Uncaught exception handled: 
08-24 15:49:24.005 15627 15682 E ExceptionHandler: java.lang.IllegalStateException: Method addObserver must be called on the main thread
08-24 15:49:24.005 15627 15682 E ExceptionHandler: 	at androidx.lifecycle.LifecycleRegistry.enforceMainThreadIfNeeded(LifecycleRegistry.java:317)
08-24 15:49:24.005 15627 15682 E ExceptionHandler: 	at androidx.lifecycle.LifecycleRegistry.addObserver(LifecycleRegistry.java:172)
08-24 15:49:24.005 15627 15682 E ExceptionHandler: 	at mozilla.components.support.base.observer.ObserverRegistry.register(ObserverRegistry.kt:72)
08-24 15:49:24.005 15627 15682 E ExceptionHandler: 	at mozilla.components.feature.push.AutoPushFeature.register(Unknown Source:12)
08-24 15:49:24.005 15627 15682 E ExceptionHandler: 	at mozilla.components.feature.accounts.push.FxaPushSupportFeature$initialize$1.invokeSuspend(FxaPushSupportFeature.kt:91)
08-24 15:49:24.005 15627 15682 E ExceptionHandler: 	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
08-24 15:49:24.005 15627 15682 E ExceptionHandler: 	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
08-24 15:49:24.005 15627 15682 E ExceptionHandler: 	at kotlinx.coroutines.internal.LimitedDispatcher.run(LimitedDispatcher.kt:42)
08-24 15:49:24.005 15627 15682 E ExceptionHandler: 	at kotlinx.coroutines.scheduling.TaskImpl.run(Tasks.kt:95)
08-24 15:49:24.005 15627 15682 E ExceptionHandler: 	at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:570)
08-24 15:49:24.005 15627 15682 E ExceptionHandler: 	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:749)
08-24 15:49:24.005 15627 15682 E ExceptionHandler: 	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:677)
08-24 15:49:24.005 15627 15682 E ExceptionHandler: 	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:664)
08-24 15:49:24.005 15627 15682 E ExceptionHandler: 	Suppressed: kotlinx.coroutines.DiagnosticCoroutineContextException: [StandaloneCoroutine{Cancelling}@2f2dd09, Dispatchers.IO]

It looks like there's a threading issue to me, although I'm very over my head. I can try to run some more tests if it would be helpful.

[jonalmeida]

@bendk thanks for testing, that was super helpful! I made the mistake of not updating my Fenix patch to use the new initialize call so I didn't notice this earlier.

It looks like there's a threading issue to me, although I'm very over my head. I can try to run some more tests if it would be helpful.

The fix here is to ensure that the account manager and push observers are added to those features only on the Main thread. Since any feature that touches the account manager would end up initializing Fenix, we may end up adding these observers off the main thread and we've been getting pretty lucky so far.

This change made it very explicit that we run into this situation, so I've updated my patch to switch the main dispatcher when adding the above observers:

coroutineScope.launch(Main) {
    accountManager.register(accountObserver)

    pushFeature.register(autoPushObserver, owner, autoPause)
}

[bendk]

Just tested with the new changes and it worked great. I don't think I'm qualified to review the patch in general, but it seems good to me.

[mergify]

This pull request has conflicts when rebasing. Could you fix it @jonalmeida? 🙏

[csadilek]

Just one nit below. I like that we now have an explicit initialize call and all this work doesn't happen as a side effect of the feature's constructor.