Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add check to avoid crashes when currentActivity is null #846

Merged
merged 8 commits into from
Apr 18, 2022

Conversation

charliecruzan-stripe
Copy link
Collaborator

@charliecruzan-stripe charliecruzan-stripe commented Mar 15, 2022

Summary

We weren't always appropriately null-checking currentActivity, and the typecast could result in crashes. We now resolve with a consistent and useful error telling the user they can retry the method.

Motivation

closes #844 and I'm sure other instances of crashes on Android
closes #881

Testing

  • I tested this manually
  • I added automated tests

Documentation

Select one:

  • I have added relevant documentation for my changes.
  • This PR does not result in any developer-facing changes.

@@ -172,7 +172,9 @@ class StripeSdkModule(private val reactContext: ReactApplicationContext) : React
private val googlePayReceiver: BroadcastReceiver = object : BroadcastReceiver() {
override fun onReceive(context: Context?, intent: Intent) {
if (intent.action == ON_GOOGLE_PAY_FRAGMENT_CREATED) {
googlePayFragment = (currentActivity as AppCompatActivity).supportFragmentManager.findFragmentByTag("google_pay_launch_fragment") as GooglePayFragment
(currentActivity as? AppCompatActivity)?.let {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was really surprised that you have to check the current activity for null everywhere, even from @ReactMethod, but I didn't really see another way.

In order to prevent this type of error in the future I would recommend:

class StripeSdkModule ...{
...
  // Set currentActivity so it uses this instead of the base class `getCurrentActivity()`
  // this will give you type safety checking and convert it to an appCompat activity
  val currentActivity: AppCompatActivity?
    get() = (super.getCurrentActivity() as? AppCompatActivity)
...
    private val googlePayReceiver: BroadcastReceiver = object : BroadcastReceiver() {
        override fun onReceive(context: Context?, intent: Intent) {
          if (intent.action == ON_GOOGLE_PAY_FRAGMENT_CREATED) {
            currentActivity?.let {
...
}

It would also be nice to see some enforcement in the code that if you can't get the appCompat activity that a promise is fired to return a result.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the feedback! created a helper method for getting the activity or resolving the promise. It's not the perfect solution since someone can still call currentActivity, but it does avoid a lot of repeated code and hopefully anyone working here would see the current pattern for getting the activity 🤞

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michelleb-stripe do you approve the most recent changes? I'll need to rebase but since #879 contains substantial changes to the same file, i'd rather wait till after that lands to rebase (to avoid duplicating the work)

@charliecruzan-stripe
Copy link
Collaborator Author

closes #881

@charliecruzan-stripe charliecruzan-stripe force-pushed the charliecruzan-currentActivity-nullsafety branch from d11edcb to 369587b Compare April 18, 2022 14:18
@charliecruzan-stripe charliecruzan-stripe force-pushed the charliecruzan-currentActivity-nullsafety branch from 369587b to 39a63e1 Compare April 18, 2022 16:26
@charliecruzan-stripe charliecruzan-stripe merged commit e27d3f0 into master Apr 18, 2022
@charliecruzan-stripe charliecruzan-stripe deleted the charliecruzan-currentActivity-nullsafety branch April 18, 2022 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants