WeakReference in Requester can lead to "withheld" status update

[avalanchas]

Describe the bug The prebid SDK currently allows for one situation, where a caller is not informed about the result of a .fetchDemand() request. This can lead to "infinite loading" and the UI breaking. As far as I can tell, this bug was introduced in 2.x

The class Requester houses the important method makeAdRequest. Before and after it, there are several well-handled callback updates that cascade through the layers to the caller, for example via sendAdException and the adResponseCallback. However (I presume since the UserConsentManager rewrite for 2.x) there is the case, where if userConsentManager.canAccessDeviceData(), the requester making the call is stored in a WeakReference inside the AdIdInitListener.

This reference can be cleared by the system if no reference is holding on to it, which will lead the call to adIdFetchCompletion or adIdFetchFailure (whether successful or not does not matter, thus we are actively losing cases in which ads could be shown) to reach line 199, log a warning and be done with it. I have not determined 100% yet what can lead to the reference being cleared, however, I suspect that the dreaded AsyncTask has something to do with it, since the callback is executed in the FetchAdIdInfoTask:onPostExecute

To Reproduce

1. Add weakRequester.clear(); before Requester:L197
2. Install any of the demo apps on any device that has play services (so that apiAvailability.isGooglePlayServicesAvailable(context) in AdIdManager:L52 succeeds)
3. Observe that there are no success or error callbacks, the UI remains clueless and thinks the request is still going on. This can best be seen in the PrebidInternalTestApp, where neither the "onAdLoaded" nor the "onAdFailed" counters are incremented

Expected behavior There are two levels to this bug. Level 1 is that the problem must be surfaced so that the client can react to the knowledge that no request will be made (for example in the radio.net app we show a loading shimmer during ad load, this shimmer can not be cleared until we know what happened to our fetchDemand). You now might argue that clients should implement a timer to tear down dead requests after some timeout. In that case however, the documentation should communicate this front and center. Also:

Level 2 to solve this problem is finish all work on the Requester instance itself, probably convert it to Kotlin, put the AdId request in a suspend function with an IO-dispatcher (and put it on a scope that is bound to the app, so it survives until the app is dead). The requester shouldn't die in the first place, I can demonstrate in my app log that this problem can occur when a request was successful and I could totally have called makeAdRequest and shown an Ad. I mean... the simplest solution would be for Requester to implement AdIdFetchListener and simply call AdIdManager.initAdId(context, this);? But I suppose this would open us up to memory leaks, which is probably why the code is there in the first case?

I don't know if we should invest more work into making this 100% reproducible (I see the error on average in about 1/7 devices on my desk, and when I had Prebid 2.x in production in the beginning of december last year, that's also roughly how much our ad revenue suddenly dipped) when the volatile state of the instance could be definitively fixed by a small rewrite, when AsyncTask is deprecated anyway?

Screenshots

Smartphone:

• Prebid SDK: 2.0.8
• com.google.android.gms:play-services-ads version: 21.5.0
• Device / OS: unclear, small RAM sizes might contribute to this? Previously randomly seen on
  • Galaxy S10 Android 12
  • Galaxy S20 Android 13
  • Pixel 1 Android 11

[YuriyVelichkoPI]

Hi @avalanchas ! Thanks for the catch and the detailed report. It looks like a serious implementation error, and we should fix it as soon as possible.

I noticed that you've just committed several fixes to the repo that, frankly speaking, doesn't correspond to the prebid development flow, but still, are you going to provide a fix, or would you like to delegate fixing to us? @ValentinPostindustria can start working on it.

[avalanchas]

Please do be frank! I might be stuck in a bad process for contributing there, from other repos, sorry. I only committed one fix for the demo app (to dip my toe into contributing here so to speak), which has nothing to do with this issue here, so we should probably move the conversation out of this thread right away :)

I am hesitant to work on this issue here, as the change will have quite an impact, so please, do pass it on to Valentin, thank you so much already!

[YuriyVelichkoPI]

No worries :)

Great! @ValentinPostindustria will start working on this tomorrow. It defiantly has the highest priority.

[YuriyVelichkoPI]

Hi @avalanchas ! I want to let you know that we are working on this issue.

So far, we can confirm that SDK can fall into the infinite fetechDemand method, but we think that the Requeser's lifetime is not a reason for this. We've double-checked and can say that the weak reference looks correct in the current object structure.

We see a potential problem in the dependencies between AdUnit->AdLoader->BidRequester and calling the fetch demand in manual and autorefresh modes. But it's pretty subtle and assumes a deeper understanding of the integration environment. Need a bit more time.

Could you provide us a code snippet of your production integration of the Prebid SDK? It can be just a file with only the entrance of Prebid and GAM classes. You can remove all other code. We would like to see the integration approach. You can send it to me via email.

I understand that even if you provide your code snippet, it will be just a single example, but still, there are some concerns that we could confirm or reject and interpolate takeaways conclusions to other possible integration variants.

Also, you mention a revenue loss after migration to 2.0. Could you share more details about the reasons:

• a drop in requests
• a drop in bids
• a drop in bid price
• other

It would be helpful to figure out the reason.

Thank you!

[avalanchas]

Hi Yuriy, all I can say is I see the "Requester is null" log so yeah, I thought its lifetime should probably be pretty central to this issue, but you're the expert, I am eagerly awaiting the fix, whatever it is :)

Here are the snippets you requested

DisplayAdController.java

``` public DisplayAdController(..., @NonNull PreferenceDomain preferences) { super(context, preferences); ... PrebidController.setupPrebid(mPreferences.isDebugMode()); } ```

PrebidController.kt

``` @Override public void loadAd(@NonNull AdType adType, @Nullable Bundle bundle) { ... GoogleAdUtils.AdInfo adInfo = GoogleAdUtils.getAdInfo(context, mAdType, mPreferences.isAdTesting()); prepareAdView(context, adInfo); Timber.tag(TAG).d("loading Ad with: adType = [%s], adInfo = [%s]", mAdType, adInfo); AdManagerAdRequest.Builder builder = makeConsentAwareBuilder(); addCustomTargeting(builder); AdManagerAdRequest request = builder.build(); if (adInfo.getPrebidConfigId() != null && mPreferences.isPrebidEnabled()) { mPrebid = true; // PreBid injects itself into the ad request to select a winner for this specific one PrebidController.loadPrebid(context, adInfo.getAdSize(), adInfo.getPrebidConfigId(), request, result -> { ... // Once PreBid is set up we get a callback. We can now show the selected ad if still alive loadAdIntoView(request); }); } else { mPrebid = false; loadAdIntoView(request); } } class PrebidController { companion object { @JvmStatic fun setupPrebid(isDebugMode: Boolean) { if (isDebugMode) { PrebidMobile.setLogLevel(PrebidMobile.LogLevel.DEBUG) } // Hardcoded, our company uses a custom server PrebidMobile.setPrebidServerHost(Host.createCustomHost(AdConstants.PREBID_SERVER_HOST_URL)) PrebidMobile.setPrebidServerAccountId(AdConstants.PREBID_SERVER_ACCOUNT_ID) } @JvmStatic fun loadPrebid( context: Context, adSize: AdSize, prebidConfigId: String, request: AdManagerAdRequest, onCompleteListener: OnCompleteListener, ) { // Application context is stored statically (as weak reference) inside the PrebidMobile if (!PrebidMobile.isSdkInitialized()) { PrebidMobile.initializeSdk(context.applicationContext) { Timber.tag(TAG).d("onInitializationComplete with status = {$it}") } } // Note that we do not have to set "TargetingParams.setSubjectToGdpr()" since Prebid can // read IAB keys from HDD // See: https://github.com/prebid/prebid-mobile-android/issues/485#issuecomment-1338030020 Timber.d("loadPrebidAd with prebidConfigId: [%s]", prebidConfigId) val bannerAdUnit = BannerAdUnit( prebidConfigId, // Careful, width and height must be set to a valid value that is configured in // the backend. Otherwise, the server will never return any bids on this unit adSize.width, adSize.height ) try { bannerAdUnit.fetchDemand(request, onCompleteListener) // Catch for Prebid crash: https://github.com/prebid/prebid-mobile-android/issues/485 } catch (ex: Exception) { onCompleteListener.onComplete(ResultCode.PREBID_SERVER_ERROR) Timber.e(ex, "loadPrebid fetchDemand crashed") recordException(ex) } } } } ```

I sent you the ad server numbers from our release in a mail. As we already discussed the inventory (request numbers) trended downwards - in parallel with the responses and impressions

Thanks again

[YuriyVelichkoPI]

Hi @avalanchas! Thanks for the snippets and monetization figures! They gave us more understanding of publisher's integration approaches and details which will help to make the SDK more robust.

In this case, the possible reason for endless fetch demand is that BannerAdUnit is a local variable. So even if the bid response is received, it may happen that the chain of recipients may be already destroyed, so the app won't receive the callback.

So adding a strong reference to the requester or a forced callback for the timed-out request may not fix the issue. We should investigate the previous behavior and fix the regression. So 2.0 should support using of local AdUnit instances if 1.X supported them.

In any case, we will review the behavior of broken requests and will add a callback if the request is stuck for some reason so that publishers can handle this situation respectively.

We'll do our best to introduce the fix this week.

[avalanchas]

Interesting. I will add then, that this issue happened to me while the app was in the foreground and actively being used / touched. The screen on which the display ad is shown stays alive, so does the fragment that contains it

[YuriyVelichkoPI]

Yep, it makes sense.

The scope of the val bannerAdUnit is the loadPrebid function. In the end of execution no one else references the bannerAdUnit, so it can be destroyed anytime, regardless of foreground or background state of the app/activity/fragment.

[yangling1985]

Hi @avalanchas! Thanks for the snippets and monetization figures! They gave us more understanding of publisher's integration approaches and details which will help to make the SDK more robust.

In this case, the possible reason for endless fetch demand is that BannerAdUnit is a local variable. So even if the bid response is received, it may happen that the chain of recipients may be already destroyed, so the app won't receive the callback.

So adding a strong reference to the requester or a forced callback for the timed-out request may not fix the issue. We should investigate the previous behavior and fix the regression. So 2.0 should support using of local AdUnit instances if 1.X supported them.

In any case, we will review the behavior of broken requests and will add a callback if the request is stuck for some reason so that publishers can handle this situation respectively.

We'll do our best to introduce the fix this week.

Hi @YuriyVelichkoPI, does prebid mobile Android SDK support bannerAdUnit or BannerView to be local variable, without the callback missing issue now?

[YuriyVelichkoPI]

Hi @yangling1985! Sorry for the delay.

Using local variables for ad units is not a preferable implementation because they are not stateless objects. It's better to use the class-level property. And manage its lifecycle respectively.

[yangling1985]

Hi @yangling1985! Sorry for the delay.

Using local variables for ad units is not a preferable implementation because they are not stateless objects. It's better to use the class-level property. And manage its lifecycle respectively.

No worries, @YuriyVelichkoPI. thanks for the reply. How about using local variables of BannerView for the rendering API case. Below is the sample code:

private static void loadBanner() {
       BannerView bannerView = new BannerView(context, card.configId, adSize); 
       bannerView.setBannerListener(prebidBannerListener);
       bannerView.loadAd();
}

Do you think the above sample code could have problems?

Further more, we notice many cases that None of any callbacks of interface BannerViewListener is called after method bannerView.loadAd() is called. Could they be related to the local BannerView reference?

Thanks!

[YuriyVelichkoPI]

Hi @yangling1985! I wouldn't recommend using local variables for ad unit classes from Rendering API.

The ad units in Original API play mostly the transport role. They request the bid and provide the bid info to the app. No other responsibilities.

But ad units of Rendering API have many more responsibilities and a much longer life cycle, so they should stay alive and should be managed by the app code to display the ad correctly respectively to the app's flow.

Suppose the app destroys the ad unit, including the case of leaving the scope for local variables. In that case, all ad unit content will be destroyed as well - WebView, listeners, analytic trackers, etc. SDK doesn't know the app's flow it only knows the "usage" of the ad unit - reference count. If no objects in the app use the ad unit (no strong references), it will be destroyed.

So, it's better to keep the ad unit alive (hold the class-level reference) while it displays the ads according to the app flow. Also, the app developer is responsible for destroying the ad unit when it is not needed anymore to avoid memory leaks.

[yangling1985]

Thanks for your reply, @YuriyVelichkoPI! I agree with you that the app is responsible to hold and maintain the BannerView instance's lifecycle. I have a question here for a more specific situation: is it OK to use a local variable for the BannerView instance to load but persist the BannerView instance(I assume it is the same as before) returned from the callback, as demonstrated as below code clip?

private static void loadBanner() {
       BannerView bannerView = new BannerView(context, card.configId, adSize); 
       bannerView.setBannerListener(new PrebidBannerListener());
       bannerView.loadAd();
}

static class PrebidBannerListener implements BannerViewListener {
        @Override
        public void onAdLoaded(BannerView bannerView) {
                 // hold and keep bannerView alive during the displaying until it is destroyed.  
        }
}

Further more, I read the source code and found the below reference chain that will hold the local bannerView instance before the callback method onAdLoaded(BannerView bannerView) is called, so that I think for the above case we do not have to worry the bannerView will be destroyed because of out of scope?

BaseNetworkTask ---> ResponseHandler ---> BidRequesterListener ---> BannerView

[YuriyVelichkoPI]

I wouldn't recommend relying on this behavior because there is no guarantee that it won't change in the future intentionally or even accidentally.

As for me, it looks like a potential memory leak or an imperfect memory management right now. Because SDK keeps the listener alive until the response is processed, and even if the app flow drops out the activity or fragment with an ad view, the SDK still keeps the listener alive.

So I see here the lack of solid spec for Prebid SDK, both iOS and Android, about how it manages the bid sessions, objects, and callbacks for both Original and Rendering API. Which publishers can rely on and devs should follow. I've created a ticket to address it: https://github.com/prebid/prebid-mobile-ios/issues/910. Once the community prioritizes this work - I'll let you know so we can develop the best policy for the bid sessions and memory management from both publishers' and SDK developers' perspectives.

[yangling1985]

Got it! Please let us know when there is some update on this memory management initiative. Thanks! 🔥