diff --git a/FirebaseAuth/CHANGELOG.md b/FirebaseAuth/CHANGELOG.md index 78a1bbfa96c..7bfc6f936e9 100644 --- a/FirebaseAuth/CHANGELOG.md +++ b/FirebaseAuth/CHANGELOG.md @@ -1,4 +1,6 @@ # 13.0.0 +- [fixed] Fixed a timeout race in the APNs token manager that could leave concurrent phone + authentication requests waiting indefinitely. - [changed] Replaced a force-unwrapped error with a safe fallback during Game Center authentication. (#16136) diff --git a/FirebaseAuth/Sources/Swift/SystemService/AuthAPNSTokenManager.swift b/FirebaseAuth/Sources/Swift/SystemService/AuthAPNSTokenManager.swift index c444217fb3e..56ab12e66e9 100644 --- a/FirebaseAuth/Sources/Swift/SystemService/AuthAPNSTokenManager.swift +++ b/FirebaseAuth/Sources/Swift/SystemService/AuthAPNSTokenManager.swift @@ -56,22 +56,21 @@ } if pendingCallbacks.count > 0 { pendingCallbacks.append(callback) - // TODO(ncooke3): This is likely a bug in that the async wrapper method - // cannot make forward progress. return } + pendingRequestGeneration += 1 + let requestGeneration = pendingRequestGeneration pendingCallbacks = [callback] DispatchQueue.main.async { self.application.registerForRemoteNotifications() } - let applicableCallbacks = pendingCallbacks let deadline = DispatchTime.now() + timeout kAuthGlobalWorkQueue.asyncAfter(deadline: deadline) { - // Only cancel if the pending callbacks remain the same, i.e., not triggered yet. - if applicableCallbacks.count == self.pendingCallbacks.count { - self.callback(.failure(AuthErrorUtils.missingAppTokenError(underlyingError: nil))) - } + // Only cancel if this is still the active request and it has not completed. + guard requestGeneration == self.pendingRequestGeneration, + !self.pendingCallbacks.isEmpty else { return } + self.callback(.failure(AuthErrorUtils.missingAppTokenError(underlyingError: nil))) } } @@ -124,6 +123,7 @@ /// Enable unit test faking. var application: AuthAPNSTokenApplication private var pendingCallbacks: [(Result) -> Void] = [] + private var pendingRequestGeneration = 0 private func callback(_ result: Result) { let pendingCallbacks = self.pendingCallbacks diff --git a/FirebaseAuth/Tests/Unit/AuthAPNSTokenManagerTests.swift b/FirebaseAuth/Tests/Unit/AuthAPNSTokenManagerTests.swift index 36e214f6149..4184f9280bd 100644 --- a/FirebaseAuth/Tests/Unit/AuthAPNSTokenManagerTests.swift +++ b/FirebaseAuth/Tests/Unit/AuthAPNSTokenManagerTests.swift @@ -164,6 +164,119 @@ waitForExpectations(timeout: 5) } + /** @fn testMultipleCallbacksTimeout + @brief Tests all pending callbacks are called when registration times out. + */ + func testMultipleCallbacksTimeout() throws { + manager = AuthAPNSTokenManager( + withApplication: fakeApplication!, + timeout: kRegistrationTimeout + ) + let manager = try XCTUnwrap(manager) + let expectation = self.expectation(description: #function) + expectation.expectedFulfillmentCount = 2 + let callbackCount = UnfairLock(0) + + for _ in 0 ..< 2 { + manager.getTokenInternal { result in + switch result { + case let .success(token): + XCTFail("Unexpected success: \(token)") + case let .failure(error): + XCTAssertEqual( + error as NSError, + AuthErrorUtils.missingAppTokenError(underlyingError: nil) as NSError + ) + } + callbackCount.withLock { $0 += 1 } + expectation.fulfill() + } + } + + waitForExpectations(timeout: 2) + XCTAssertEqual(callbackCount.value(), 2) + } + + /** @fn testOldTimeoutDoesNotCancelNewRequest + @brief Tests a timeout from a cancelled request cannot complete a later request. + */ + func testOldTimeoutDoesNotCancelNewRequest() throws { + let timeout: TimeInterval = 1 + manager = AuthAPNSTokenManager( + withApplication: fakeApplication!, + timeout: timeout + ) + let manager = try XCTUnwrap(manager) + let expectedError = error + + let firstRequestCanceled = expectation(description: "firstRequestCanceled") + let secondCallbackCalled = UnfairLock(false) + + manager.getTokenInternal { result in + switch result { + case let .success(token): + XCTFail("Unexpected success: \(token)") + case let .failure(error): + XCTAssertEqual(error as NSError, expectedError) + } + firstRequestCanceled.fulfill() + } + + // Start the second request before the first request's timeout fires, so the old + // timeout would incorrectly match it if requests were compared by callback count. + let noEarlyCallback = expectation(description: "noEarlyCallback") + + DispatchQueue.main.asyncAfter(deadline: .now() + 0.2) { + manager.cancel(withError: expectedError) + + manager.getTokenInternal { result in + secondCallbackCalled.withLock { $0 = true } + + switch result { + case let .success(token): + XCTFail("Unexpected success: \(token)") + case let .failure(error): + XCTAssertEqual(error as NSError, expectedError) + } + } + } + + DispatchQueue.main.asyncAfter(deadline: .now() + 1.1) { + XCTAssertFalse(secondCallbackCalled.value()) + manager.cancel(withError: expectedError) + noEarlyCallback.fulfill() + } + + waitForExpectations(timeout: 2) + } + + /** @fn testGetTokenTimeoutWithMultipleWaiters + @brief Tests concurrent async callers complete when registration times out. + */ + func testGetTokenTimeoutWithMultipleWaiters() async throws { + manager = AuthAPNSTokenManager( + withApplication: fakeApplication!, + timeout: kRegistrationTimeout + ) + let manager = try XCTUnwrap(manager) + + async let firstResult = getTokenResult(from: manager) + async let secondResult = getTokenResult(from: manager) + + let results = await [firstResult, secondResult] + for result in results { + switch result { + case let .success(token): + XCTFail("Unexpected success: \(token)") + case let .failure(error): + XCTAssertEqual( + error as NSError, + AuthErrorUtils.missingAppTokenError(underlyingError: nil) as NSError + ) + } + } + } + /** @fn testCancel @brief Tests cancelling the pending callbacks. */ @@ -211,5 +324,14 @@ registerCalled = true } } + + private func getTokenResult(from manager: AuthAPNSTokenManager) async -> + Result { + do { + return try .success(await manager.getToken()) + } catch { + return .failure(error) + } + } } #endif