[In_App_Purchase] Avoids possible NullPointerException with background registrations.

[juliocbcotta]

Description

This PR delays the get of a reference of a Activity instance and raises an error in case of a invalid access to the method that uses it.

Related Issues

flutter/flutter#38692

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

• I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
• My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
• All existing and new tests are passing.
• I updated/added relevant documentation (doc comments with ///).
• The analyzer (flutter analyze) does not report any problems on my PR.
• I read and followed the [Flutter Style Guide].
• The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
• I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
• I updated CHANGELOG.md to add a description of the change.
• I signed the [CLA].
• I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

• Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
• No, this is not a breaking change.

[mklim]

Thanks for finding this issue and submitting a fix!

Could you also add a test for this? We don't have it wired up to CI yet unfortunately, but we do have JUnit tests that we've been running manually in packages/in_app_purchase/example/android/app/src/test/. I've been running them by opening packages/in_app_purchase/example/android in Android Studio, and then executing them from there.

[mklim]

I think for devs in this state it's better to throw a RuntimeException with an error message and crash than to silently fail, even in release mode. This case means that none of the functions of the plugin are going to actually work, so silently returning is slipping the app into an unrecoverable error state. None of the API calls will work, and there isn't going to be a clear sign or reason why from the app developer's side. What do you think?

[juliocbcotta]

I used the same strategy used here. It is of my understanding that the plugin will get a second change to register itself when an Activity is available. Raising an exception here would make that impossible to to flutter developers to handle. When a background or foreground registration happens it execute a "register all command" . That can be seem here and in foreground here. In other words, it would make impossible to use Alarm Manager Plugin and In App Purchase plugin in the same project.

[mklim]

Got it, thanks for the clarification. I've done some digging on my end to understand this better.

So from my testing IFF the user sets up a custom Application.java with android_alarm_manager, like in the example app, the callbacks always try to register with a null activity. The Flutter app using these periodic callbacks sets up the plugins using MainActivity in the typical way in its standard isolate, and can make calls like expected. However the callbacks are always registered with a null activity, regardless of whether or not the plugin is in the foreground when the callbacks fire.

Throwing an exception like I originally suggested definitely doesn't make sense, because this registration happens for all plugins whether or not they're actually used in the callback. So that would make this unusable in conjunction with the plugin, like you mentioned.

This return also introduces some potential new runtime errors IFF the callbacks try to use the IAP API at all in their callbacks, since they always execute with a background isolate and null activity, but not all of the IAP methods actually need an activity at all. On some more thought about this I think that's a regression we probably want to avoid. The critical launchBillingFlow one requires an Activity, but I can see a dev using only unrelated methods like queryPurchases in their background callbacks instead.

Sorry about the back and forth. With that in mind, I think a better approach would be to actually accept registration when activity is null, but to mark InAppPurchasePlugin#activity as @Nullable and return result.error() in launchBillingFlow if it's called while activity is null. This is similar to how we enforce that BillingClient has been set correctly in API calls that require it with the InAppPurchase#billingClientError helper method. I think the same "UNAVAILABLE" tag but with a different message about the activity would make sense. What do you think?

[juliocbcotta]

There is no problem in this back and forth. We are evolving in the solution. :-)
I hadn't thought about enabling the plugin usage in background, nice catch.
I will update the code/tests as needed.

[mklim]

Thanks again for the contribution! This is a robust solution.

[mklim]

@BugsBunnyBR I'm not sure why, but the wait_for_emulator step is consistently failing on this PR. I don't see how it would be related, but it's not currently failing at the flutter/plugins tip of tree. Would you mind merging in the latest changes from master into this branch?

ETA: Talked offline with @collinjackson and confirmed that merging in master will fix this failure for sure.

[juliocbcotta]

@BugsBunnyBR I'm not sure why, but the wait_for_emulator step is consistently failing on this PR. I don't see how it would be related, but it's not currently failing at the flutter/plugins tip of tree. Would you mind merging in the latest changes from master into this branch?

ETA: Talked offline with @collinjackson and confirmed that merging in master will fix this failure for sure.

I updated the master branch, but the tests are still failing. I tried to run ./script/incremental_build.sh drive-examples in my local machine, but a GoogleMaps test failed instead. Would I have to set any config locally? Any ideas?

[juliocbcotta]

Thanks doc!