Flutterfire: [firebase_admob] FLTMobileAd.m should be member variables

[ouxiaoyong]

NSNumber *_mobileAdId in FLTMobileAd.m should be member variables

[ThrowJojo]

You're talking about this right?

https://github.com/FirebaseExtended/flutterfire/blob/master/packages/firebase_admob/ios/Classes/FLTMobileAd.m#L13

I use Swift a bit but I haven't used Objective C in years but it looks like you're onto something, a quick search comes up with this(looks like the above FLTMobileAd class is missing the curly brackets):

https://stackoverflow.com/questions/13263229/objective-c-instance-variables

I wonder if this is related to https://github.com/FirebaseExtended/flutterfire/issues/135

[ThrowJojo]

I had some time to fork the repo and poke around a bit with my limited Objective C knowledge. I added an NSLog that prints the allAds dictionary every time a new instance of FLTMobileAd is created. I added the log above: https://github.com/FirebaseExtended/flutterfire/blob/master/packages/firebase_admob/ios/Classes/FLTMobileAd.m#L62. When I run the example app this is what happens:

1. The example app starts up, the first banner loads without issue:

2019-11-03 14:49:10.361566+1000 Runner[56293:1669599] Dictionary: {
    1059320913 = "<FLTBannerAd: 0x600002640820> CREATED mobileAdId:1059320913 for: (null)";
}

1. I hit the 'Load Interstitial button' and this is the output for the allAds dictionary

Dictionary: {
    1059320913 = "<FLTBannerAd: 0x600002640820> CREATED mobileAdId:65009951 for: <GADBannerView: 0x7f9ff7510f80; frame = (0 0; 320 50); clipsToBounds = YES; layer = <CALayer: 0x60000242ab20>>";
    65009951 = "<FLTInterstitialAd: 0x600002659520> CREATED mobileAdId:65009951 for: (null)";
}

It looks like the mobileAdId for the banner is overridden by the mobileAdId for the newly loaded interstitial. I think this may be due to the member variable issue explained above.

[ThrowJojo]

Adding member variables like this seems to fix the issue:

@interface FLTMobileAd : NSObject

@property (nonatomic, assign) FLTMobileAdStatus status;
@property (nonatomic, assign) NSNumber *mobileAdId;

+ (void)configureWithAppId:(NSString *)appId;
+ (FLTMobileAd *)getAdForId:(NSNumber *)mobileAdId;
- (FLTMobileAdStatus)status;
- (void)loadWithAdUnitId:(NSString *)adUnitId targetingInfo:(NSDictionary *)targetingInfo;
- (void)show;
- (void)showAtOffset:(double)anchorOffset
       hCenterOffset:(double)horizontalCenterOffset
          fromAnchor:(int)anchorType;
- (void)dispose;
@end

I just created two for a quick test, I'm pretty sure all of them would need to be changed (and for FLTBannerAd and FLTInterstitialAd as well). I'm not really sure what the wider implications would be for this change either, I have pretty limited Objective-C experience.

[Paul-Todd]

@ThrowJojo 's suggestion seems to fix it - at least for me.

I removed the _mobileAdId and _status from the FLTMobileAd.m file.
Then in the FLTMobileAd.h file I changed the interface to

@interface FLTMobileAd : NSObject {
    //NSNumber * _mobileAdId;
    @protected FLTMobileAdStatus _status;
}

    @property (readonly, assign) FLTMobileAdStatus status;
    @property (readonly, assign) NSNumber * mobileAdId;

+ (void)configureWithAppId:(NSString *)appId;
+ (FLTMobileAd *)getAdForId:(NSNumber *)mobileAdId;

- (void)loadWithAdUnitId:(NSString *)adUnitId targetingInfo:(NSDictionary *)targetingInfo;
- (void)show;
- (void)showAtOffset:(double)anchorOffset
       hCenterOffset:(double)horizontalCenterOffset
          fromAnchor:(int)anchorType;
- (void)dispose;
@end

This should be considered a bandaid however as we should be able to remove the allAds variable as it is not needed anymore.

[Paul-Todd]

Just as an explanation as to what is happening. There are two bugs both related to the fact that the implementation variables are static. If you have two concurrent ads then the status property gets changed by two different FLTMobileAd calls and the underlying state machine's state jumps around since it should be per FLTMobileAd not a single static instance.

This is also the same issue with the mobileAdId though this can be mitigated.

[iapicca]

NSNumber *_mobileAdId in FLTMobileAd.m should be member variables

Hi @ouxiaoyong
given that your message isn't describing any bug or error,
but proposing an improvement I changed the label of the issue.
Can you please describe in details your proposal?
Thank you

[Paul-Todd]

I will chime in here as the original poster has not.

The bug is specific to iOS. If you create a banner ad and load it then create an interstitial ad then load it, then two things go wrong:

1. the callbacks get confused and the banner sometimes get the interstitial callback on the flutter side.
2. The state machine for each ad type gets confused and out of order.

This is happening because the status and the admobid are effectively static variables and so the last ad object to be created is the one that is used.

To fix this we need to have the admobid and the status variables as part of the FLTMobileAd object. This will ensure the ids are correctly mapped and the state machines use their correct status variable.

[Kuoyhout]

@Paul-Todd Yes, I also have this issue.

[blasten]

This is already fixed in the latest version.