fix(auth): fix "Already resumed" crash in phone auth SMS auto-verification - #2447
fix(auth): fix "Already resumed" crash in phone auth SMS auto-verification#2447demolaf wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
19c6f52 to
655b983
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves phone number verification by handling coroutine cancellation and multiple callbacks more robustly. It transitions from suspendCoroutine to suspendCancellableCoroutine and introduces an AtomicBoolean to prevent multiple callbacks from resuming the continuation. Additionally, user-initiated cancellation now clears the pending loading state and propagates the CancellationException directly rather than emitting an error state. Comprehensive unit tests have been added to cover these scenarios. The review feedback suggests registering an invokeOnCancellation listener to mark the operation as resolved upon cancellation, which prevents late-arriving callbacks from attempting to resume the cancelled continuation.
655b983 to
233082c
Compare
russellwheatley
left a comment
russellwheatley
left a comment
There was a problem hiding this comment.
Crash is gone, but first-callback-wins still drops auto-retrieval on the path this PR is fixing.
| .setCallbacks(object : | ||
| PhoneAuthProvider.OnVerificationStateChangedCallbacks() { | ||
| override fun onVerificationCompleted(credential: PhoneAuthCredential) { | ||
| if (!isResolved.compareAndSet(false, true)) { |
There was a problem hiding this comment.
This latch stops the crash, but it also drops the auto-retrieved credential on the exact path this PR is fixing. Firebase's SMS auto-retrieval order is onCodeSent then onVerificationCompleted, so the user lands on the code-entry screen after Firebase already verified them. The Verifier interface is single-shot, so you'll need a side channel (PhoneAuthScreen already signs in on SMSAutoVerified). Can you keep the latch for the crash, but still emit SMSAutoVerified when a late onVerificationCompleted arrives after onCodeSent?
| @@ -136,12 +136,10 @@ internal suspend fun FirebaseAuthUI.verifyPhoneNumber( | |||
| } | |||
| } | |||
| } catch (e: CancellationException) { | |||
There was a problem hiding this comment.
This is now the only provider that rethrows a raw CancellationException and resets to Idle. Everything else wraps it as AuthCancelledException and emits AuthState.Error. Rethrowing is closer to structured concurrency, but it's a one-off. Can you either match the other providers, or call out why phone verify is special?
| * Clears a pending [AuthState.Loading] by resetting to [AuthState.Idle]. States written by | ||
| * another operation in the meantime are left untouched. | ||
| */ | ||
| internal fun clearLoadingState() { |
There was a problem hiding this comment.
This only skips non-Loading states, so a newer Loading from a resend or another in-flight call would get reset to Idle. The new test covers PasswordResetLinkSent, not a second Loading. Can you key this on the Loading this call actually wrote, or drop the "left untouched" claim in the doc comment if that's not actually guaranteed?
2 participants
Fixes #2446.
DefaultVerifier.verifyPhoneNumberadapted Firebase'sOnVerificationStateChangedCallbacks— which can fire more than once per request — onto a single-shotsuspendCoroutine, resuming the same continuation fromonVerificationCompleted,onVerificationFailedandonCodeSent. On the SMS auto-retrieval path Firebase deliversonCodeSentand thenonVerificationCompleted, so the second resume threwIllegalStateException: Already resumedon the main thread inside Firebase's own dispatcher, where nothing in the library can catch it. A singleAtomicBooleanlatch now lets the first callback resolve the continuation and drops later ones with a log line.Switching to
suspendCancellableCoroutinealso made cancellation reachable for the first time, soverifyPhoneNumbernow rethrowsCancellationExceptionrather than reporting it as an auth error, and clears a pendingLoadingstate so navigating away mid-request can't leave the UI spinning.Added
PhoneAuthDefaultVerifierTestcovering every callback ordering plus cancellation — verified the multi-callback cases fail on the old code and pass with the fix.