fix(storage, ios): resolve listAll promise once and only once on error

[TPXP]

Description

When calling listAll on a forbidden directory, the promise is rejected then RNFirebase tries to resolve it with an empty result, which leads to the following Exception (which I believe would crash the app in production):

Invariant Violation: No callback found with cbID 256 and callID 128 for  RNFBStorageModule.listAll - most likely the callback was already invoked. Args: '[{"nextPageToken":null,"items":[],"prefixes":[]}]'
2020-12-18 11:45:24.318162+0100 ExampleApp[40550:189312] [native] Running surface LogBox ({
    initialProps =     {
    };
    rootTag = 11;
})

The root cause is the lack of a returnon this line of the Firebase iOS SDK : https://github.com/firebase/firebase-ios-sdk/blob/14764b8d60a6ad023d8fa5b7f81d42378d92e6fe/FirebaseStorage/Sources/FIRStorageReference.m#L417

This issue does not occur on Android

Related issues

Upstream issue: firebase/firebase-ios-sdk#7197

Release Summary

• [Storage] FIX: Avoid a crash if listAll fails

Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • Yes
• My change supports the following platforms;
  • Android
  • iOS
• My change includes tests;
  • e2e tests added or updated in packages/\*\*/e2e
  • jest tests added or updated in packages/\*\*/__tests__
• I have updated TypeScript types that are affected by my change.
• This is a breaking change;
  • Yes
  • No

Test Plan

Try it on your project !

Storage rules

rules_version = '2';
service firebase.storage {
  match /b/{bucket}/o {
    match /stuff/{userId} {
      allow read: if false;
    }
  }
}

JS Code

import storage from "@react-native-firebase/storage";
storage().ref('stuff').listAll(console.log, console.warn);

The app will crash without this change on iOS.

---

Think react-native-firebase is great? Please consider supporting the project with any of the below:

• 👉 Star this repo on GitHub ⭐️
• 👉 Follow React Native Firebase and Invertase on Twitter

[vercel]

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/82buvjx4l
✅ Preview: https://react-native-firebase-git-fix-list-all-error.invertase.vercel.app

[codecov]

Codecov Report

Merging #4688 (2dc6f70) into master (fa28299) will increase coverage by 0.44%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4688      +/-   ##
==========================================
+ Coverage   88.61%   89.04%   +0.44%     
==========================================
  Files         109      109              
  Lines        3712     3712              
  Branches      347      347              
==========================================
+ Hits         3289     3305      +16     
+ Misses        381      365      -16     
  Partials       42       42              

[mikehardy]

Whoa! Very interesting, Do you have an upstream issue logged with the sdk team? I'm okay in principle with workarounds and will review, but only if upstream is aware and I didn't see an issue ref. If I missed it sorry

[TPXP]

I raised an issue with the upstream team although it might make sense to return the items the query was able to fetch as well as the error on iOS (which is something we can't do with promises in JS).

In any case, I didn't touch the Android code, this PR aligns behaviors on both platforms (taking Android as a reference).

[mikehardy]

thank you so much for adding a test to support this change!

[mikehardy]

Great! I subscribed to the upstream issue and replaced the code reference in your workaround with the issue link. I also fixed a blank space nit

Crashing in native code is never acceptable so I'm positive on a workaround now that it is being tracked upstream, and this looks like a good workaround.

The test coverage is wonderful, I personally appreciate that as it gives me confidence when doing releases, thank you.

[mikehardy]

Android E2E is having issues but they are unrelated, and I'm tracking those separately, this is good to merge

[mikehardy]

@TPXP deploying as v10.3.1 as I type this, cheers

[TPXP]

Wow, that was fast. Thanks for your responsiveness! <3

[mikehardy]

All the work over the last few months and years of developing test coverage, the last few months stabilizing the CI tests and polishing the release system means it is now very easy to do one of my favorite things: crush crashes :-). Always my pleasure to help crush a native crash. Thanks for the PR.

[mikehardy]

Upstream just closed with a PR, tagged for firebase-ios-sdk 7.4.0