FxA Account Manager refactor #7856

grigoryk · 2020-07-27T06:03:38Z

State machine changes:

"profile" states have been removed, profile is now handled outside of the state machine
states and events have been split into two subtypes: external and internal. External states are the AccountStates (authenticated, not authenticated, auth problem, etc) and internal states are InternalState (beginning auth, completing auth, recovering from auth problems, etc)
this split should hopefully make navigating the state machine more intuitive
retry logic and error handling was added for all internal transitions which require networking (Closes [Bug] Crash @kotlin.KotlinNullPointerException: at mozilla.components.service.fxa.manager.FxaAccountManager.postAuthenticated(FxaAccountManager.kt:36) #7536)

account manager changes:

removed ability to "set sync config" from the public API. we weren't using it (and have no plans to use this), so the added complexity wasn't benefiting anything.

async/suspend API changes:

account manager, account and device constellation APIs are now suspend, which makes both internal code and consuming of these APIs nicer and more flexible. Actual semantics of what's going on didn't change.

sync manager changes:

introduced a delay to the periodic sync scheduling, so that we can properly manage what goes on during startup (Closes #7335)

Tests are half-way being updating, so this is still a WIP.

cc @csadilek @rfk @eoger @jonalmeida

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.

rfk

There's a lot going on here, but I did a first pass and left some notes. I think you're right that it will make the state-machine easier to understand overall.

FWIW the PR would be easier to digest with the async->suspend stuff landed separately 😁

rfk · 2020-07-28T01:36:36Z

components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/Config.kt

 */
 data class SyncConfig(
    val supportedEngines: Set<SyncEngine>,
-    val syncPeriodInMinutes: Long? = null
+    val periodicSyncConfig: PeriodicSyncConfig?


I continue to be sad that we have to plumb a bunch of sync-related things through here, but oh well, not really anything you can do about it in this PR.

I'm not so sure this is bad, the upside is that it gives consumers a single point of interaction with all of these things.

If you mean something closer to "it's too bad sync manager is managed in one repo, but sync scheduling in another", then yes, I agree that it is unfortunate and something we should look into fixing 👍

components/concept/sync/src/main/java/mozilla/components/concept/sync/OAuthAccount.kt

rfk · 2020-07-28T02:27:34Z

...ents/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/FirefoxAccount.kt

        clientId: String,
        scopes: Array<String>,
        state: String,
        accessType: AccessType
-    ) = scope.async {
+    ) = withContext(scope.coroutineContext) {
        handleFxaExceptions(logger, "authorizeOAuthCode", { null }) {
            inner.authorizeOAuthCode(clientId, scopes, state, accessType.msg)
        }
    }

    override fun getSessionToken(): String? {


Do any of your consumers actually use getSessionToken, or is it exposed mostly for completeness? It doesn't feel like anyone should be calling this method in practice.

In practice, this is just used internally by the account manager (to get sync going).

In theory, if consumers aren't using the account manager this lets them do something useful with the account object beyond just fetching a profile, but... yeah, not really something we want to encourage.

Next on my plate is to work through the "we have a firefox-shaped oauth account abstraction with just a single implementation" bit, and simplify things on that front... Perhaps we can clean this up as part of that work.

rfk · 2020-07-28T02:32:42Z

...vice/firefox-accounts/src/main/java/mozilla/components/service/fxa/FxaDeviceConstellation.kt

+            AuthType.Signin,
+            AuthType.Signup,
+            AuthType.Pairing,
+            AuthType.MigratedCopy -> DeviceFinalizeAction.Initialize


Hrm, the fact that there are so many ways to spell "did a fresh signin" seems like a tiny bit of a code-smell to me. What things do we need to do that differ between Signin, Signup and Pairing? (I seem to recall this being added initially for metrics purposes?)

Another place where we use this is to kick-off sync after an onAuthenticated event (first sync vs startup sync), with a different mapping from device's init/ensure.

Otherwise, this is plumbed around in such detail mostly for telemetry, yeah.

I'll think about perhaps a different internal/external representation of this.

rfk · 2020-07-28T02:37:21Z

...e/firefox-accounts/src/main/java/mozilla/components/service/fxa/manager/FxaAccountManager.kt

+    ) = withContext(coroutineContext) {
+        when (val s = state) {
+            // Can't sync while we're still doing stuff.
+            is State.Active -> Unit


IIUC this means the syncNow request is just dropped on the floor. Is it possible that this might cause a bad user experience where they request a "sync now" but we don't honour it because we happened to be doing something else at the time?

That's right, we're dropping it here. FWIW, I think this can't really happen in practice - if we're in any of the "active" states, there isn't really a way for the user to request a sync. We're either starting/completing auth, logging out, migrating, etc - Fenix's UI, at least, doesn't expose "sync" button in any of these scenarios.

I'm not quite sure if having this check is necessary (it shouldn't happen), but left it here for completeness. This may as well produce a caught exception that we submit into Sentry, since it is an illegal event essentially (but, it exists outside of the state machine).

Another alternative is to make "sync" an "external event" on the state machine itself, but I'm not sure that's significantly better really, and it starts to introduce "sync" bits into what's otherwise an fxa-only thing.

grigoryk · 2020-07-28T02:53:49Z

Thanks for taking a look, @rfk! I'm still making some tweaks, but the core of it is there.

FWIW the PR would be easier to digest with the async->suspend stuff landed separately 😁

Yup, I got a little carried away there... At this point, it'll be quite a task to split the comments into a sensible "story", happy to chat on zoom/slack about any of this.

csadilek

OK, completed the first pass now.

The state machine looks really clean now with the different type of states! One suggestion below.

I also have one question about changing scopes in the push feature if I read this correctly and whether it was intentional.

Did a round of manual testing as well. All looking good.

...ccounts-push/src/main/java/mozilla/components/feature/accounts/push/FxaPushSupportFeature.kt

csadilek · 2020-07-28T20:50:10Z

...ccounts-push/src/main/java/mozilla/components/feature/accounts/push/FxaPushSupportFeature.kt

-            processRawEventAsync(String(rawEvent))
-        }
+        accountManager.withConstellation { CoroutineScope(Dispatchers.Main).launch {
+            processRawEvent(String(rawEvent))


Do we really want to run this on the main thread? Before this ran on AutoPushFeature.coroutineScope via notifyObservers.

Looks like we maybe want to make these observer methods suspend functions as well, so we don't need to context switch?

Yeah, this will run as before in practice (underneath we have a dedicated dispatcher), and marking the interface funs as suspend will avoid switching and certainly make the intention here clearer.

Filed #7899

@grigoryk I don't follow why we need to switch to the main dispatcher here. Previously the FxaPushSupportFeature would execute the call on the autopush dispatcher since account manager had it's own dispatcher. Shouldn't there be the ability to add a process a raw message that doesn't come from a coroutine on an internal dispatcher?

@jonalmeida Previously the FxaPushSupportFeature would execute the call on the autopush dispatcher i don't think that's quite correct. Yes, it'll execute, say, processRawEventAsync on the autopush dispatcher (or, in whichever context onMessageReceived will be invoked). However processRawEventAsync itself will do the actual work on the account manger's dispatcher. With the suspend function, the actual "work" is still happening on the account manager's dispatcher, just as before, it's just how we "kick it off" is different now - but I don't think that actually changes semantics here in a way that matters?

I agree that it's a little awkward, hence #7889. However we can't make observer methods suspend since Observable doesn't currently support "notifying" suspend observers, and I don't think we want to expand that pattern.

Another option is to remove withcontext wrapper from impl suspend methods, and rely on callers to correctly call these functions (e.g. on a worker dispatcher of sorts). Current approach holds your hand a bit, and doesn't let you screw up too badly. I'm not sure if that's necessary.

csadilek · 2020-07-28T20:50:44Z

...ccounts-push/src/main/java/mozilla/components/feature/accounts/push/FxaPushSupportFeature.kt

@@ -210,7 +215,9 @@ internal class AutoPushObserver(
                return@subscribe
            }

-            account.deviceConstellation().setDevicePushSubscriptionAsync(subscription.into())
+            CoroutineScope(Dispatchers.Main).launch {


Same question as above re: main scope.

...ure/accounts/src/main/java/mozilla/components/feature/accounts/FirefoxAccountsAuthFeature.kt

...e/firefox-accounts/src/main/java/mozilla/components/service/fxa/manager/FxaAccountManager.kt

...nents/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/manager/State.kt

.../firefox-accounts/src/test/java/mozilla/components/service/fxa/manager/FxaStateMatrixTest.kt

7915: Make sure device finalization succeeds before proceeding to postAuth r=csadilek a=grigoryk Closes #7536 Proper fix is coming in #7856, but this gets the crashes to go away. Co-authored-by: Grisha Kruglov <gkruglov@mozilla.com>

grigoryk · 2020-08-06T21:11:54Z

One remaining aspect of this PR that I need to change before this can land is improving how we handle the "offline startup" case. Current version will likely fail to finalize the account after multiple attempts, and will drive the user to the NotAuthenticated state, which isn't really something we'd ship. Existing state machine would just kind of give up at that point, and some things may not work correctly, but at least the user remains signed-in.

Ideally, we shouldn't be concerned with this stuff at the startup at all. Current fxaclient APIs don't make any guarantees around what happens during ensure/init calls, so we assume they talk to the network during a regular startup flow.
It would be nice if this wasn't the case - regular startup shouldn't require any network interactions.

We can "paper over" this at the state machine level, but I'm inclined to first investigate changing the a-s APIs first. cc @rfk

grigoryk · 2020-08-25T19:30:44Z

This should be good to land now. However, since I'm away until mid-September, and due to how various releases are aligning right now, let's land it once I'm back on the 16th.

grigoryk · 2020-09-18T21:51:09Z

@csadilek once you're back from PTO, let's get this landed. Should be good to go.

csadilek

@grigoryk OK did another pass and also looked at latest commits. Just some nits, but good to land from my perspective!

components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/Utils.kt

grigoryk · 2020-09-22T20:21:36Z

@csadilek addressed your feedback, thanks! Let's land this 👍

@csadilek

7856: FxA Account Manager refactor r=csadilek a=grigoryk ### State machine changes: - "profile" states have been removed, `profile` is now handled outside of the state machine - states and events have been split into two subtypes: external and internal. External states are the `AccountState`s (authenticated, not authenticated, auth problem, etc) and internal states are `InternalState` (beginning auth, completing auth, recovering from auth problems, etc) - this split should hopefully make navigating the state machine more intuitive - retry logic and error handling was added for all internal transitions which require networking (Closes #7536) ### account manager changes: - removed ability to "set sync config" from the public API. we weren't using it (and have no plans to use this), so the added complexity wasn't benefiting anything. ### async/suspend API changes: account manager, account and device constellation APIs are now `suspend`, which makes both internal code and consuming of these APIs nicer and more flexible. Actual semantics of what's going on didn't change. ### sync manager changes: introduced a delay to the periodic sync scheduling, so that we can properly manage what goes on during startup (Closes #7335) Tests are half-way being updating, so this is still a WIP. cc @csadilek @rfk @eoger @jonalmeida Co-authored-by: Grisha Kruglov <gkruglov@mozilla.com>

Prior to mozilla-mobile#7856 we only had a NotAuthenticated state, which user would remain in until the very end of the authentication process (receiving a 'finished...' api call). After the refactor, we introduced an in-progress states - specifically, BeginningAuthentication. If the user cancels the auth flow (e.g. closes a custom tab), they will remain in this state. Subsequent login attempts would then fail. This patch fixes this by introducing a Cancel event which we trigger whenever handling 'beginAuthentication' calls. In normal scenarios, this event is ignored. Otherwise, it "resets" the state machine into NotAuthenticated state before emitting any Begin* events.

Prior to #7856 we only had a NotAuthenticated state, which user would remain in until the very end of the authentication process (receiving a 'finished...' api call). After the refactor, we introduced an in-progress states - specifically, BeginningAuthentication. If the user cancels the auth flow (e.g. closes a custom tab), they will remain in this state. Subsequent login attempts would then fail. This patch fixes this by introducing a Cancel event which we trigger whenever handling 'beginAuthentication' calls. In normal scenarios, this event is ignored. Otherwise, it "resets" the state machine into NotAuthenticated state before emitting any Begin* events.

grigoryk requested a review from csadilek July 27, 2020 06:03

grigoryk force-pushed the issue7536PairingCrash branch 2 times, most recently from 4c9ddfa to 8432ab9 Compare July 27, 2020 19:58

csadilek self-assigned this Jul 27, 2020

rfk reviewed Jul 28, 2020

View reviewed changes

grigoryk mentioned this pull request Jul 28, 2020

Update breaking changes in the FxA/Sync integration mozilla-mobile/fenix#13014

Merged

4 tasks

grigoryk force-pushed the issue7536PairingCrash branch 3 times, most recently from 2d62208 to 27aa037 Compare July 28, 2020 03:28

This was referenced Jul 28, 2020

Connecting to sync immediately syncs twice #7335

Closed

crash at mozilla.appservices.fxaclient.rust.RustError.intoException(RustError.kt:5) #7889

Closed

csadilek reviewed Jul 28, 2020

View reviewed changes

grigoryk mentioned this pull request Jul 29, 2020

Consider switching AutoPushFeature.Observer functions to be suspend #7899

Closed

grigoryk force-pushed the issue7536PairingCrash branch 4 times, most recently from 6d6d7d4 to 8af69cf Compare July 29, 2020 01:57

BranescuMihai mentioned this pull request Jul 29, 2020

[Bug] [Synced tabs] The list of opened tabs are not displayed after restarting the app and accessing Synced tabs mozilla-mobile/fenix#12195

Closed

grigoryk mentioned this pull request Jul 29, 2020

Make sure device finalization succeeds before proceeding to postAuth #7915

Merged

4 tasks

grigoryk mentioned this pull request Aug 6, 2020

Account startup flow without requiring any network activity mozilla/application-services#3474

Closed

grigoryk mentioned this pull request Aug 20, 2020

Adding a new device capabilities: considerations #8164

Closed

grigoryk force-pushed the issue7536PairingCrash branch 4 times, most recently from 6b35775 to 60087d2 Compare August 22, 2020 06:34

grigoryk marked this pull request as ready for review August 22, 2020 06:35

grigoryk force-pushed the issue7536PairingCrash branch 2 times, most recently from 9f892e0 to ae3c302 Compare September 16, 2020 20:31

grigoryk added the 🕵️‍♀️ needs review PRs that need to be reviewed label Sep 19, 2020

csadilek approved these changes Sep 22, 2020

View reviewed changes

grigoryk force-pushed the issue7536PairingCrash branch from 6703c34 to 129e50c Compare September 22, 2020 19:57

Grisha Kruglov added 4 commits September 22, 2020 13:18

Account manager state machine refactoring

d774cb5

Move withRetries* into Utils, add tests

55367ec

Lint and test fixes

c74f17e

Review feedback - kdocs, etc

e06ef87

grigoryk force-pushed the issue7536PairingCrash branch from e263b28 to e06ef87 Compare September 22, 2020 20:18

grigoryk added 🛬 needs landing PRs that are ready to land and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Sep 22, 2020

mozilla-mobile deleted a comment from bors bot Sep 22, 2020

Merge branch 'master' into issue7536PairingCrash

49fc974

mergify bot merged commit a49be43 into mozilla-mobile:master Sep 22, 2020

grigoryk deleted the issue7536PairingCrash branch September 22, 2020 20:40

mstange mentioned this pull request Sep 24, 2020

Crash in [@ mozilla.appservices.fxaclient.FxaException$Network: at mozilla.appservices.fxaclient.rust.RustError.intoException(RustError.kt:5)] #8492

Closed

grigoryk mentioned this pull request Oct 14, 2020

AbnormalFxaEvent$MissingExpectedAccountAfterStartup mozilla-mobile/fenix#15882

Closed

grigoryk mentioned this pull request Nov 13, 2020

For fenix#16232 - Make sure we cancel any in-progress auth #8976

Merged

4 tasks

st3fan mentioned this pull request Nov 14, 2020

Uplift: For fenix#16232 - Make sure we cancel any in-progress auth #8978

Merged

4 tasks

grigoryk mentioned this pull request Jan 4, 2021

[Bug] Sync logs off on poor connection mozilla-mobile/fenix#17301

Closed

data-sync-user mentioned this pull request Jun 22, 2021

Account startup flow without requiring any network activity mozilla/uniffi-rs#835

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FxA Account Manager refactor #7856

FxA Account Manager refactor #7856

grigoryk commented Jul 27, 2020 •

edited

Loading

rfk left a comment

rfk Jul 28, 2020

grigoryk Jul 28, 2020

rfk Jul 28, 2020

grigoryk Jul 28, 2020

rfk Jul 28, 2020

grigoryk Jul 28, 2020

grigoryk Jul 28, 2020

rfk Jul 28, 2020

grigoryk Jul 28, 2020 •

edited

Loading

grigoryk commented Jul 28, 2020 •

edited

Loading

csadilek left a comment

csadilek Jul 28, 2020

csadilek Jul 28, 2020

grigoryk Jul 29, 2020

grigoryk Jul 29, 2020

jonalmeida Aug 7, 2020 •

edited

Loading

grigoryk Sep 18, 2020

csadilek Jul 28, 2020

grigoryk commented Aug 6, 2020

grigoryk commented Aug 25, 2020

grigoryk commented Sep 18, 2020

csadilek left a comment

grigoryk commented Sep 22, 2020 •

edited

Loading

FxA Account Manager refactor #7856

FxA Account Manager refactor #7856

Conversation

grigoryk commented Jul 27, 2020 • edited Loading

State machine changes:

account manager changes:

async/suspend API changes:

sync manager changes:

Pull Request checklist

After merge

rfk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grigoryk Jul 28, 2020 • edited Loading

Choose a reason for hiding this comment

grigoryk commented Jul 28, 2020 • edited Loading

csadilek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonalmeida Aug 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grigoryk commented Aug 6, 2020

grigoryk commented Aug 25, 2020

grigoryk commented Sep 18, 2020

csadilek left a comment

Choose a reason for hiding this comment

grigoryk commented Sep 22, 2020 • edited Loading

grigoryk commented Jul 27, 2020 •

edited

Loading

grigoryk Jul 28, 2020 •

edited

Loading

grigoryk commented Jul 28, 2020 •

edited

Loading

jonalmeida Aug 7, 2020 •

edited

Loading

grigoryk commented Sep 22, 2020 •

edited

Loading