googleads-mobile-unity icon indicating copy to clipboard operation
googleads-mobile-unity copied to clipboard

[UMP + MobileAds] MobileAds.RaiseAdEventsOnUnityMainThread = true does not guarantee that the callbacks will be on the Unity main thread, leading to unknown crashes.

Open shobhit-samaria opened this issue 1 year ago • 19 comments

[REQUIRED] Step 1: Describe your environment

  • Unity version: 2020.3.45f1
  • Google Mobile Ads Unity plugin version: 8.6.0
  • Platform: Android
  • Platform OS version: Android 13
  • Any specific devices issue occurs on:
  • Mediation ad networks used, and their versions:

[REQUIRED] Step 2: Describe the problem

I have upgraded from 8.4.1 version to 8.6.0 today and implemented the UMP workflow. However I am getting crashes in my app which seems to be due to order of execution of MobileAds.RaiseAdEventsOnUnityMainThread and ConsentInformation.Update() method.

If the order in the code is as follows, everything works correctly:

MobileAds.RaiseAdEventsOnUnityMainThread = true;
ConsentInformation.Update(request, OnConsentInfoUpdated);

But if the order is as follows:

ConsentInformation.Update(request, OnConsentInfoUpdated);

void OnConsentInfoUpdated(FormError consentError)
{
        ConsentForm.LoadAndShowConsentFormIfRequired((FormError formError) =>
        {
            // Consent has been gathered.
            MobileAds.RaiseAdEventsOnUnityMainThread = true;
            MobileAds.Initialize(initStatus =>
            {
               ...............................
            });
        });
}

When above code is used, it seems it does not capture the Unity main thread correctly and the ad callbacks are not called on the main thread. Therefore, if there is some code in the callback which needs to be on the main thread (like a TextMeshProUGUI etc.) it causes a fatal crash.

I think the race condition of #2841 is not fixed completely. It seems the Unity main thread is set in a static constructor of MobileAds class. From the docs (https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/static-constructors) -- The user has no control on when the static constructor is executed in the program. If the static constructor gets called at the wrong time, then it may be possible that wrong values are captured by it. This can cause unexpected crashes which can be very hard to trace. Either the constructor needs to be forced to be executed at the correct time (by accessing a static property or method) or a different solution needs to be found. If this is working as expected, then perhaps the docs should be updated to reflect that the RaiseAdEventsOnUnityMainThread flag (or any other static property/method) should be set before any calls to UMP api.

This is just my basic analysis based on the issues that I faced. Happy to be corrected if I went wrong somewhere.

shobhit-samaria avatar Dec 15 '23 19:12 shobhit-samaria

Hey. Recently I integrated UMP Consent form with the latest version 8.6.0. Right away after the rollout I started seeing higher crash rate in Crashlytics and Google dashboard. Currently I'm not using RaiseAdEventsOnUnityMainThread at all and I'm not experiencing any crashes on my android devices. But all of the devices in the crash reports are on Android 13. Can I ask what type of error you getting when crashing? In Firebase I see many crashes like SIGSEGV and SIGABRT. Do you have something similar?

EAdemov avatar Dec 16 '23 22:12 EAdemov

Hey. Recently I integrated UMP Consent form with the latest version 8.6.0. Right away after the rollout I started seeing higher crash rate in Crashlytics and Google dashboard. Currently I'm not using RaiseAdEventsOnUnityMainThread at all and I'm not experiencing any crashes on my android devices. But all of the devices in the crash reports are on Android 13. Can I ask what type of error you getting when crashing? In Firebase I see many crashes like SIGSEGV and SIGABRT. Do you have something similar?

Yes, I was getting the same kind of errors with a bunch of memory addresses which is nearly impossible to trace. I was also seeing the crash on lower android versions as well. You can try and use RaiseAdEventsOnUnityMainThread = true. A lot of UnityEngine code requires to be run on the main thread and crashes the app if you run it on a wrong thread. If the Ad callbacks are triggered on a wrong thread, the whole sequence of events started from the callback is on a wrong thread, which can potentially cause a crash a long way down the line and impossible to trace to the original source. Maybe that is what is happening in your app.

Call RaiseAdEventsOnUnityMainThread = true as the first thing in your code before any UMP/MobileAds code and make sure to call it from a Unity method (Start/Awake etc) directly and not in any separate thread. This gives you a better chance of avoiding such crashes but like I have mentioned in the bug, as the code stands, there is no guarantee that the code will pickup the correct main unity thread and could still cause crashes. Try it out and see if it helps you to reduce crashes (assuming this is the issue that is causing crashes in your app).

shobhit-samaria avatar Dec 17 '23 07:12 shobhit-samaria

The more I think about it, I believe that I labeled the issue incorrectly. The issues is not as much as a race condition but that MobileAds can NOT GUARANTEE that it picks up the main unity thread correctly. If the MobileAds.SetUnityMainThreadSynchronizationContext is made public and developers are forced to call it as the first thing before any other code execution from any of the Unity lifecycle methods (or any such enforcement from the plugin itself), that will probably ensure that the thread and context being picked up are correct. Otherwise there is no way to be certain that the main thread in MobileAds is the correct one. While this may work correctly 99% of the time, but it will be more by luck than by design. But the times it fails, it will be hard for developers to figure out what is causing the issue in their apps.

And this is probably not an UMP workflow issue. The issue probably exists in earlier versions as well. In case the MobileAds.Initialize method is called from a new Thread, then maybe the same issue would occur there as well (as the context and thread id were set inside the initialize method earlier).

Again I am just noting down my thoughts. Happy to be corrected.

shobhit-samaria avatar Dec 17 '23 08:12 shobhit-samaria

Thank you for the detailed reply. I will try the solution will MobileAds.RaiseAdEventsOnUnityMainThread = true; and gonna look the crash rates.

EAdemov avatar Dec 17 '23 08:12 EAdemov

Sadly didn't help. I call MobileAds.RaiseAdEventsOnUnityMainThread = true; before MobileAds initialization or any Ads related SDK. I call it in Start() which is executed on MainThread.

EAdemov avatar Dec 18 '23 23:12 EAdemov

Sorry to hear that it didn't help. It could be likely that the cause of your error is not related to this issue. Or like I have mentioned above that even by using MobileAds.RaiseAdEventsOnUnityMainThread = true; does not guarantee execution of code on main thread. I am still awaiting any kind of response from the plugin team.

If you still believe that this is a likely cause of your crashes, then you can try one more thing. You can use UnityMainThreadDispatcher (https://github.com/PimDeWitte/UnityMainThreadDispatcher) to force execution of your code on the main thread.

So instead of doing something like this -

rewardedAd.Show((Reward Reward) => {
       //your code goes here                
}); 

Do something like this -

rewardedAd.Show((Reward Reward) => {
            UnityMainThreadDispatcher.Instance().Enqueue(() => {
                    //your code goes here       
            }
}); 

Doing something like this for all the admob callbacks (including UMP workflow callbacks) should ensure that your code is executed on the main unity thread irrespective of what the MobileAds sdk picks up. I hope the plugin team can use something like UnityMainThreadDispatcher instead of relying on a static constructor.

That is all the help that I can provide. Good luck.

shobhit-samaria avatar Dec 20 '23 14:12 shobhit-samaria

Thank you. Worth trying it. Based on logs right before the crash, it seems like it happens before initialization MobileAds. But I think since the ConsentForm.LoadAndShowConsentFormIfRequired(...) is UI related method and I should call wrap it inside:

UnityMainThreadDispatcher.Instance().Enqueue(() => {
                    //your code goes here       
            }

Thanks for the heads up again.

EAdemov avatar Dec 20 '23 16:12 EAdemov

@shobhit-samaria

I will look into a fix for this. For the time being please use the suggested work around:

MobileAds.RaiseAdEventsOnUnityMainThread = true;
ConsentInformation.Update(request, OnConsentInfoUpdated);

NVentimiglia avatar Dec 20 '23 22:12 NVentimiglia

Thank you. Worth trying it. Based on logs right before the crash, it seems like it happens before initialization MobileAds. But I think since the ConsentForm.LoadAndShowConsentFormIfRequired(...) is UI related method and I should call wrap it inside:

UnityMainThreadDispatcher.Instance().Enqueue(() => {
                    //your code goes here       
            }

Thanks for the heads up again.

Again good luck. Also keep an open mind that your issue could be a completely different bug or problem, so keep looking in all directions. The consent code internally use MobileAds class itself so the consent callbacks suffer from the same issue. Using UnityMainThreadDispatcher can probably fix our code, but if there is some plugin code itself that needs the main thread, then it is something we won't be able to fix. I am currently just adding UnityMainThreadDispatcher calls throughout my code. :)

OnConsentInfoUpdated(FormError consentError)
{
        UnityMainThreadDispatcher.Instance().Enqueue(() =>
        {
            ConsentForm.LoadAndShowConsentFormIfRequired((FormError formError) =>
            {
                UnityMainThreadDispatcher.Instance().Enqueue(() =>
                {
                    // Consent has been gathered.                   
                });
            });
        });
} 

shobhit-samaria avatar Dec 21 '23 06:12 shobhit-samaria

@shobhit-samaria

I will look into a fix for this. For the time being please use the suggested work around:

MobileAds.RaiseAdEventsOnUnityMainThread = true;
ConsentInformation.Update(request, OnConsentInfoUpdated);

Thank you. Looking forward to a robust solution. I have updated the title to better reflect the issue.

shobhit-samaria avatar Dec 21 '23 06:12 shobhit-samaria

I confirm this issue is also exist on me the time I import UMP Sdk & trying to do ConsentForm

Google Ads: 8.52 Unity: 2021.3.21f1

StinkySteak avatar Dec 29 '23 14:12 StinkySteak

We are also experiencing a high crash rate after integrating Google UMP. We are on Unity 2021.3.28f1.

Stack traces look like this: image

DenizPiri avatar Jan 02 '24 18:01 DenizPiri

@NVentimiglia @EAdemov @DenizPiri This is unrelated to the original issue, but I found this known issue https://issuetracker.unity3d.com/issues/android-crash-on-android-when-androidjavaproxy-is-calling-from-multiple-threads inside Unity Release Notes. Although it says fixed for certain versions, but it is still added as a known issue in LTS latest releases so I am a bit confused about the situation of this bug. There are very little details on the bug report so hard to confirm what kind of crashes occur. Do you think this could be a reason for the higher crash rates since the plugin uses AndroidJavaProxy?

Also found this issue - https://issuetracker.unity3d.com/issues/android-crash-on-slash-apex-slash-com-dot-android-dot-runtime-slash-lib64-slash-bionic-slash-libc-dot-so-abort-plus-168-when-the-jni-table-gets-overflowed-with-java-dot-lang-dot-integer-and-java-dot-lang-dot-class-java-dot-lang-dot-integer which is once again related to AndroidJavaObjects and Proxy.

Hope this information is helpful.

shobhit-samaria avatar Jan 05 '24 17:01 shobhit-samaria

Hey!

I will have to try this, we are using 2021.3.20f1 version so it's a possibility. Not sure about the second problem. Seems like unrelated to the SDK.

I found something else tho. We are using another Ads SDK - IronSource when we added Google Ads SDK and implemented Consent Screen we experienced higher crash rate. Temporary fix was removing the Consent Screen flow implementation. But crashes continued. Lastly I completely removed the Google Ads SDK and after couple days seems like crash rate normalized. It could be something to do with the combination of the Ads SDKs.

EAdemov avatar Jan 05 '24 19:01 EAdemov

Thanks for the update.

NVentimiglia avatar Jan 10 '24 19:01 NVentimiglia

@EAdemov , same here. I'm using:

  • Unity: 2020.3.28f1.
  • Ironsource 7.5.0
  • Google Mobile Ads 8.5.1 (just for show CMP)

minnleee avatar Jan 14 '24 16:01 minnleee

@minnleee

Thanks, for the time being make sure you call MobileAds.RaiseAdEventsOnUnityMainThread = true; from the Start() method and outside of the callback scope.

NVentimiglia avatar Jan 16 '24 23:01 NVentimiglia

@NVentimiglia

If there is a possibility that there is a problem with the synchronization context, I do not think that setting the MobileAds.RaiseAdEventsOnUnityMainThread = true; process in the Start() function will do any good.

I think the best way to make this work is to set the synchronization context in the RaiseAdEventsOnUnityMainThread property instead of a static constructor and document that it should be made sure to be called in Start() etc.

private static bool _raiseAdEventsOnUnityMainThread;
public static bool RaiseAdEventsOnUnityMainThread
{
        get => _raiseAdEventsOnUnityMainThread;
        set
        {
            _raiseAdEventsOnUnityMainThread = value;
            SetUnityMainThreadSynchronizationContext();
        });
}

A second option is to create a temporary gameobject using RuntimeInitializeOnLoadMethod and set the synchronization context in the Start() method. This way the user can set RaiseAdEventsOnUnityMainThread at any time.

By the way, for those who use a different mediation like me and only want to install UMP, you should release Ump as a separate SDK. There are no dependencies other than MobileAds.RaiseAction() and a few codes.

KarahanOnarlar avatar Jan 29 '24 02:01 KarahanOnarlar

@NVentimiglia

If there is a possibility that there is a problem with the synchronization context, I do not think that setting the MobileAds.RaiseAdEventsOnUnityMainThread = true; process in the Start() function will do any good.

I think the best way to make this work is to set the synchronization context in the RaiseAdEventsOnUnityMainThread property instead of a static constructor and document that it should be made sure to be called in Start() etc.

private static bool _raiseAdEventsOnUnityMainThread;
public static bool RaiseAdEventsOnUnityMainThread
{
        get => _raiseAdEventsOnUnityMainThread;
        set
        {
            _raiseAdEventsOnUnityMainThread = value;
            SetUnityMainThreadSynchronizationContext();
        });
}

A second option is to create a temporary gameobject using RuntimeInitializeOnLoadMethod and set the synchronization context in the Start() method. This way the user can set RaiseAdEventsOnUnityMainThread at any time.

By the way, for those who use a different mediation like me and only want to install UMP, you should release Ump as a separate SDK. There are no dependencies other than MobileAds.RaiseAction() and a few codes.

You may be right, I have recommended that this is fixed and made more error-proof.

NVentimiglia avatar Jan 29 '24 20:01 NVentimiglia

@minnleee

Thanks, for the time being make sure you call MobileAds.RaiseAdEventsOnUnityMainThread = true; from the Start() method and outside of the callback scope.

Q1- @NVentimiglia can we call MobileAds.RaiseAdEventsOnUnityMainThread = true; from Awake? If no any specific reason to call from Start.

Q2- If we have two scripts 1 is managing UMP and 1 is Managing Ads, can i have to call MobileAds.RaiseAdEventsOnUnityMainThread = true; in both scripts or once in any script?

I am avg programmer, tolerate our basic questions. ;) Thanks

MuhammadWaqasOfficial avatar Mar 07 '24 06:03 MuhammadWaqasOfficial

Q2 (seeing MobileAds is singleton and RaiseAdEventsOnUnityMainThread is static bool, i think calling MobileAds.RaiseAdEventsOnUnityMainThread = true; only one time is enough)

MuhammadWaqasOfficial avatar Mar 08 '24 06:03 MuhammadWaqasOfficial

@NVentimiglia please reply and guide us on time. millions of dollars worth business depends on you :)

MuhammadWaqasOfficial avatar Mar 08 '24 10:03 MuhammadWaqasOfficial

@MuhammadWaqasOfficial

Yes, you can call MobileAds.RaiseAdEventsOnUnityMainThread = true as soon as the app loads before UMP.

NVentimiglia avatar Mar 13 '24 23:03 NVentimiglia