fix(banner): prevent NullPointerException from mAdView reassignment race#434
Open
GingerAdonis wants to merge 1 commit into
Open
fix(banner): prevent NullPointerException from mAdView reassignment race#434GingerAdonis wants to merge 1 commit into
GingerAdonis wants to merge 1 commit into
Conversation
BannerExecutor reads the shared mutable `mAdView` field from posted UI-thread tasks and from AdListener callbacks. When the field is nulled or reassigned between posting a task and running it (rapid show/remove cycles, or a stale onAdFailedToLoad from an already-removed AdView), the task operates on the wrong view or crashes. Most visibly this surfaces as a NullPointerException in createNewAdView -> AdViewIdHelper.assignIdToAdView (setAdUnitId on a null AdView); updateExistingAdView can NPE on loadAd the same way. Capture the AdView instance per call and skip tasks/callbacks whose view is no longer the current banner.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The banner crashes with a
NullPointerExceptionwhen banners are shown/removed in quick succession (common for apps that drive a banner from route changes):Root cause
BannerExecutorholds the banner in a single mutable field,mAdView. Work posted to the UI thread (createNewAdView,updateExistingAdView) and theAdListenercallbacks all read/write the field at execution time, rather than theAdViewinstance they were created for.Capacitor dispatches plugin calls on a single background thread, so
showBanner/removeBannerbodies never overlap — but the GMA SDK's ad callbacks fire independently on the UI thread. That opens a race:showBannercreatesadViewA, callsloadAd(), attaches a listener.removeBannerruns →adViewA.destroy(),mAdView = null.adViewA's load is still in flight.showBannerruns again →mAdView = adViewB, postscreateNewAdView's UI task.adViewA's load now fails →onAdFailedToLoadfires foradViewA's listener. Itsif (mAdView != null)check seesadViewB, so it destroysadViewBand nulls the field.createNewAdView's task finally runs →AdViewIdHelper.assignIdToAdView(mAdView, …)withmAdView == null→setAdUnitIdNPE.The same shared-field hazard lets
updateExistingAdViewNPE onmAdView.loadAd(...).Fix
Bind each posted UI task and the
AdListenerto theAdViewinstance they belong to (a capturedfinallocal) instead of the shared field:createNewAdView/updateExistingAdViewcapture theAdViewand return early if it is no longer the currentmAdViewwhen the UI task runs.AdListenercallbacks return early when theirAdViewis no longer current, so a staleonAdFailedToLoad/onAdLoadedfrom an already-removed banner can no longer tear down — or emit events for — the current banner.Behavior note: a stale
onAdFailedToLoadno longer emitsSizeChanged(0,0)/FailedToLoad. Those events described a banner the JS layer had already discarded, so emitting them was itself spurious.Testing
This is a threading-correctness change; the race depends on GMA SDK callback timing and isn't practically reproducible in a unit test. Verified by reasoning through Capacitor's single bridge-thread + UI-thread callback model. The file is unchanged by
prettier --plugin=prettier-plugin-java;cd android && ./gradlew clean buildto confirm compilation.