Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HB-6075] Improve IsValid logic for Ad Disposal #152

Conversation

SCastanedaMunoz
Copy link
Collaborator

@SCastanedaMunoz SCastanedaMunoz commented Jun 26, 2023

Description

Current implementation of IsValid logic worked perfectly fine for iOS for the following scenarios:

  • Manual Disposal using Destroy/Invalidate ✔️
  • Garbage Collection Disposal using Destroy Invalidate ✔️

For Android the following scenarios are the case:

  • Manual Disposal using Destroy/Invalidate ✔️
  • Garbage Collection Disposal using Destroy Invalidate ❌

Although the approaches above were tested during the development of the fullscreen API changes and all scenarios were working fine. It seems like official release of 4.3.0 might have had threading changes that affect how Unity retains the AndroidJavaObject reference.

As such, the main problem is that the AndroidJavaObject reference gets disposed from the Unity side of things before we the finalizer for the C# objects is called. This means that whenever we try to call Invalidate or Destroy, the action is performed on an invalid pointer thus causing a crash. This happens into a different thread from the Unity main thread, when calling such code from the Unity main thread, we get a null reference exception.

The solution implemented was to create an AdStore, similar to how we do in iOS. This AdStore lives purely on the Native layer, there we keep a reference to our Android ads. Whenever an Ad is disposed, either manually or automatically by the GC, we call the method to remove such ad from the AdStore, which on itself calls the invalidate/destroy methods accordingly properly disposing of the native Android ad.

By implementing this solution we tackle the issue above, but we also provide an improvement on the ad integration experience for the Chartboost Mediation Unity SDK. Currently, if users do not dispose of Ads manually, they will remain on the application, which can end up in edge scenarios where a banner view, or a fullscreen ad leaks through the application lifetime. This fix makes sure that if an Ad is not manually disposed, it will wait until it is GC, and then call the appropriate methods.

It is important to notice that even though this solution makes it so the GC properly disposed of the native ads, this is not the most optimized integration. The most optimized integration will always be to properly manage and dispose the ad instances when no longer needed.

Finally, we have added a warning for when ads get disposed by the GC. It is as follows:

EventProcessor.ReportUnexpectedSystemError($"Interstitial Ad with placement: {placementName}, got GC. Make sure to properly dispose of ads utilizing Destroy for the best integration experience.");

Similar warnings have been added for all kind of placements.

@SCastanedaMunoz SCastanedaMunoz added bug Something isn't working bugfix Something has been fixed labels Jun 26, 2023
@SCastanedaMunoz SCastanedaMunoz requested a review from a team June 26, 2023 13:02
@SCastanedaMunoz SCastanedaMunoz self-assigned this Jun 26, 2023
@SCastanedaMunoz SCastanedaMunoz requested review from bwised, kushG and cb-jpadilla and removed request for a team June 26, 2023 13:02
@github-actions
Copy link

Test Results

1 tests  ±0   1 ✔️ ±0   3s ⏱️ +3s
1 suites ±0   0 💤 ±0 
1 files   ±0   0 ±0 

Results for commit 2dbb9bf. ± Comparison against base commit f78457f.

Copy link
Collaborator

@pleasesavemycat pleasesavemycat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@cb-jpadilla cb-jpadilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@SCastanedaMunoz SCastanedaMunoz merged commit 6c80088 into develop Jun 27, 2023
@SCastanedaMunoz SCastanedaMunoz deleted the HB-6075-mediation-unity-improve-is-valid-logic-for-deprecated-api branch June 27, 2023 02:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working bugfix Something has been fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants