Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[In_App_Purchase] Avoids possible NullPointerException with background registrations. #2014

Merged
mklim merged 10 commits into from
Aug 29, 2019
Merged

[In_App_Purchase] Avoids possible NullPointerException with background registrations. #2014

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
52cd0a0
Do not register plugin if Activity is null.
juliocbcotta Aug 26, 2019
da055d5
Revert unrelated changes.
juliocbcotta Aug 26, 2019
bcf733a
Adds UnitTest for registration.
juliocbcotta Aug 26, 2019
cdaea15
Allow background registration and adds check for Activity nullability…
juliocbcotta Aug 27, 2019
65a6a1a
update tests and changelog.
juliocbcotta Aug 27, 2019
346dd51
Update packages/in_app_purchase/android/src/main/java/io/flutter/plug…
juliocbcotta Aug 27, 2019
ee54c28
Update packages/in_app_purchase/CHANGELOG.md
juliocbcotta Aug 27, 2019
f7ef931
Merge branch 'master' into plugin/in_app_purchase_null_fix
juliocbcotta Aug 28, 2019
7db64ef
Merge branch 'plugin/in_app_purchase_null_fix' of github.com:BugsBunn…
juliocbcotta Aug 28, 2019
78f82ff
Merge branch 'master' into plugin/in_app_purchase_null_fix
juliocbcotta Aug 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/in_app_purchase/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 0.2.1+1

* Android: Do not register if Activity is null.

## 0.2.1

* iOS: Add currencyCode to priceLocale on productDetails.
Expand Down
32 changes: 20 additions & 12 deletions ..._purchase/android/src/main/java/io/flutter/plugins/inapppurchase/InAppPurchasePlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import android.app.Activity;
import android.content.Context;
import android.util.Log;
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import androidx.annotation.VisibleForTesting;
import com.android.billingclient.api.BillingClient;
Expand Down Expand Up @@ -67,10 +68,13 @@ static final class MethodNames {

/** Plugin registration. */
public static void registerWith(Registrar registrar) {
final Activity activity = registrar.activity();
if (activity == null) {
return;

@mklim mklim Aug 26, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 juliocbcotta Aug 26, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 mklim Aug 26, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 juliocbcotta Aug 27, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

}
final MethodChannel channel =
new MethodChannel(registrar.messenger(), "plugins.flutter.io/in_app_purchase");
channel.setMethodCallHandler(
new InAppPurchasePlugin(registrar.context(), registrar.activity(), channel));
channel.setMethodCallHandler(new InAppPurchasePlugin(registrar.context(), activity, channel));
}

public InAppPurchasePlugin(Context context, Activity activity, MethodChannel channel) {
Expand All @@ -80,33 +84,37 @@ public InAppPurchasePlugin(Context context, Activity activity, MethodChannel cha
}

@Override
public void onMethodCall(MethodCall call, Result result) {
public void onMethodCall(MethodCall call, @NonNull Result result) {
Comment thread
mklim marked this conversation as resolved.
Outdated
final String skuType = call.argument("skuType");
switch (call.method) {
case MethodNames.IS_READY:
isReady(result);
break;
case MethodNames.START_CONNECTION:
startConnection((int) call.argument("handle"), result);
final Integer handle = call.argument("handle");
startConnection(handle, result);
break;
case MethodNames.END_CONNECTION:
endConnection(result);
break;
case MethodNames.QUERY_SKU_DETAILS:
querySkuDetailsAsync(
(String) call.argument("skuType"), (List<String>) call.argument("skusList"), result);
final List<String> skusList = call.argument("skusList");
querySkuDetailsAsync(skuType, skusList, result);
break;
case MethodNames.LAUNCH_BILLING_FLOW:
launchBillingFlow(
(String) call.argument("sku"), (String) call.argument("accountId"), result);
final String sku = call.argument("sku");
final String accountId = call.argument("accountId");
launchBillingFlow(sku, accountId, result);
break;
case MethodNames.QUERY_PURCHASES:
queryPurchases((String) call.argument("skuType"), result);
queryPurchases(skuType, result);
break;
case MethodNames.QUERY_PURCHASE_HISTORY_ASYNC:
queryPurchaseHistoryAsync((String) call.argument("skuType"), result);
queryPurchaseHistoryAsync(skuType, result);
break;
case MethodNames.CONSUME_PURCHASE_ASYNC:
consumeAsync((String) call.argument("purchaseToken"), result);
final String purchaseToken = call.argument("purchaseToken");
consumeAsync(purchaseToken, result);
break;
default:
result.notImplemented();
Expand All @@ -121,7 +129,7 @@ public void onMethodCall(MethodCall call, Result result) {
this.activity = null;
}

private void startConnection(final int handle, final Result result) {
private void startConnection(final Integer handle, final Result result) {
if (billingClient == null) {
billingClient = buildBillingClient(context, channel);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/in_app_purchase/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: in_app_purchase
description: A Flutter plugin for in-app purchases. Exposes APIs for making in-app purchases through the App Store and Google Play.
author: Flutter Team <flutter-dev@googlegroups.com>
homepage: https://github.com/flutter/plugins/tree/master/packages/in_app_purchase
version: 0.2.1
version: 0.2.1+1


dependencies:
Expand Down