Skip to content

fix(banner): prevent NullPointerException from mAdView reassignment race#434

Open
GingerAdonis wants to merge 1 commit into
capacitor-community:mainfrom
GingerAdonis:fix/banner-adview-race-npe
Open

fix(banner): prevent NullPointerException from mAdView reassignment race#434
GingerAdonis wants to merge 1 commit into
capacitor-community:mainfrom
GingerAdonis:fix/banner-adview-race-npe

Conversation

@GingerAdonis
Copy link
Copy Markdown

Problem

The banner crashes with a NullPointerException when banners are shown/removed in quick succession (common for apps that drive a banner from route changes):

java.lang.NullPointerException: Attempt to invoke virtual method
'void com.google.android.gms.ads.AdView.setAdUnitId(java.lang.String)' on a null object reference
  at com.getcapacitor.community.admob.helpers.AdViewIdHelper.assignIdToAdView(AdViewIdHelper.java:28)
  at com.getcapacitor.community.admob.banner.BannerExecutor.lambda$createNewAdView$5(BannerExecutor.java:245)
  at android.os.Handler.handleCallback(Handler.java:1027)
  at android.os.Looper.loop(Looper.java:412)
  at android.app.ActivityThread.main(ActivityThread.java:9998)

Root cause

BannerExecutor holds the banner in a single mutable field, mAdView. Work posted to the UI thread (createNewAdView, updateExistingAdView) and the AdListener callbacks all read/write the field at execution time, rather than the AdView instance they were created for.

Capacitor dispatches plugin calls on a single background thread, so showBanner/removeBanner bodies never overlap — but the GMA SDK's ad callbacks fire independently on the UI thread. That opens a race:

  1. showBanner creates adViewA, calls loadAd(), attaches a listener.
  2. removeBanner runs → adViewA.destroy(), mAdView = null. adViewA's load is still in flight.
  3. showBanner runs again → mAdView = adViewB, posts createNewAdView's UI task.
  4. adViewA's load now fails → onAdFailedToLoad fires for adViewA's listener. Its if (mAdView != null) check sees adViewB, so it destroys adViewB and nulls the field.
  5. createNewAdView's task finally runs → AdViewIdHelper.assignIdToAdView(mAdView, …) with mAdView == nullsetAdUnitId NPE.

The same shared-field hazard lets updateExistingAdView NPE on mAdView.loadAd(...).

Fix

Bind each posted UI task and the AdListener to the AdView instance they belong to (a captured final local) instead of the shared field:

  • createNewAdView / updateExistingAdView capture the AdView and return early if it is no longer the current mAdView when the UI task runs.
  • The AdListener callbacks return early when their AdView is no longer current, so a stale onAdFailedToLoad / onAdLoaded from an already-removed banner can no longer tear down — or emit events for — the current banner.

Behavior note: a stale onAdFailedToLoad no longer emits SizeChanged(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 build to confirm compilation.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant