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(storage, ios): do not enumerate on dictionary being mutated #5455

Merged
merged 1 commit into from
Jun 25, 2021

Conversation

danielhover
Copy link
Contributor

Description

Noticed crashes originating from RNFBStorage regarding Collection <__NSDictionaryM: XXXXXXX> was mutated while being enumerated.. This appears to be addressed in some of the other packages already.

Related issues

#3736

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • This is a breaking change;
    • Yes
    • No

@vercel
Copy link

vercel bot commented Jun 25, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/JCiDdtLxrKR5Td4HBbu3gBkNtiph
✅ Preview: https://react-native-firebase-git-fork-hoverinc-fix-st-b46756-invertase.vercel.app

@CLAassistant
Copy link

CLAassistant commented Jun 25, 2021

CLA assistant check
All committers have signed the CLA.

@danielhover danielhover changed the title fix(storage) - dont enumerate on dictionary being mutated fix(storage): do not enumerate on dictionary being mutated Jun 25, 2021
@danielhover danielhover force-pushed the fix-storage-dealloc branch from f9fd465 to 6819dde Compare June 25, 2021 22:26
@mikehardy mikehardy added the Workflow: Needs Review Pending feedback or review from a maintainer. label Jun 25, 2021
@codecov
Copy link

codecov bot commented Jun 25, 2021

Codecov Report

Merging #5455 (72b5f36) into master (0d26af9) will not change coverage.
The diff coverage is n/a.

❗ Current head 72b5f36 differs from pull request most recent head 6819dde. Consider uploading reports for the commit 6819dde to get more accurate results

@@           Coverage Diff           @@
##           master    #5455   +/-   ##
=======================================
  Coverage   74.56%   74.56%           
=======================================
  Files          96       96           
  Lines        4287     4287           
  Branches      921      921           
=======================================
  Hits         3196     3196           
  Misses       1021     1021           
  Partials       70       70           

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Nice catch! Yes this the same as a couple other areas that had the issue, as you correctly linked. I grepped through the whole code base, since this is obviously a recurring bug and after verifying all the other dealloc cases I think you've found and fixed the last one.

Thank you!

@mikehardy mikehardy changed the title fix(storage): do not enumerate on dictionary being mutated fix(storage, ios): do not enumerate on dictionary being mutated Jun 25, 2021
@mikehardy mikehardy merged commit daaa72d into invertase:master Jun 25, 2021
@mikehardy
Copy link
Collaborator

I'm working on a release now but want to fix one other issue first. That means this should be released shortly but in case you want it immediately one of the checks that runs on the PR generates a patch-package .zip which you may download and integrate into your project easily without regard to any possible delay here. Cheers!

@mikehardy mikehardy removed the Workflow: Needs Review Pending feedback or review from a maintainer. label Jun 25, 2021
@genintho
Copy link

@mikehardy Thank you for the quick review, your very thoughtful answer!

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.

4 participants