larsacus / LARSAdController

Lightweight ad mediation for iOS to properly manage multiple ad networks dynamically including iAd and Google ads.
http://theonlylars.com/blog/2013/01/10/stupid-easy-ads-with-larsadcontroller-3-dot-0/
MIT License
269 stars 60 forks source link

Frequent crash in -[LARSAdController cleanUpAdAdapter:] #106

Open chamberlain2007 opened 9 years ago

chamberlain2007 commented 9 years ago

I'm seeing a fairly frequent crash in production, caused by removing an observer from an ad adapter which it is not observing, along the lines of the following:

Cannot remove an observer <LARSAdController 0x155c0f40> for the key path "adVisible" from <TOLAdAdapterGoogleAds 0x1f1f7db0> because it is not registered as an observer.

Full stacktrace is as follows:

Thread : Fatal Exception: NSRangeException
0  CoreFoundation                 0x30b75f0b __exceptionPreprocess + 130
1  libobjc.A.dylib                0x3b30cce7 objc_exception_throw + 38
2  CoreFoundation                 0x30b75e4d -[NSException initWithCoder:]
3  Foundation                     0x314c2b0f -[NSObject(NSKeyValueObserverRegistration) _removeObserver:forProperty:] + 578
4  Foundation                     0x314c2537 -[NSObject(NSKeyValueObserverRegistration) removeObserver:forKeyPath:] + 166
5  Transit360                     0x001fade3 -[LARSAdController cleanUpAdAdapter:] (LARSAdController.m:404)
6  UIKit                          0x333f8b3b -[UIViewAnimationBlockDelegate _sendDeferredCompletion:] + 66
7  Foundation                     0x31555117 __NSFireDelayedPerform + 414
8  CoreFoundation                 0x30b40f4f __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 14
9  CoreFoundation                 0x30b40b6b __CFRunLoopDoTimer + 794
10 CoreFoundation                 0x30b3eef3 __CFRunLoopRun + 1218
11 CoreFoundation                 0x30aa9769 CFRunLoopRunSpecific + 524
12 CoreFoundation                 0x30aa954b CFRunLoopRunInMode + 106
13 GraphicsServices               0x35a166d3 GSEventRunModal + 138
14 UIKit                          0x33408891 UIApplicationMain + 1136
15 Transit360                     0x00021d93 main (main.m:23)
16 libdyld.dylib                  0x3b80aab7 start + 2

I haven't been able to reproduce the crash with any consistency (I've seen it a few times), but have seen it a few times. However, it outweighs the other crashes in Crashlytics by a wide margin.

We are currently using version 3.0.2 installed via CocoaPods. I see there has been some work around the KVO with kLARSAdObserverKeyPathAdLoaded, but this seems to be the opposite issue with kLARSAdObserverKeyPathIsAdVisible: instead of registering too many times for notifications, it seems to happen to few.

Any thoughts on this?

mrcmd commented 9 years ago

I can confirm the issue. I have seen it regurarly in production but never able to figure out the root cause.

Workaround to fix it: https://github.com/mrcmd/LARSAdController/commit/dcd39374b5f055d8e07c23b787c9afd26b9faabe

larsacus commented 9 years ago

@chamberlain2007 The latest version is 3.0.7 -- please try that and see if this alleviates the issue. I know we've done some work specifically with the KVO stuff, as well as added a delegation pattern to use rather than relying on KVO -- which is much less error-prone and won't result in crashes when something goes wrong.

@mrcmd We should try and find the root cause of over-removing the observer rather than just catching it. Try using the latest version and see if this helps.

chamberlain2007 commented 9 years ago

@larsacus This crash is around kLARSAdObserverKeyPathIsAdVisible, which has no change in the adding/removal of the KVO around it. That is to say, I agree with the delegation pattern, but it probably won't solve this crash. I haven't been able to get a proper test case, so trying the new version won't verify much. For what it's worth, it happens most frequently on iPhone 4S running 7.1.2 (according to Crashlytics).

@mrcmd I'm hesitant to do this. If there is an observer that is being removed to many times, it's probably also being removed too early. I worry that there is a period in which something thinks that it should be getting notified of visibility changes and is not. The crash itself is worrisome, sure, but I wouldn't be surprised if there were other side-effects that simply catching the error wouldn't solve.