1864667 - fxa-client-other-error: No stored session token

Status: RESOLVED FIXED

(Firefox for Android :: Accounts and Sync, defect, P3)

Description

[Ben Dean-Kawamura [:bdk]]

We currently have a good deal of these in Sentry.

The FxA component believes the user is logged in, but there's no session token stored. I think this means that either the FxA component is wrong or we're somehow losing the session token.

Comment 1

[Ben Dean-Kawamura [:bdk]]

Could this be what's happening:

• The user gets into the AuthenticationProblems state
• Fenix restarts
• Fenix sees persisted data so it tries to restore the acccount
• Fenix gets to this code block but doesn't see any errors since both finalizeDevice() and authenticationSideEffects() may do nothing if the right data is cached.
• Fenix considers the user as logged in and starts sending notifications about it, even though they're not really logged in.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

ah - so the fact we are in a bad auth state isn't actually persisted? I wonder what should be persisted - shouldn't we drop the session token at that point?

Comment 3

[Ben Dean-Kawamura [:bdk]]

It might be persisted, but this code doesn't seem to check that. It assumes that if we restored an account from disk, then we should try to connect using the stored credentials.

Comment 4

[Ben Dean-Kawamura [:bdk]]

After looking through the metrics and reading the code some more, I have a slightly different theory. I don't think this happens if we persist the account in the AuthenticationProblems state, since I don't believe we delete the session token in that state. Instead, I think we're persisting the the NotAuthenticated state.

When the android-components code moves to this state it deletes the account storage. However, I don't think there's anything from stopping us from then writing the account storage again. For example, if some code calls get_access_token

Let's add some metrics to test this theory.

In general, it seems that android-components avoids persisting data for disconnected accounts, while the application-services code persists a more-or-less empty JSON dict instead. My feeling is that both approaches are fine, but mixing the two is likely going to lead to issues. I think the simplest way forward would be to update the android-components code to follow the app-services model by:

• Not deleting the account storage
• Checking for disconnected accounts when restoring

Comment 5

[Ben Dean-Kawamura [:bdk]]

I'm slightly less sure about the above theory because we already have a check for restoring an account in a non-authenticated state and there's no sentry events for it.

There's not a ton of errors in Sentry, so let's wait until the state machine gets moved to app-services to investigate this one more.

Comment 6

[Ben Dean-Kawamura [:bdk]]

Looking through the logs, this definitely seems related to initialization and restoring the state. All the Sentry breadcrumbs I looked at look like this:

fxa-client-other-error: No stored session token
fxa_client::internal::http_client[386]: get_devices timestamp: 1757437600061, refresh len: 64 (4)
fxa_client::state_machine[81]: FxaStateMachine.process_event finished (Complete(Connected)) (3)
fxa_client::state_machine[93]: FxaStateMachine.process_event EnsureDeviceCapabilitiesSuccess -> Complete(Connected) (2)
fxa_client::state_machine[93]: FxaStateMachine.process_event GetAthStateSuccess -> EnsureDeviceCapabilities (1)
fxa_client::state_machine[65]: FxaStateMachine.process_event starting: Initialize (0)

I wonder if this is just a side-effect of https://bugzilla.mozilla.org/show_bug.cgi?id=1794212. When we initialize, if the user was in the auth issues state, we try to move to the Authenticated state anyways. This was to match the a-c behavior to make the state machine checker work better, but it's likely that it caused this bug. I'm going to try to fix that one and see what happens.

I don't think this happens if we persist the account in the AuthenticationProblems state, since I don't believe we delete the session token in that state

Maybe this was true in the past, but it's not anymore.

Comment 7

[Ben Dean-Kawamura [:bdk]]

We merged https://github.com/mozilla/application-services/pull/6982 to try to fix this on 10/15. The last AS nightly version that appears on the Sentry discover page is 10/17. Seems likely that the new code fixed this. I'm not 100% sure, but the error volume is very low so I think we can justs declare success and not spend more time on this.