fix(dynamic-links, android): check for null currentIntent in getInitialLink to avoid crash

[daltonpurnell]

Description

If a null intent is passed to FirebseDynamicLinks.getInstance().getDynamicLink(currentIntent), it crashes with the following error:
Attempt to invoke virtual method 'java.lang.String android.content.Intent.getDataString()' on a null object reference.

To avoid that, I added a check for null currentIntent. Of it's null, resolve the promise and return early (same as the check above it for null currentActivity).

Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • Yes
• My change supports the following platforms;
  • Android (resolves a crash that was only happening on 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
    🔥

[CLAassistant]

All committers have signed the CLA.

[vercel]

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

react-native-firebase-next – ./website_modular

🔍 Inspect: https://vercel.com/invertase/react-native-firebase-next/BtuWpJ9r9MNocVvs9qMeXgCr78Kh
✅ Preview: Canceled

[Deployment for d7c863e canceled]

react-native-firebase – ./

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/7fR685ET1iSZgu5BLd4rkREdLpV6
✅ Preview: https://react-native-firebase-git-fork-daltonpurnell-d-2d0859-invertase.vercel.app

[codecov]

Codecov Report

Merging #5662 (ff403ab) into master (600dee0) will increase coverage by 0.39%.
The diff coverage is 0.00%.

❗ Current head ff403ab differs from pull request most recent head d7c863e. Consider uploading reports for the commit d7c863e to get more accurate results

@@             Coverage Diff              @@
##             master    #5662      +/-   ##
============================================
+ Coverage     53.26%   53.64%   +0.39%     
  Complexity      632      632              
============================================
  Files           208      208              
  Lines         10084    10087       +3     
  Branches       1543     1544       +1     
============================================
+ Hits           5370     5410      +40     
+ Misses         4427     4393      -34     
+ Partials        287      284       -3     

[mikehardy]

This actually looks like an upstream bug. We check for null before de-referencing anything on the Intent, though we do pass it to the underlying firebase-android-sdk API.

We haven't seen this before as a reported issue ever though, and I haven't experienced it in my work projects, under what conditions does it happen? Did it just start happening?

The upstream API is specifically documented as being able to tolerate null (emphasis mine):

https://firebase.google.com/docs/reference/android/com/google/firebase/dynamiclinks/FirebaseDynamicLinks#public-abstract-taskpendingdynamiclinkdata-getdynamiclink-intent-intent

The intent parameter should be the intent that launched the application, or can be null if the intent does not include the dynamic link. A non-null intent is necessary only when the app is launched directly using the dynamic link, such as when using App Links. The app must configure an IntentFilter to override the default capture processing when the link is clicked.

The source code is a bit of comedy though, examine the javadoc (which generated the doc I quote and link above) in contrast to the @NonNull annotation on the parameter 😨

https://github.com/firebase/firebase-android-sdk/blob/82b02af331d4674682bfb90c195da45543b808c3/firebase-dynamic-links/src/main/java/com/google/firebase/dynamiclinks/FirebaseDynamicLinks.java#L82-L102

And indeed, here's your crash:

https://github.com/firebase/firebase-android-sdk/blob/82b02af331d4674682bfb90c195da45543b808c3/firebase-dynamic-links/src/main/java/com/google/firebase/dynamiclinks/internal/FirebaseDynamicLinksImpl.java#L109

Looks like the null check is necessary, but either the documentation or code needs an update.

marking this for merge after CI passes since it's definitely necessary with current + prior (up to 2 years minimum, according to git blame) dynamic links implementation in firebase-android-sdk even if they add null guards internally in response to a ticket I'll open with them [Edit: they already have a ticket open: https://github.com/firebase/firebase-android-sdk/issues/2336]

Thanks for the patch!

[mikehardy]

Oh - until there's a release here (not sure when I'll do the next one - need to see if there's anything else queued up...) you should check the artifacts link from the create test patches action - we generate a patch-package set for every PR, since it's otherwise a real pain to integrate stuff from our monorepo structure. That makes it trivial

[daltonpurnell]

This actually looks like an upstream bug. We check for null before de-referencing anything on the Intent, though we do pass it to the underlying firebase-android-sdk API.

We haven't seen this before as a reported issue ever though, and I haven't experienced it in my work projects, under what conditions does it happen? Did it just start happening?

The upstream API is specifically documented as being able to tolerate null (emphasis mine):

https://firebase.google.com/docs/reference/android/com/google/firebase/dynamiclinks/FirebaseDynamicLinks#public-abstract-taskpendingdynamiclinkdata-getdynamiclink-intent-intent

The intent parameter should be the intent that launched the application, or can be null if the intent does not include the dynamic link. A non-null intent is necessary only when the app is launched directly using the dynamic link, such as when using App Links. The app must configure an IntentFilter to override the default capture processing when the link is clicked.

The source code is a bit of comedy though, examine the javadoc (which generated the doc I quote and link above) in contrast to the @NonNull annotation on the parameter 😨

https://github.com/firebase/firebase-android-sdk/blob/82b02af331d4674682bfb90c195da45543b808c3/firebase-dynamic-links/src/main/java/com/google/firebase/dynamiclinks/FirebaseDynamicLinks.java#L82-L102

And indeed, here's your crash:

https://github.com/firebase/firebase-android-sdk/blob/82b02af331d4674682bfb90c195da45543b808c3/firebase-dynamic-links/src/main/java/com/google/firebase/dynamiclinks/internal/FirebaseDynamicLinksImpl.java#L109

Looks like the null check is necessary, but either the documentation or code needs an update.

marking this for merge after CI passes since it's definitely necessary with current + prior (up to 2 years minimum, according to git blame) dynamic links implementation in firebase-android-sdk even if they add null guards internally in response to a ticket I'll open with them [Edit: they already have a ticket open: https://github.com/firebase/firebase-android-sdk/issues/2336]

Thanks for the patch!

In our case, it's a weird interaction between this library and 2 others that we are using. We have a CodePush.restartApp() call, which reloads the JS. We are also using 'react-native-receive-sharing-intent' library, which checks for received files in a useEffect in the same file where we have the dynamic link implementation.
When the JS is reloaded, the current intent is nullified, but the sharing intent library somehow keeps the initialPromise in ReactNativeFirebaseDynamicLinksModule.java from being nullified, so onHostResume checks that initialPromise and calls getInitialLink, which then crashes the app because it doesn't null-check the currentIntent. It seems like a rare case and I did not see any similar issues open here, but I figured it would be good to have the null-check anyway.

Side note -- I signed the CLA but it still says it's not signed. Not sure what to do about that

[mikehardy]

That makes sense with regard to the reproduction case you have - that certainly does sound rare, and perhaps sort of a bug in it's own right but there's no excuse for a native crash, this patch is good no matter what.

The CLA issue is almost always because the commit email address is different than the github / CLA address. The details link has some info on it but usually you just need to make sure all the email addresses involved are associated as aliases in your github account settings. Sorry for the hassle there, seems a bit much for a small patch but it's still necessary

Thanks!

[mikehardy]

This has some unrelated CI issue, I just re-kicked Android E2E CI check to make sure but it should pass. I have another crash fix queued up now, will release inside an hour or two