search

Adrotator / AdrotatorV2

A highly customizable ad rotator component for Windows Phone and Windows 8 platforms, supporting the XAML, WinJS, Silverlight, XNA and Monogame frameworks.
24 stars 22 forks source link

Memory Leak with AdMob #53

Open dnt294 opened 9 years ago

dnt294 commented 9 years ago

I found this issue with Google Admob Banner: I created an user control contain AdRotator , and enable only Admob and Pubcenter with Probability = 50/50. I added it to a single page and repeatly refresh that page . Each time the Admob loaded, memory rose for 10-15MB and app crashed after 15 + times refresh page. This seem like google's problem since I tackled this problem with my previous app using AdMob.

SimonDarksideJ commented 9 years ago

Hmm, Yeah, the Google control does cause issues with it's use. We'll see if there is anything further we can do to manage it . Granted this will be a lot simpler once we get the WebAPI interface up and running for V2, no pesky controls just use their API (sorry no timescales yet, we're all maxed out at the mo, feel free to dig into the XNA V1 code and see if you can help :D)

jamesfdickinson commented 9 years ago

I have dug into this memory leak and made some code changes on my fork. I could not solve the admob memory leak, but i did for 2 other control, plus the adrotator as a whole.

In short, some ad provider controls do not turn off their timer on the unload. This leave a reference to them and they are never garbage collected. They need to be stopped using their stopad method. I added support to auto trigger these when the add changes. This stop all the extra calls it keeps making and lets it be GCed. I solved this for Smaato and Vserv (a new ad provider i just added to my version). I could not find any method to tell the admob control to stop its timer.

In my case, i have the adrotaor on a few page, so the adrotaor as a whole keeps ticking away even when i have unloaded the page. It cant be GCed. I also added support for that memory leak. I tend to call these timer leaks.

Only solution i can think of for the admob control is to create an instance of the control and then keep that one around in memory. Sounds messy, Id like to see the WebApi solution for admob.

I'm very new to Git, so when i figure out how to request a merge when i done testing ill try to add it to the project.

Changes

• Add provider Vserv for wp8.0/wp8.1 • Bug fix: Memory leak (timer leak) when using multiple pages • Update adrotator's timer to stop on unload (or manually with the dispose method) • Added support to stop the providers ads when switching ads (timer/memory/cpu leak) • Vserv never garbage collects and continues loading ads off screen because its internal timer stays on • Smaato never garbage collects continues loading ads off screen because its internal timer stays on • Admob never garbage collects continues loading ads off screen because its internal timer stays on (and there is no way to turn it off) • Updated Smaato AdProvider code to support stopAds • Updated Vserv AdProvider code to support stopAds • Added logging to debug output

SimonDarksideJ commented 9 years ago

That is fantastic and awesome news @jamesfdickinson

You can submit a PR in one of two ways, either:

Any issues, please let me know.

SimonDarksideJ commented 9 years ago

RIght @jamesfdickinson , Managed to find some spare time and look through your changes this weekend. TO save you time, I'll pull them and incorporate them from your repo this time. I posted on your PR that you pulled with instructions for how to better comtribute in the future :dancer: (it's just learning the Dance with GIT) Will comment again once it's up

SimonDarksideJ commented 9 years ago

OK, pulled out the updates and moved them to a new fork on my repo https://github.com/DDReaper/adrotatorv2/tree/Vserv_jamesfdickinson

Still a bit of work to do as my main machine only has 2015 now, need to use another PC for 2012/2013 verification.

jamesfdickinson commented 9 years ago

Not sure you got the last 2 i did last night. That should be all the changes. I don't plan on updating anything else soon. I was developing in VS 2012, so the 2013/2015 project file may not have a reference to the new code file for Vserv.

As for testing, I have been doing my own dev testing by displaying the logs in my app in debug mode. I'm also pushing my build to one of my lesser used apps for production verification(1000 daily active users). ill watch the error logs in my analytic, then if no issues, use the new build in my high traffic app (30,000 daily uses).

Smaato(wp8.0) seems to not load ads sometime without error so it does not move to the next. From my testing, it seem as if it is getting ads, but sometimes it is getting it in rich media format not image and does not display. Seems to be an issue with Smaato's control. I get the same behavior with their wp8 demo code.

AdRetrievalMode in the config is a new feature, so not sure that is out side of the scope without discussing the change first. BTW, it looked like AdMode.Stepped does nothing, it may but i didn't see any code next to it in the switch case. I didn't test or look further.

• Bug fix: Event Handler leak adRotatorControl.AdAvailable onload. If the page the control is on is navigated back to, the on load is called again and it will create a 2nd delegate for the event handler for each page load. This caused the ad to be created multiple times. • Added AdRetrievalMode(Random/Ordered) to be configurable from the settings file.

https://github.com/jamesfdickinson/AdrotatorV2/commits/develop

SimonDarksideJ commented 9 years ago

Great work @jamesfdickinson I like the addition of the AdMode config from the settings file, must have forgotten to catch that.

As to AdMode.Stepped doing nothing. Basically Both of those cases use the same Switch block, if it's ordered, then it breaks out early with the current top most successful Ad. Ordered basically means my most preferred Ad will show until it fails, then fall back to second favourite. If the mode is stepped, then it selects the next ad based on the next order value (then loops). Stepping loops through the ad providers in order.

Right, I'll commit these changes in to the base, just won't make a new build yet as a few other things need to be added.

If you then want to a Git Pull from The source (upstream) repo, it should get you back in sync. Then when you start work on another feature / fix, just create a new branch, put your changes in that and submit a PR from GitHub (the article I mentioned should help).

P.S. If you want to drop me a mail (through the contact page on my blog http://darkgenesis.zenithmoon.com), I'd like to talk further :dancer:

P.S. Moved your Debug code to the Log function, bound by a #IF, so that it doesn't get used in a production/ release build

SimonDarksideJ commented 9 years ago

Done @jamesfdickinson https://github.com/Adrotator/AdrotatorV2/pull/61 Core default branch now contains your updates, refresh your default branch from the main repo to sync up now.

alexsorokoletov commented 8 years ago

@jamesfdickinson thank you for your great work! Maybe this version of GoogleAds.dll will help you get access to timers inside AdMob and fix it. https://dl.dropboxusercontent.com/u/7022824/GoogleAds.Updated.zip

There are only two of them: adView.CurrentAdViewState?.Timer and adView.ViewModel.OtherStateField?.Timer Looks like calling .Stop on second one stops subsequent ads loading.

Good luck!