fix(app): convert NativeFirebaseError.getStackWithMessage to static to fix crash

[BlaShadow]

Description

After receiving an error from AdMob, the framework was trying to create a NativeFirebaseError but it was not able to call getStackWithMessage from the constructor.

Related issues

Release Summary

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

---

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

[CLAassistant]

All committers have signed the CLA.

[mikehardy]

This seems okay to me, but asking for a second look as I come from a Java-based background, so javascript-this to me takes a lot of mental effort and I might miss something. My main question (for @Salakar perhaps...) is, how did https://github.com/invertase/react-native-firebase/pull/3751/files#diff-d095ca1ef9393986b92cbb22e7790ba7L62 ever work then?

According to https://www.w3schools.com/JS/js_object_constructors.asp - the 'this' keyword in the constructor has no value yet so should have never worked, and (if I understand correctly) an alternate (not recommended! but possible) approach would be to define the getStackWithMessage method in the constructor and attach it to this, then you could call it.

[mikehardy]

I looked at this again and actually chatted with a colleague here and we can't figure out what the original issue was exactly or why this change would fix it.

We will need to see a simple App.js that reproduces the problem before we can move forward

[thomasonweb]

Hi @mikehardy

We are seeing the same issue in the Google Play crashes. I'm unable to reproduce it myself but it happens quite frequently. Currently our top crash.

I'm seeing "undefined is not a function" on this line:

this.stack = this.getStackWithMessage(NativeFirebaseError: ${this.message});

in app/lib/internal/NativeFirebaseError.js

As I'm unable to reproduce it myself I can't really pinpoint or debug why it is happening. Our team is still investigating so if we would find the cause I'm happy to share it. However, turning them static as @BlaShadow is proposing would most likely resolve them.

[mikehardy]

@thomasonweb (or @BlaShadow ) If you implement this locally via patch-package and report success, post the exact patch up (so I can compare to verify - as we don't have a solid reproduction, right?) then I could reopen and merge this

I love fixing crash bugs but obviously I need to be careful chasing things without good reproductions...

[thomasonweb]

Hi @mikehardy

To give some context: we used to be on an older version of Firebase and a few weeks ago we started upgrading to v8.3.0 which was now released in production. Because we upgraded we started seeing this crash happening. I've applied a patch where we made the function static and did a rollout into production. I can confirm the crash is gone but we were never able to find what caused it as we could not reproduce it ourselves.

I've applied the patch on v8.3.0 but the file looks similar in v10.

@react-native-firebase+app+8.3.0.txt

[BlaShadow]

The first time I got this error my Admob was kind of new and it was failing to retrieve ads (throwing an error). I don't know how to reproduce it now, because the account it's not failing anymore.

[mikehardy]

So strange - thanks for reporting back. As mentioned I do love fixing crash bugs but I am trying to be careful here. I'll investigate and see if I can get this in, it does seem like at worst it is harmless, and at best if could fix something.

[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/l8ji4dlq5
✅ Preview: Failed

[codecov]

Codecov Report

Merging #3751 (bbca8de) into master (d98b80e) will increase coverage by 24.17%.
The diff coverage is 86.60%.

@@             Coverage Diff             @@
##           master    #3751       +/-   ##
===========================================
+ Coverage   64.81%   88.98%   +24.17%     
===========================================
  Files         114      109        -5     
  Lines        3788     3710       -78     
  Branches      275      346       +71     
===========================================
+ Hits         2455     3301      +846     
+ Misses       1228      368      -860     
+ Partials      105       41       -64     

[mikehardy]

This appears to pass my local testing (I pulled the branch and tested it)
I rebased the PR and pushed the result to a fresh PR to make sure it got a clean CI run from the most up to date code and will merge from there #4619