fix(admob, android): unity ads require Activity Context

[zyzo]

Description

Fix Admob crash from UnityAds with Admob Mediation.

Seems like a regression from #1892

Fixes #1792

Related issues

#1792
#1892

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

[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/CnSDkVA8ff161FJRamQrrJe1Xzjn
✅ Preview: https://react-native-firebase-git-fork-betonyou-master-invertase.vercel.app

[CLAassistant]

All committers have signed the CLA.

[zyzo]

@mikehardy let me know if there is need for special logging in case of null Activity.

[codecov]

Codecov Report

Merging #4921 (2f570aa) into master (fa3d260) will increase coverage by 0.43%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4921      +/-   ##
==========================================
+ Coverage   88.50%   88.93%   +0.43%     
==========================================
  Files         109      109              
  Lines        3721     3721              
  Branches      348      348              
==========================================
+ Hits         3293     3309      +16     
+ Misses        385      369      -16     
  Partials       43       43              

[mikehardy]

Thanks for this! I think rather than a regression this was a fix that landed on the v5 branch but never made it to the main branch! That's a shame but at least you are getting it now.

More than anything we need the CLA signed, we can't merge until that's done. The details link for the CLA check in the block of checks on the PR has more info to help you get it done

[mikehardy]

Hi there - thank you for your patience!
This looks close, and while I think it would work, may I ask you to reshape it just a bit?

Specifically, now that I look closely it appears that getCurrentActivity being not-null is actually a requirement for the whole thing to work. So the change should not really be so concerned about whether it is null or not, rather since we use the Activity in three places now it should actually store it as a temporary variable, do the null check once, then use the temp variable in all three spots.

Specifically

• the import of Context should not be necessary any more but you may need to import Activity
• the first if (getCurrentActivity() == null) check should be changed to (in pseudocode)

Activity currentActivity = getCurrentActivity();
if (currentActivity == null) { /* the existing error handling here, all the same */ }
currentActivity.runOnUiThread(() -> {
  // now just use currentActivity in place of getApplicationContext())
}

Unless I am missing something I think this will make for a very clean implementation and is even smaller than the proposed change.

What do you think?

[mikehardy]

for the avoidance of doubt: I'm +1 on the change idea in general though of course, looking forward to merging a fix here

[mikehardy]

@zyzo just in case you didn't see this ⬆️

[zyzo]

@mikehardy sure, I'll see what I can do tomorrow !

[zyzo]

@mikehardy ☝️

[mikehardy]

This looks really clean to me now, thank you!

[mikehardy]

As soon as CI finishes (it failed on the iOS e2e tests, which obviously has nothing to do with this PR, it should go green in a few minutes now that I've restarted it) I'll merge this and release

Thanks @zyzo !

[mikehardy]

Releasing as 10.8.1 right now, thanks!