[in_app_purchase] Android: fix potential crash when casting in v1 embedding. (Trivial) #2585

cyanglaz · 2020-03-06T19:14:11Z

Description

Fix a potential crash when we cast context() to Application in the v1 embedding code.

Related Issues

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.

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.

jmagman · 2020-03-06T22:17:53Z

...app_purchase/android/src/main/java/io/flutter/plugins/inapppurchase/InAppPurchasePlugin.java

+    if (registrar.context().getApplicationContext() != null
+        && registrar.context().getApplicationContext() instanceof Application) {


Is it bad if it's null or not an Application? Does that need to be handled?

if either of those situations happens, we are not able to get the application, which makes us no way to register life cycle callbacks. We kind of just bailing here.
I'm not sure if there would be a way to handle that situation in V1 embedding, but we definitely don't want to crash here.

Do we know how people are getting in this state?

I am not entirely sure. @mklim or @matthew-carroll might have more ideas on this.

I believe that you can just say registrar.context().getApplicationContext(). I don't think that context() can return null, and I don't think that getApplicationContext() can return null

BTW, do you know why you need an Application? What are the Application methods that are being called that are not defined on a Context?

ok. I just did some search and it seems to be the case. I'll update the fix.

I believe .registerActivityLifecycleCallbacks is only in Application?

That looks right.

To explain this a little more, there is a difference between an ApplicationContext (can be cast to Application) and an ActivityContext (can be cast to Activity) even though they're both Contexts. Generally it's hard to tell the difference but occasionally it's critical. The Activity Context is tied to the Activity lifecycle so can be invalidated much easier. registrar.context() returns an activity context if an activity has been registered with the Engine and an application context otherwise. getApplicationContext() should always be castable to an application like @matthew-carroll mentioned.

…crash_fix

jmagman

WorkspaceSettings.xcsettings looks right. Defer to @matthew-carroll for the java change.

matthew-carroll · 2020-03-07T01:16:17Z

...mple/android/app/src/test/java/io/flutter/plugins/inapppurchase/InAppPurchasePluginTest.java

+  }
+
+  @Test
+  public void registerWith_doNotCrashWhenApplicationContextIsTypeApplication() {


Is this test necessary? We should probably avoid testing situations that can't happen, because it might indicate to readers that it can happen...

Isn't context.getApplicationContext() always going to return an application? This looks like the standard case to me.

Though on a second look, maybe a more meaningful test here would be to return an Activity from registrar.context() and make sure that that doesn't crash?

@mklim Sounds good, I will update the test.

mklim

LGTM

mklim · 2020-03-07T01:21:34Z

...mple/android/app/src/test/java/io/flutter/plugins/inapppurchase/InAppPurchasePluginTest.java

+  }
+
+  @Test
+  public void registerWith_doNotCrashWhenApplicationContextIsTypeApplication() {


Isn't context.getApplicationContext() always going to return an application? This looks like the standard case to me.

mklim · 2020-03-07T01:24:40Z

...app_purchase/android/src/main/java/io/flutter/plugins/inapppurchase/InAppPurchasePlugin.java

  public static void registerWith(Registrar registrar) {
    InAppPurchasePlugin plugin = new InAppPurchasePlugin();
    plugin.setupMethodChannel(registrar.activity(), registrar.messenger(), registrar.context());
-    ((Application) registrar.context())


Just for a little more info in case this doesn't make sense, the bug here is that sometimes registrar.context() will return an Activity context instead of an Application context if it's available and this case will fail. My understanding is that it's the typical for it to do this so I'm actually surprised that this was routinely working before.

this was newly added in my new patch so it might be that my patch broke it. However, when I tested locally, it was working. I wonder why activity was not available when i was testing it.

cyanglaz · 2020-03-09T17:34:09Z

Updated the tests based on discussion. Will merge on green.

cyanglaz · 2020-03-09T19:38:54Z

Landing on tree red as it is going to fix a ci error.

…edding. (flutter#2585)

fix potential crash

5b437f5

cyanglaz requested a review from mklim as a code owner March 6, 2020 19:14

googlebot added the cla: yes label Mar 6, 2020

cyanglaz changed the title ~~[in_app_purchase] Android: fix potential crash when casting in v1 embedding.~~ [in_app_purchase] Android: fix potential crash when casting in v1 embedding. (Trivial) Mar 6, 2020

Chris Yang added 3 commits March 6, 2020 11:42

tests

fa6804a

remove legacy build setting

2423162

Update CHANGELOG.md

216ee9b

jmagman reviewed Mar 6, 2020

View reviewed changes

Chris Yang added 2 commits March 6, 2020 16:18

remove the null check

a3abc72

Merge branch 'iap_crash_fix' of github.com:cyanglaz/plugins into iap_…

80d6869

…crash_fix

jmagman approved these changes Mar 7, 2020

View reviewed changes

matthew-carroll reviewed Mar 7, 2020

View reviewed changes

mklim approved these changes Mar 7, 2020

View reviewed changes

mklim reviewed Mar 7, 2020

View reviewed changes

update test

365b46a

cyanglaz merged commit 43d9b5a into flutter:master Mar 9, 2020

cyanglaz deleted the iap_crash_fix branch March 9, 2020 19:39

EdwinRomelta pushed a commit to EdwinRomelta/plugins that referenced this pull request Jun 11, 2020

[in_app_purchase] Android: fix potential crash when casting in v1 emb…

f61e608

…edding. (flutter#2585)

FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020

[in_app_purchase] Android: fix potential crash when casting in v1 emb…

d92c273

…edding. (flutter#2585)

		if (registrar.context().getApplicationContext() != null
		&& registrar.context().getApplicationContext() instanceof Application) {

[in_app_purchase] Android: fix potential crash when casting in v1 embedding. (Trivial) #2585

[in_app_purchase] Android: fix potential crash when casting in v1 embedding. (Trivial) #2585

Uh oh!

Conversation

cyanglaz commented Mar 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Checklist

Breaking Change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyanglaz Mar 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mklim Mar 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mklim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyanglaz commented Mar 9, 2020

Uh oh!

cyanglaz commented Mar 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cyanglaz commented Mar 6, 2020 •

edited

Loading

cyanglaz Mar 6, 2020 •

edited

Loading

mklim Mar 7, 2020 •

edited

Loading