AuthAPNSTokenManager.getToken() fatal error unwrapping nil

[snapxo]

Description

When performing PhoneAuthProvider.provider().verifyPhoneNumber and getting a timeout, the app crashes since AuthAPNSTokenManager runs into a nil unwrapping fatal error.

To my understanding, there is an issue in the commit [auth-swift] Finish Transition to async-await for RPC calls (#11557) from 19 Jul 2023.

Indeed the design of the callback in getTokenInternal(callback: @escaping (AuthAPNSToken?, Error?) -> Void) lacks consistency in my opinion.
It takes a tuple of two optional (AuthAPNSToken?, Error?) instead of using more appropriate structures such as Result<AuthAPNSToken, Error>.

It is probably envisioned by the developer that "it will never happen that both are nil at the same time", and thus we come with the implementation of getToken() async throws AuthAPNSToken which force unwraps error in the case of token being nil. See AuthAPNSTokenManager lines 76 to 86.

What strikes me is that in the case of a timeout, this callback (through the call of self.callback which gets over all the pending callbacks in the array which was set line 61) is called with (nil, nil) !

So it comes with not so much surprise that line 82 leads to a fatal error by force unwrapping a nil value.

Reproducing the issue

1. configure Firebase
2. call PhoneAuthProvider.provider().verifyPhoneNumber
3. get a timeout after 5 sec
4. app crashes

Firebase SDK Version

11.0.0

Xcode Version

15.4

Installation Method

Swift Package Manager

Firebase Product(s)

Authentication

Targeted Platforms

iOS

Relevant Log Output

No response

If using Swift Package Manager, the project's Package.resolved

Expand Package.resolved snippet

Not applicable.

If using CocoaPods, the project's Podfile.lock

Expand Podfile.lock snippet

Not applicable.

[google-oss-bot]

I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.

[paulb777]

@snapxo Thanks for the report and sharing the investigation.

We'll work on a fix for the next release.

[ncooke3]

Thanks again for the report, @snapxo. A fix has been staged for next release.

[snapxo]

@ncooke3 thanks for being so quick to respond to this issue. Looks good with Result in #13472.

[edit] changed my commentary to a review note on the PR, to have it at the right place

But the most important is that it doesn't crash 💥, which it shouldn't 👍