[firebase_admob] Fix firebase_admob issues caused by un-handled exception situations #1807

archanpaul · 2019-07-06T13:29:02Z

Description

While using BannerAd from fairly complex application which expects BannerAd to be disposed gracefully there are multiple un-handled exception situations in the plugin which are addressed in following commits in https://github.com/archanpaul/flutter-plugins/commits/fix-firebase_admob

commit 7dfeeac (HEAD -> fix-firebase_admob, origin/fix-firebase_admob)
Author: Archan Paul [email protected]
Date: Sat Jul 6 18:36:29 2019 +0530

Handle exception in _invokeBooleanMethod(...) and return false in case of exception.

commit a2b1eac
Author: Archan Paul [email protected]
Date: Sat Jul 6 17:51:34 2019 +0530

Interim fix for dangling BannerAd during dispose call.

Related code and log references

  BannerAd _adbmobBanner = BannerAd(
      adUnitId: BannerAd.testAdUnitId,
      size: AdSize.smartBanner,
      targetingInfo: _targetingInfo,
      listener: (MobileAdEvent event) {
        _subjectAdEvent.sink.add(event);
        switch (event) {
          case MobileAdEvent.clicked:
            break;
          case MobileAdEvent.closed:
            break;
          case MobileAdEvent.failedToLoad:
            break;
          case MobileAdEvent.impression:
            break;
          case MobileAdEvent.leftApplication:
            break;
          case MobileAdEvent.loaded:
            break;
          case MobileAdEvent.opened:
            break;
          default:
            break;
        }
      },
    );
  ...
  Future dispose() async {
    await _adbmobBanner?.dispose();
  }

E/flutter (29705): [ERROR:flutter/lib/ui/ui_dart_state.cc(148)] Unhandled Exception: PlatformException(no_ad_for_id, dispose failed, no add exists for id=663181399, null)
E/flutter (29705): #0      StandardMethodCodec.decodeEnvelope 
package:flutter/…/services/message_codecs.dart:564
E/flutter (29705): #1      MethodChannel.invokeMethod 
package:flutter/…/services/platform_channel.dart:316
E/flutter (29705): <asynchronous suspension>
E/flutter (29705): #2      _invokeBooleanMethod 
package:firebase_admob/firebase_admob.dart:524
E/flutter (29705): <asynchronous suspension>
E/flutter (29705): #3      MobileAd.dispose 
package:firebase_admob/firebase_admob.dart:242
E/flutter (29705): #4      AdmobProvider.dispose 
package:fitzy_20190415/…/controllers/AdmobProvider.dart:39
E/flutter (29705): <asynchronous suspension>
E/flutter (29705): #5      _WidgetPromoState.dispose 
package:fitzy_20190415/…/widgets/widgetPromo.dart:36
E/flutter (29705): <asynchronous suspension>
E/flutter (29705): #6      StatefulElement.unmount 
package:flutter/…/widgets/framework.dart:4107
E/flutter (29705): #7      _InactiveElements._unmount 
package:flutter/…/widgets/framework.dart:1737
E/flutter (29705): #8      _InactiveElements._unmount.<anonymous closure> 
package:flutter/…/widgets/framework.dart:1735
E/flutter (29705): #9      MultiChildRenderObjectElement.visitChildren 
package:flutter/…/widgets/framework.dart:5181
E/flutter (29705): #10     _InactiveElements._unmount 
package:flutter/…/widgets/framework.dart:1733
E/flutter (29705): #11     ListIterable.forEach  (dart:_internal/iterable.dart:39:13)
E/flutter (29705): #12     _InactiveElements._unmountAll 
package:flutter/…/widgets/framework.dart:1746
E/flutter (29705): #13     BuildOwner.finalizeTree.<anonymous closure> 
package:flutter/…/widgets/framework.dart:2426
E/flutter (29705): #14     BuildOwner.lockState 
package:flutter/…/widgets/framework.dart:2258
E/flutter (29705): #15     BuildOwner.finalizeTree 
package:flutter/…/widgets/framework.dart:2425
E/flutter (29705): #16     _WidgetsFlutterBinding&BindingBase&GestureBinding&ServicesBinding&SchedulerBinding&PaintingBinding&SemanticsBinding&RendererBinding&WidgetsBinding.drawFrame 
package:flutter/…/widgets/binding.dart:702
E/flutter (29705): #17     _WidgetsFlutterBinding&BindingBase&GestureBinding&ServicesBinding&SchedulerBinding&PaintingBinding&SemanticsBinding&RendererBinding._handlePersistentFrameCallback 
package:flutter/…/rendering/binding.dart:285
E/flutter (29705): #18     _WidgetsFlutterBinding&BindingBase&GestureBinding&ServicesBinding&SchedulerBinding._invokeFrameCallback 
package:flutter/…/scheduler/binding.dart:1016
E/flutter (29705): #19     _WidgetsFlutterBinding&BindingBase&GestureBinding&ServicesBinding&SchedulerBinding.handleDrawFrame 
package:flutter/…/scheduler/binding.dart:958
E/flutter (29705): #20     _WidgetsFlutterBinding&BindingBase&GestureBinding&ServicesBinding&SchedulerBinding._handleDrawFrame 
package:flutter/…/scheduler/binding.dart:874
E/flutter (29705): #21     _rootRun  (dart:async/zone.dart:1124:13)
E/flutter (29705): #22     _CustomZone.run  (dart:async/zone.dart:1021:19)
E/flutter (29705): #23     _CustomZone.runGuarded  (dart:async/zone.dart:923:7)
E/flutter (29705): #24     _invoke  (dart:ui/hooks.dart:236:10)
E/flutter (29705): #25     _drawFrame  (dart:ui/hooks.dart:194:3)
E/flutter (29705):

…e of exception.

googlebot · 2019-07-06T13:29:05Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

archanpaul · 2019-07-08T10:48:49Z

I signed CLA.

googlebot · 2019-07-10T05:36:38Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

bparrishMines

Hi @archanpaul

Thanks for the contribution! I left you a comment.

packages/firebase_admob/lib/firebase_admob.dart

…ide.

collinjackson

Thanks for the contribution!

Can you please update pubspec.yaml and CHANGELOG for release?

It looks like it should be possible to write a unit test for the added behavior (right now all the unit tests assert that dispose() returns true). Could you please add that test here: https://github.com/flutter/plugins/blob/master/packages/firebase_admob/test/firebase_admob_test.dart

archanpaul · 2019-07-16T11:47:56Z

Thanks @collinjackson for your feedback.

I think it is not easy to add a test case to simulate the exact scenario, which prompted me to added the patch. In the unit test we need to create a scenario in which, while Platform is working on loadBannerAd/showAd ( _bannerAd..load()..show() .... not listening and acting based on event callbacks) real Ad (over a non LTE network), the banner object is released without waiting (eg: timeout scenario in real app). This might result into a scenario of no_ad_for_id error, which should return false. Do you have better idea?

For the benefit of plugin users, you may consider merging the current patch now and later add a comprehensive unit-test related to "dispose() returns false".

Can you please update pubspec.yaml and CHANGELOG for release?

Do you want me to create 0.9.0+2 ?

collinjackson · 2019-07-16T12:24:19Z

Thanks @collinjackson for your feedback.

I think it is not easy to add a test case to simulate the exact scenario, which prompted me to added the patch. In the unit test we need to create a scenario in which, while Platform is working on loadBannerAd/showAd ( _bannerAd..load()..show() .... not listening and acting based on event callbacks) real Ad (over a non LTE network), the banner object is released without waiting (eg: timeout scenario in real app). This might result into a scenario of no_ad_for_id error, which should return false. Do you have better idea?

This is a unit test, not an integration test; you should be able to simulate the no_ad_for_id error in the setMockMethodCallHandler.

You could either throw a PlatformException in the unit test or (my preference) update the native side so that disposeAd returns false instead of a "no_ad_for_id" PlatformException. Then you won't need to worry about catching and handling an exception on the Dart side, and there's no need to test the special handing of the "no_ad_for_id" on the Dart side.

Can you please update pubspec.yaml and CHANGELOG for release?

Do you want me to create 0.9.0+2 ?

Yes.

archanpaul · 2019-07-16T15:27:56Z

@collinjackson

You could either throw a PlatformException in the unit test or (my preference) update the native side so that disposeAd returns false instead of a "no_ad_for_id" PlatformException. Then you won't need to worry about catching and handling an exception on the Dart side, and there's no need to test the special handing of the "no_ad_for_id" on the Dart side.

I have added unit test case in 24cdbe6 where I throw PlatformException with corresponding error code.

I prefer Platform to make Dart side of plugin aware that no_ad_for_id happened and let Dart plugin make a note and take care of it (return false to upper layer).

Can you please update pubspec.yaml and CHANGELOG for release?

Done.

collinjackson

Thanks. This is looking good, I edited your changelog and I have one other nit

collinjackson · 2019-07-16T17:49:50Z

packages/firebase_admob/test/firebase_admob_test.dart

+        size: AdSize.banner,
+      );
+      final int id = banner.id;
+      invalidAdId = banner.id;


I think what you're doing here is fragile and might cause the other disposeAd tests to fail depending on the order that the tests run, because they're re-using the same id testAdUnitId.

You might want to reset invalidAdId to null in setUp()

@collinjackson thanks for pointing out. I have updated in f409fc4.

collinjackson

I'm looking at this PR more carefully and I'm wondering what is the situation is causing dispose() to be called on a banner ad more than once. It seems to me that dispose() should be a Future<void> that can only be called once per ad, that calling dispose on an ad more than once should be prevented with a Dart assertion; in release builds where the assertion is disabled we could return early out of dispose() rather than sending the request over the bridge where we know a platform exception will be generated. This would help developers catch bugs in their code where they are keeping around references to banner ads after disposal.

If you feel that being able to call dispose() on ads multiple times is an important use case I'd love to see an example of usage.

Thanks.

archanpaul · 2019-07-18T06:39:31Z

@collinjackson

I do not have any use-case where BannerAd.dispose() is required to be called multiple times. The dispose() should return immediately in case of non-existent adID .. there is no need to wait for PlatformException to happen which is anyway inevitable in this case.

Generally speaking, most of the app developers only care to know the bool return of dispose() and ensure that bannerAd = null after successful dispose.

if you want to enforce the app developer to know that dispose is called on a non-existent adID, instead of assert (which actually does not give any meaningful reason in log), you can either throw a Dart exception informing the developer the reason explicitly or throw the PlatformException as it is (though not intuitive).

I personally like to have hide() api call which should immediately hide the Ad instead of dispose(). As a Flutter app developer I will not like to await on dispose() to hide the Ad from UI. It is always preferable to have features like load() once (and sometimes await refresh() with corresponding event listner) the Ad and do show() & hide() as an when required to make Admob and this plugin intuitive/useful for the developer. This is my wish, not related to this PR.

collinjackson · 2019-08-26T16:09:54Z

Migrated to firebase/flutterfire#37

archanpaul added 2 commits July 6, 2019 17:51

Interim fix for dangling BannerAd during dispose call.

a2b1eac

Handle exception in _invokeBooleanMethod(...) and return false in cas…

7dfeeac

…e of exception.

googlebot added the cla: no label Jul 6, 2019

cyanglaz changed the title ~~Fix firebase_admob issues caused by un-handled exception situations~~ [firebase_admob] Fix firebase_admob issues caused by un-handled exception situations Jul 9, 2019

Handle only no_ad_for_id.

a5272ea

googlebot added cla: yes and removed cla: no labels Jul 10, 2019

bparrishMines suggested changes Jul 11, 2019

View reviewed changes

packages/firebase_admob/lib/firebase_admob.dart Outdated Show resolved Hide resolved

bparrishMines added the in review label Jul 11, 2019

Return immediately when _allAds[id] == null otherwise call platform s…

e0a82e1

…ide.

collinjackson added the submit queue The Flutter team is in the process of landing this PR. label Jul 15, 2019

collinjackson self-requested a review July 15, 2019 17:05

collinjackson suggested changes Jul 15, 2019

View reviewed changes

collinjackson added the flutterfire label Jul 16, 2019

Added Dart side unit test for no_ad_for_id scenario.

24cdbe6

archanpaul requested a review from amirh as a code owner July 16, 2019 15:13

archanpaul and others added 3 commits July 16, 2019 22:59

Merge branch 'master' into fix-firebase_admob

18d194f

Updated version to 0.9.0+3

891494e

Update CHANGELOG.md

3acf43b

collinjackson reviewed Jul 16, 2019

View reviewed changes

archanpaul added 2 commits July 17, 2019 09:19

Merge branch 'master' into fix-firebase_admob

310639d

Reset invalidAdId to null in setUp().

f409fc4

collinjackson self-requested a review July 17, 2019 15:13

collinjackson reviewed Jul 17, 2019

View reviewed changes

archanpaul added 2 commits August 19, 2019 17:57

Merge branch 'master' into fix-firebase_admob

377b8e8

Updated version to 0.9.0+5

0bff10c

collinjackson mentioned this pull request Aug 26, 2019

[firebase_admob] Fix firebase_admob issues caused by un-handled exception situations firebase/flutterfire#37

Closed

collinjackson closed this Aug 26, 2019

collinjackson added a commit to collinjackson/flutterfire that referenced this pull request Aug 30, 2019

Merge remote-tracking branch 'origin/master' into flutter/plugins#1807

38c8e61

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[firebase_admob] Fix firebase_admob issues caused by un-handled exception situations #1807

[firebase_admob] Fix firebase_admob issues caused by un-handled exception situations #1807

archanpaul commented Jul 6, 2019

googlebot commented Jul 6, 2019

archanpaul commented Jul 8, 2019

googlebot commented Jul 10, 2019

bparrishMines left a comment

collinjackson left a comment

archanpaul commented Jul 16, 2019

collinjackson commented Jul 16, 2019 •

edited

Loading

archanpaul commented Jul 16, 2019

collinjackson left a comment

collinjackson Jul 16, 2019

archanpaul Jul 17, 2019

collinjackson left a comment

archanpaul commented Jul 18, 2019

collinjackson commented Aug 26, 2019

[firebase_admob] Fix firebase_admob issues caused by un-handled exception situations #1807

[firebase_admob] Fix firebase_admob issues caused by un-handled exception situations #1807

Conversation

archanpaul commented Jul 6, 2019

Description

Related code and log references

googlebot commented Jul 6, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

archanpaul commented Jul 8, 2019

googlebot commented Jul 10, 2019

bparrishMines left a comment

Choose a reason for hiding this comment

collinjackson left a comment

Choose a reason for hiding this comment

archanpaul commented Jul 16, 2019

collinjackson commented Jul 16, 2019 • edited Loading

archanpaul commented Jul 16, 2019

collinjackson left a comment

Choose a reason for hiding this comment

collinjackson Jul 16, 2019

Choose a reason for hiding this comment

archanpaul Jul 17, 2019

Choose a reason for hiding this comment

collinjackson left a comment

Choose a reason for hiding this comment

archanpaul commented Jul 18, 2019

collinjackson commented Aug 26, 2019

collinjackson commented Jul 16, 2019 •

edited

Loading