-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[Firestore] Fix crash fetching Auth and App Check tokens #15558
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why the tests here are changing how the provider is created.
Do the added class members need to be public?
// Firestore/core/src/credentials/firebase_app_check_credentials_provider_apple.h
public std::enable_shared_from_this<FirebaseAppCheckCredentialsProvider> {There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's Gemini's explanation for why the tests needed changes:
When the production code is built, it uses std::make_shared. But when the unit tests are built,
they are likely creating the FirebaseAuthCredentialsProvider object directly on the stack, like this:
1 // From your search results in firebase_auth_credentials_provider_test.mm
2 FirebaseAuthCredentialsProvider credentials_provider(app, auth);
When credentials_provider.GetToken(...) is called in the test, it's being called on a stack-allocated object. The object isn't managed
by a shared_ptr, so the shared_from_this() call inside GetToken fails.
I need to modify the unit test file (firebase-ios-sdk/Firestore/core/test/unit/credentials/firebase_auth_credentials_provider_test.mm)
to create the provider using std::make_shared, mirroring the production code. This will resolve the build error and ensure the tests are
more accurate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that explanation sounds reasonable.
ncooke3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with Cheryl and Morgan's reviews.
Fix #15281
Gemini's analysis:
After analyzing firebase_auth_credentials_provider_apple.mm, I've identified the source of the crash.
The Problem
Here's the relevant section of the GetToken method in the Firestore SDK:
The C++ object FirebaseAuthCredentialsProvider holds a shared pointer (std::shared_ptr) to its internal state called contents_.
std::weak_ptr. When the async getTokenForcingRefresh method is called, the block is passed along.gone. The if (!contents) { return; } check handles this correctly.
However, the crash isn't happening there. It's happening because of what the block doesn't capture.
The block implicitly captures the C++ completion object by value. This completion is a std::function, which itself holds a reference to the necessary context to continue the Firestore operation. When the FirebaseAuthCredentialsProvider is destroyed, the context that this completion function needs is also destroyed.
The race condition is:
The Solution
This is a bug within the Firestore SDK. The fix is to ensure that the completion object (and any other necessary state) is handled safely within the callback block, likely by capturing self or a shared_ptr to self and checking for its existence before proceeding.
I will now patch the file to fix this issue. I'll modify the block to capture a shared_ptr to the FirebaseAuthCredentialsProvider itself, ensuring it stays alive until the callback completes.