Fix for 680 NullPointerException in Requester.sendAdException

[jsligh]

Response Callback could be null so surrounding with Null checks. Closes #680

[YuriyVelichkoPI]

@jsligh, please attach the PR to the issues using the following guide
https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

It will help to close the related issues automatically once the PR is merged.

[YuriyVelichkoPI]

Hi @jsligh, the changes will definitely prevent the crash, but I don't understand the reason why the adResponseCallBack can be null here. If you take a look at the flow, you will see that it is pretty straightforward.

The load() method invokes the sendBidRequest, which creates the BidRequester with the responseHandler as a listener, and, right after that, runs the startAdRequest. So the listener should be alive.

The other question is getAdId(), that asynchronously calls the makeAdRequest(). But still, the listener is just a local variable of the Requester, so it's extremely strange that it is null.

In this case, it's better to dig deeper and figure out the situation why the adResponseCallBack can be null and introduce respective changes.

It is important not to miss some silent error behavior that will lead to bidding performance issues.

[YuriyVelichkoPI]

@linean, please, share the code of Prebid's ad unit integration in your app. I would like to understand the lifetime of the objects due to their scope.

[linean]

I cannot really share exact code we use. It's pretty complex and involves different mediations. Simplified version of our Prebid logic would look like that:

class PrebidBannerDemo: ComponentActivity() {

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)

        val maxAdView = MaxAdView("adUnitId", MaxAdFormat.BANNER, this)
        val utils = MaxMediationBannerUtils(maxAdView)
        val adSize = AdSize(320, 50)
        val adUnit = MediationBannerAdUnit(maxAdView.context, "configId", adSize, utils)

        adUnit.fetchDemand {
            maxAdView.loadAd()
        }

        lifecycleScope.launch {
            suspendCancellableCoroutine<Unit> { cancellation ->
                cancellation.invokeOnCancellation {
                    adUnit.destroy()
                    maxAdView.destroy()
                }
            }
        }
    }
}

[linean]

I should probably mention that crash on our end is pretty rare. It affects less than 0,1% of the users.

[linean]

Looking at code I think it is possible that AdIdManager.initAdId returns after Requester got destroyed. There is no cancellation in this flow.

[YuriyVelichkoPI]

Hi @linean! Thanks for sharing!

Yes, this initAdId adds, I would say, unnecessary async behavior, and it would be great to change it.

From your code, I see that due to the lifecycleScope.launch the adUnit wouldn't be destroyed on leaving the scope of onCreate, and it looks good.

So yes, let's take a bit more time to think about improvements around getAdId to make initial loading steps synchronous or at least more reliable and straightforward. So, we will definitely know what is happening and why internal objects disappear.

One more question about the crash info - do you see any common properties among reports, like device model or OS versions?

[linean]

These are the results for the past 7 days:

As you have control over the AdIdManager class, I would suggest incorporating a cancellation feature into it. This would enable the Requester to call adIdManager.destroy()

[jsligh]

Hi @linean! Thanks for sharing!
Yes, this initAdId adds, I would say, unnecessary async behavior, and it would be great to change it.
So yes, let's take a bit more time to think about improvements around getAdId to make initial loading steps synchronous or at least more reliable and straightforward. So, we will definitely know what is happening and why internal objects disappear.

As getAdvertisingIdInfo has to be called from a background thread, we have to find a way to either pass the task back to the Requester or to make sure all objects exist once initAdId is finished.

So I have some proposed ways to tackle this:

1. Have initAdId() return the FetchAdIdInfoTask so it can be cancelled in the destroy() method in Requester. This does require exposing FetchAdIdInfoTask and making it a public class.

2. Surround the makeAdRequest() calls with try/catch since this is an edge case that shouldn't happen very often (and errors only affecting 0.1% of users).

3. Have the AdIdFetchListener check for the existence of objects (or include local objects) before calling makeAdRequest.

4. (most difficult/time consuming) Refactor AdIdManager.initAdId. I'm against this mostly because it is a very small issue affecting a very small number of users.

[linean]

Please keep in mind that even a seemingly small percentage, such as 0.1%, can have a considerable impact. In the case of an application with 10 million DAU, this would translate to 10,000 affected users, which is a significant number. 😢

If I were in charge of the codebase, I would lean towards option 4. Option 3 also looks promising: the Requester could maintain a local destroyed flag and make decisions based on that.

[YuriyVelichkoPI]

We shouldn't tolerate any crashes despite the affected users because they are not our users but users of the publishers who integrate the SDK. Any affected user can write negative reviews of the apps that will harm the publisher's business.

I always vote for stateless (as much as possible) and decoupled solutions. So, making the standalone task that performs a particular job that we can easily cancel looks like the most robust way. However, we need to make sure that it will work fast enough. Could you clarify why the task class should be public in this case?

One more way is to pass the weak reference of the Requester to AdIdFetchListener so that if the requester is destroyed before the system response, it won't be captured in the listener's methods. As a result - no memory leaks and no calls to expired, partially destroyed objects.

Also, I would like to ask @ValentinPostindustria to look at our discussion and initial issue because he has already made some fixes and improvements in this class recently, and he can help us not reinvent the old bugs with new fixes :).

[YuriyVelichkoPI]

As for me, this implementation is much more reliable and SOLID-like. However, I'm not a big Android expert, so @linean, @ValentinPostindustria, it would be great if you review the changes as well.

From my side - I want to commit a research and design a different approach for obtaining the advertisement identifier because the current one looks extremely time-consuming. According to the Android docs, retrieving the identifier has a 10-second timeout. Due to the comments in the code the SDK waits for 3 seconds. But it is still a significant number.

There are publishers whose app's average screen time is just several seconds. So, to spend so much time on each bid request, it looks like not the optimal approach.

It would be better to introduce some smart AdIdManager, which will take care of updating the value according to the Android policy but will not block the bid requests.

If you think it makes sense, I'll prepare the ticket for optimization. And, of course, it is out of the scope of the current ticket.

cc: @alexsavelyev @mmullin

[linean]

It would be better to introduce some smart AdIdManager, which will take care of updating the value according to the Android policy but will not block the bid requests.

This is a good idea 👍 It's pretty common among other ad providers to cache this identifier.

[YuriyVelichkoPI]

The follow-up task for prefetching the advertising info:
#696

[YuriyVelichkoPI]

LGTM