fix(admob,android): add null checks for interstitialAd object

[aguglie]

Description

Hi, just got a null-pointer exception while using AdMob in my app:

java.lang.NullPointerException: 
  at io.invertase.firebase.admob.ReactNativeFirebaseAdMobInterstitialModule.lambda$interstitialShow$1 (ReactNativeFirebaseAdMobInterstitialModule.java:132)
  at io.invertase.firebase.admob.-$$Lambda$ReactNativeFirebaseAdMobInterstitialModule$HC0J2W104AWtZIk60UVqQO1y4sE.run (Unknown Source:6)
  at android.os.Handler.handleCallback (Handler.java:888)
  at android.os.Handler.dispatchMessage (Handler.java:100)
  at android.os.Looper.loop (Looper.java:213)
  at android.app.ActivityThread.main (ActivityThread.java:8178)
  at java.lang.reflect.Method.invoke (Native Method)
  at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run (RuntimeInit.java:513)
  at com.android.internal.os.ZygoteInit.main (ZygoteInit.java:1101)

I think this should sort out the issue although it was not tested.

Related issues

Found no related issue.

Release Summary

Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • [x ] Yes
• My change supports the following platforms;
  • [x ] 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
  • [x ] No

Test Plan

I simply ran the yarn tests:jest and compiled with yarn tests:android:build 🔥

---

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/54td5tojh
✅ Preview: https://react-native-firebase-git-fix-interstitial-nullpointer.invertase.vercel.app

[CLAassistant]

All committers have signed the CLA.

[codecov]

Codecov Report

Merging #4670 (ac6cd40) into master (111a65f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4670   +/-   ##
=======================================
  Coverage   24.06%   24.06%           
=======================================
  Files         109      109           
  Lines        3712     3712           
  Branches      347      347           
=======================================
  Hits          893      893           
  Misses       2611     2611           
  Partials      208      208           

[mikehardy]

@Guglio95 thank you so much for posting this! I'm traveling right now and I want to give this a deeper look - sometimes a null pointer exception has the easy + correct solution of just catching it, but sometimes it means there is a much deeper assumption that is being violated and I need to study it to see which case it is :-)

So it might be about a week however please don't let me get in the way of your project - if you have not already integrated patch-package you definitely should do so, and you may easily + maintainably include this in your project as a patch. If you click on the patch-package task here from the PR https://github.com/invertase/react-native-firebase/pull/4670/checks?check_run_id=1539655716 you'll see an artifacts link and in there we generate patch-package format patches for people to use while we make sure a PR is good

Thanks for your patience!

[mikehardy]

Very interesting - I can't understand how this would happen unless show was called before load, since the InterstitialAd object is pushed into the array here

react-native-firebase/packages/admob/android/src/main/java/io/invertase/firebase/admob/ReactNativeFirebaseAdMobInterstitialModule.java

Line 116 in 9bd0725

interstitialAdArray.put(requestId, interstitialAd);

on the UI thread even, and then accessed on the UI thread which should avoid any race conditions even. So this should be impossible, and on inspection I don't see any deeper issues.

However, I don't see any harm in a null check here and it doesn't seem like we're hiding any deep underlying problem (at least as I understand it) so on the off-chance we catch a crash, I can merge this and hopefully you've got one less crash in your crash stats. Look for a 10.2.1 (or higher) to have it until then patch-package can get it into your project

Thanks @Guglio95

[aguglie]

Hi @mikehardy :)

Very interesting - I can't understand how this would happen unless show was called before load, since the InterstitialAd object is pushed into the array here

Do you think there may be something wrong with the JavaScript code I'm using? I'll double check it! Maybe there's some race condition there.

Thanks Mike 👍

[mikehardy]

@Guglio95 yeah - all I can think is that somehow show was called before load - maybe in javascript it's a failure to await and somehow / unluckily / race-y the show call then makes it across the JS bridge before the load call, or maybe they are actually mis-ordered somehow, I'm not sure. But that seems to be what is going on to cause this stack. Always bear in mind that I may have an incorrect assumption too, that's just my hypothesis from reading the code

[aguglie]

I'll check my code, thank you @mikehardy ! 😄