Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix a Firestore transaction crash if getDocument was called after terminateWithCompletion. #8760

Merged
merged 3 commits into from
Oct 8, 2021
Merged

Fix a Firestore transaction crash if getDocument was called after terminateWithCompletion. #8760

merged 3 commits into from
Oct 8, 2021

Conversation

dconeybe
Copy link
Contributor

@dconeybe dconeybe commented Oct 7, 2021

Fix a crash if [FIRTransaction getDocument] was called after [FIRFirestore terminateWithCompletion], and add a test for it.

The crash error message looked like this:

libc++abi: terminating with uncaught exception of type std::__1::bad_weak_ptr: bad_weak_ptr

The crash was due to terminateWithCompletion triggering the reference count of a shared_ptr to the Datastore object to reach zero, causing the object to be deleted. The Transaction class, however, stored a raw pointer to this Datastore object and calling [FIRTransaction getDocument] attempted to use this dangling pointer.

The fix was to change the Datastore* instance variable to a std::weak_ptr<Datastore> in the Transaction class and gracefully handle the situation where it got nulled out.

This bug also affects the C++ SDK and the Unity SDK and those SDKs will get this fix automatically once they upgrade their dependencies. Googlers can see b/201415845 for details.

@dconeybe
Fix a crash if [FIRTransaction getDocument] was called after `[FIRF…
fe80d56 
…irestore terminateWithCompletion]` and add a test for it.
@dconeybe dconeybe added this to the 8.9.0 - M106 milestone Oct 7, 2021
@dconeybe dconeybe self-assigned this Oct 7, 2021
@google-cla google-cla bot added the cla: yes label Oct 7, 2021
@google-oss-bot
Copy link

google-oss-bot commented Oct 7, 2021 •
edited

Binary Size Report

Affected SDKs

  • FirebaseFirestore

    Type Base (a5859f9) Head (0273aef) Diff
    firebase-ios-sdk 4.96 MB 4.96 MB -16 B (-0.0%)

Test Logs

@google-oss-bot
Copy link

google-oss-bot commented Oct 7, 2021 •
edited

Coverage Report

Affected SDKs

  • FirebaseFirestore-iOS-FirebaseFirestore.framework

    SDK overall coverage changed from 87.64% (a5859f9) to 87.68% (0273aef) by +0.04%.

    Filename Base (a5859f9) Head (0273aef) Diff
    leveldb_key.cc 96.75% 97.99% +1.24%
    task.cc 93.04% 94.78% +1.74%
    transaction.cc 91.03% 89.17% -1.86%

Test Logs

schmidt-sebastian
schmidt-sebastian approved these changes Oct 7, 2021
View reviewed changes
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the test. LGTM.

@dconeybe dconeybe merged commit 58c7566 into master Oct 8, 2021
@dconeybe dconeybe deleted the dconeybe/TransactionGetCrashFixAfterTerminate branch October 8, 2021 14:31
@firebase firebase locked and limited conversation to collaborators Nov 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@schmidt-sebastian schmidt-sebastian schmidt-sebastian approved these changes

Assignees

@dconeybe dconeybe

Labels
api: firestore cla: yes
Projects
None yet
Milestone
8.9.0 - M106
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dconeybe @google-oss-bot @schmidt-sebastian