Migrate Fenix to using the app-services FxA state machine
Categories
(Firefox for Android :: Accounts and Sync, enhancement)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox145 | --- | fixed |
People
(Reporter: bdk, Assigned: lina, Mentored)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
We're going to be adding an FxA state machine in app-services that provides the same functionality as the Fenix code. We should move Fenix over to using this state machine so that:
- In the short term, the app-services team intends to fix some FxA bugs using that state machine
- In the long term, if other applications want to consume the app-services FxA client they will be using that state machine and it will be good to get Fenix on the same code.
|
||
Updated•2 years ago
|
|
Reporter | |
Comment 2•2 years ago
•
|
||
I think the time has finally come to do this one for a few reasons.
I've been monitoring at the Sentry reports for mismatches between the app-services state machine and the android state machine. The events have gotten pretty low as a result of several fixes. They're now at about 5 per day, down from about 500 a month ago. From what I can see, the remaining events are caused by 2 cases and both of them represent issues in the android code.
The first mismatch is on access token key errors. In this case the android-components code transitions directly to the authentication problems state. However, the application-services code forces you to check the authorization status and only transitions to the auth issues state if that fails. The "authorization status check" means attempting to get a new access token from the refresh token.
My feeling is that the android-components code is wrong since there are times where the access token has expired, but you would be able to get a new one using the refresh token. I could be wrong though, maybe the android-components code has identified cases where we know the refresh token is definitely invalid and we can skip the authentication check. However, even if that's true I think the current application-services API is better from our POV. It feels much safer to always do the check an not give the consumer an event that skips it. The worst case scenario is an extra API request to FxA, which seems fine.
The second mismatch feels like a pretty bad bug in the android-components code to me. The code creates a CoroutineContext using Executors.newSingleThreadExecutor and only calls processQueue inside that context to try to ensure serialized execution of the queue. However, there's an issue with that model: if a task running in that CoroutineContext calls processQueue again then the code will run processQueue again while the first one is still running. This is currently happening in this corner case:
- firefox-android performs some FxA operation inside processQueue
- All the Sentry reports I read were for completing oauth, but I think it could be any operation.
- The operation fails with an auth error
- Eventually, encounteredAuthError is called which starts another processQueue run (note that the first one is still in the middle of processing)
- At this point everything is very weird, the inner process queue will probably cause the state machine to transition to the auth problems state. Then the first process queue call will then continue and probably error out as well, but I suspect that there can be some subtle/weird bugs that happen in this process.
To summarize, I think the state machine process has found all the bugs in the app-services code it's going to find and it's now revealing bugs in the android-components code that should be fixed. The simplest way to fix them would be to migrate to using the app-services code.
The other reason I want to make the switch now is https://bugzilla.mozilla.org/show_bug.cgi?id=1887071. I believe we can fix that without too much work, but for a while I was worried that we would need to know that the client transitioned to the auth issues state in order to fix it. This makes me think there are potential bugs out there that would be easier to fix if the consumers were using our state machine rather than theirs.
|
|
Reporter | |
Comment 3•2 years ago
|
||
I spoke too soon. After digging through the sentry reports more, I found a couple other issues buried in the noise:
- https://bugzilla.mozilla.org/show_bug.cgi?id=1887958
- https://bugzilla.mozilla.org/show_bug.cgi?id=1887959
I'm still thinking it's a good idea to do this transition, but we should fix those first.
|
|
Reporter | |
Comment 4•2 years ago
|
||
This is my attempt to merge the rest of
https://github.com/mozilla-mobile/firefox-android/pull/4912 to
moz-central.
The tests won't pass. My feeling is that most of them can be deleted
since we have tests for the same functionality in app-services.
I'm not sure if the code will compile, I'm just getting used to the new
environment and couldn't figure out how to build it without also running
the tests.
There's probably more refactoring / code deletion that could happen as
well.
|
Assignee | |
Updated•2 years ago
|
|
||
Updated•1 year ago
|
|
|
Reporter | |
Comment 5•1 year ago
|
||
|
||
Comment 8•1 year ago
|
||
|
|
||
Updated•1 year ago
|
|
|
Reporter | |
Comment 9•1 year ago
|
||
FxaAccountManagerTest.kt was failing after the move, which makes sense since we totally overhauled it. I pushed a new version of that test and it's waiting for review now.
|
||
Comment 10•1 year ago
|
||
|
||
Comment 11•1 year ago
|
||
| bugherder | ||
Description
•