dataconnect: Fixed occasional NullPointerException leading to erroneous UNAUTHENTICATED exceptions
#7001
Conversation
…initialize` function to fix race condition (b/419064737)
📝 PRs merging into main branchOur main branch should always be in a releasable state. If you are working on a larger change, or if you don't want this change to see the light of the day just yet, consider using a feature branch first, and only merge into the main branch when the code complete and ready to be released. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a race condition in Data Connect where DataConnectAuth could register a null token listener before its constructor completed, leading to spurious NullPointerException and downstream UNAUTHENTICATED errors. It introduces an explicit initialize() API to control the setup phase, hooks initialize() into FirebaseDataConnectImpl for both Auth and AppCheck, and augments unit tests to cover initialize and close behaviors.
- Extract initialization logic from constructor init block into an explicit
initialize()method and update the internal state machine. - Automatically invoke
initialize()inFirebaseDataConnectImplfor Auth and AppCheck to ensure proper startup order. - Add comprehensive tests for
initialize(), idempotency, ordering withclose(), and integrateinitialize()calls into existing tests.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/core/DataConnectAuthUnitTest.kt | Added new tests for initialize(), updated existing tests to call initialize() before other methods |
| firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/FirebaseDataConnectImpl.kt | Chained .apply { initialize() } to Auth and AppCheck instances |
| firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectCredentialsTokenManager.kt | Moved init-block logic into initialize(), introduced State.New and State.Initialized enum values and updated state transitions |
| firebase-dataconnect/CHANGELOG.md | Documented the fixed race condition |
Comments suppressed due to low confidence (3)
firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectCredentialsTokenManager.kt:84
- [nitpick] The state name
Newis quite generic and could be confused with other states. Consider renaming it toUninitializedto clearly indicate that initialization has not yet occurred.
object New : State<Nothing>
firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/core/DataConnectAuthUnitTest.kt:98
- Consider adding a test to verify that calling
getToken()(or other public methods) beforeinitialize()throws the expectedIllegalStateException, as described in theinitialize()KDoc.
fun `initialize() should succeed if called on a brand new instance`() = runTest {
firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/FirebaseDataConnectImpl.kt:126
- [nitpick] Automatically calling
initialize()during construction changes the lifecycle and startup ordering guarantees forDataConnectAuth. Ensure this side effect is intentional and document the eager initialization behavior.
.apply { initialize() }
Coverage Report 1Affected Products
Test Logs |
Test Results 66 files + 42 66 suites +42 1m 19s ⏱️ +45s Results for commit 4df2df4. ± Comparison against base commit af24598. This pull request removes 102 and adds 559 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
…d `getToken() before `initialize()`
Size Report 1Affected Products
Test Logs |
This PR fixes a race condition in Data Connect where
DataConnectAuthcould register a null token listener before its constructor completed, leading to spuriousNullPointerExceptionand downstreamUNAUTHENTICATEDerrors. It introduces an explicitinitialize()API to control the setup phase, hooksinitialize()intoFirebaseDataConnectImplfor both Auth and AppCheck, and augments unit tests to coverinitializeandclosebehaviors.There is, unfortunately, no workaround for the bug. The only mitigation would be to close the
FirebaseDataConnectobject, re-open it, and try again. This fix will be included in the release of the firebase-android-sdk in about 4 weeks, near the end of June 2025.The bug was easy to reproduce: simply add a call to
Thread.sleep()in a certain place and, boom. This is a race condition in this code:firebase-android-sdk/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectCredentialsTokenManager.kt
Lines 77 to 84 in 51b4a1c
The problem occurred when the
DeferredProviderHandlerImplwas called back very quickly and theDataConnectAuthconstructor had not yet completed, namely, not having yet assignedidTokenListener(link), and, therefore, registering a null listener, resulting in the NullPointerException.The problematic exception was:
which would result in an exception later on when a query or mutation was executed:
Googlers see b/419064737 for full details of the bug, as reported by a customer.