dataconnect: Fixed occasional NullPointerException leading to erroneous UNAUTHENTICATED exceptions - #7001
Conversation
Sorry, something went wrong.
…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. |
Sorry, something went wrong.
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() in FirebaseDataConnectImpl for Auth and AppCheck to ensure proper startup order.
- Add comprehensive tests for initialize(), idempotency, ordering with close(), and integrate initialize() 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 |
firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectCredentialsTokenManager.kt:84
- [nitpick] The state name New is quite generic and could be confused with other states. Consider renaming it to Uninitialized to 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) before initialize() throws the expected IllegalStateException, as described in the initialize() 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 for DataConnectAuth. Ensure this side effect is intentional and document the eager initialization behavior.
.apply { initialize() }
Sorry, something went wrong.
Coverage Report 1Affected Products
Test Logs |
Sorry, something went wrong.
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.com.google.firebase.ai.DevAPIStreamingSnapshotTests ‑ citation parsed correctly com.google.firebase.ai.DevAPIStreamingSnapshotTests ‑ image rejected com.google.firebase.ai.DevAPIStreamingSnapshotTests ‑ long reply com.google.firebase.ai.DevAPIStreamingSnapshotTests ‑ prompt blocked for safety com.google.firebase.ai.DevAPIStreamingSnapshotTests ‑ short reply com.google.firebase.ai.DevAPIStreamingSnapshotTests ‑ stopped for recitation com.google.firebase.ai.DevAPIUnarySnapshotTests ‑ citation returns correctly com.google.firebase.ai.DevAPIUnarySnapshotTests ‑ invalid api key com.google.firebase.ai.DevAPIUnarySnapshotTests ‑ long reply com.google.firebase.ai.DevAPIUnarySnapshotTests ‑ response blocked for safety … com.google.firebase.dataconnect.AnyValueSerializerUnitTest ‑ descriptor should have expected values com.google.firebase.dataconnect.AnyValueSerializerUnitTest ‑ deserialize() should throw UnsupportedOperationException com.google.firebase.dataconnect.AnyValueSerializerUnitTest ‑ serialize() should throw UnsupportedOperationException com.google.firebase.dataconnect.AnyValueUnitTest ‑ constructor(Boolean) creates an object with the expected value com.google.firebase.dataconnect.AnyValueUnitTest ‑ constructor(Double) creates an object with the expected value (edge cases) com.google.firebase.dataconnect.AnyValueUnitTest ‑ constructor(Double) creates an object with the expected value (normal cases) com.google.firebase.dataconnect.AnyValueUnitTest ‑ constructor(List) creates an object with the expected value (edge cases) com.google.firebase.dataconnect.AnyValueUnitTest ‑ constructor(List) creates an object with the expected value (normal cases) com.google.firebase.dataconnect.AnyValueUnitTest ‑ constructor(Map) creates an object with the expected value (edge cases) com.google.firebase.dataconnect.AnyValueUnitTest ‑ constructor(Map) creates an object with the expected value (normal cases) … ♻️ This comment has been updated with latest results. |
Sorry, something went wrong.
…d `getToken() before `initialize()`
Size Report 1Affected Products
Test Logs |
Sorry, something went wrong.
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.
There is, unfortunately, no workaround for the bug. The only mitigation would be to close the FirebaseDataConnect object, 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 DeferredProviderHandlerImpl was called back very quickly and the DataConnectAuth constructor had not yet completed, namely, not having yet assigned idTokenListener (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.