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.

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

fix(firestore): Prevent iOS crash caused by concurrency issue #772

Merged
merged 5 commits into from
Dec 16, 2024

Conversation

TomCarey
Copy link
Contributor

@TomCarey TomCarey commented Dec 11, 2024

Close #771

Wraps the listenerRegistrationMap in an actor to manage the reading and writing in a thread-safe way. It is necessary because addCollectionSnapshotListener and addCollectionGroupSnapshotListener dispatch a Task in order to build the constraints but then immediately write to the listenerRegistrationMap within that Task.

Pull request checklist

Please check if your PR fulfills the following requirements:

  • The changes have been tested successfully.
  • A changeset has been created (npm run changeset).
  • I have read and followed the pull request guidelines.

.changeset/eleven-hornets-joke.md Outdated Show resolved Hide resolved
packages/firestore/ios/Plugin/FirebaseFirestore.swift Outdated Show resolved Hide resolved
packages/firestore/ios/Plugin/FirebaseFirestore.swift Outdated Show resolved Hide resolved
Co-authored-by: Robin Genz <mail@robingenz.dev>
Co-authored-by: Robin Genz <mail@robingenz.dev>
@TomCarey
Copy link
Contributor Author

TomCarey commented Dec 15, 2024

@robingenz I think there is an error introduced in your code suggestion. The dispatch queue of the capacitor bridge is internal so we can't dispatch work onto that queue. The event listeners of CAPPlugin are stored in a dictionary on that bridge queue. By calling super in the completion block we are running removeEventListeners in a different thread so there is a race condition where we might unsafely manipulate that dictionary. I'm not sure there is a simple solution here. It seems like it would be nice for capacitor to expose that queue. In theory though it would probably be "safer" to run the code in the way that I had it before where we didn't call super.removeAllListeners but instead extract that behaviour like we had it before.

@robingenz
Copy link
Member

@TomCarey You are right. Please revert this change. We'll try your approach first.

@TomCarey
Copy link
Contributor Author

Thanks @robingenz I've reverted that now.

Copy link
Member

@robingenz robingenz left a comment

Choose a reason for hiding this comment

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

Thank you!

Copy link

pkg-pr-new bot commented Dec 16, 2024

Open in Stackblitz

@capacitor-firebase/analytics

npm i https://pkg.pr.new/capawesome-team/capacitor-firebase/@capacitor-firebase/analytics@772

@capacitor-firebase/app

npm i https://pkg.pr.new/capawesome-team/capacitor-firebase/@capacitor-firebase/app@772

@capacitor-firebase/app-check

npm i https://pkg.pr.new/capawesome-team/capacitor-firebase/@capacitor-firebase/app-check@772

@capacitor-firebase/authentication

npm i https://pkg.pr.new/capawesome-team/capacitor-firebase/@capacitor-firebase/authentication@772

@capacitor-firebase/crashlytics

npm i https://pkg.pr.new/capawesome-team/capacitor-firebase/@capacitor-firebase/crashlytics@772

@capacitor-firebase/firestore

npm i https://pkg.pr.new/capawesome-team/capacitor-firebase/@capacitor-firebase/firestore@772

@capacitor-firebase/functions

npm i https://pkg.pr.new/capawesome-team/capacitor-firebase/@capacitor-firebase/functions@772

@capacitor-firebase/messaging

npm i https://pkg.pr.new/capawesome-team/capacitor-firebase/@capacitor-firebase/messaging@772

@capacitor-firebase/performance

npm i https://pkg.pr.new/capawesome-team/capacitor-firebase/@capacitor-firebase/performance@772

@capacitor-firebase/remote-config

npm i https://pkg.pr.new/capawesome-team/capacitor-firebase/@capacitor-firebase/remote-config@772

@capacitor-firebase/storage

npm i https://pkg.pr.new/capawesome-team/capacitor-firebase/@capacitor-firebase/storage@772

commit: 48a9593

@robingenz robingenz merged commit f1f07aa into capawesome-team:main Dec 16, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: iOS swift concurrency EXC_BAD_ACCESS in Firestore addCollectionSnapshotListener
2 participants