fix: [SDK-4983] destroy IAM WebView on dismiss to stop Activity leaks

[abdulraqeeb33]

Summary

• Fixes LeakCanary Activity leak from OSWebView after In-App Message dismiss (#2312, SDK-4983).
• Destroy and null the IAM WebView on all dismiss paths (back/system PopupWindow, close action, drag, timer, programmatic), and clear InAppDisplayer.lastInstance via an onDismissed callback.
• Adds unit coverage for destroy/callback behavior and idempotent repeated dismiss.

Test plan

• ./gradlew :OneSignal:in-app-messages:testOriginalUnitTest --tests com.onesignal.inAppMessages.internal.display.WebViewManagerDismissCleanupTests
• Reproduce #2312: show IAM on Activity A → dismiss (Back) → start Activity B → confirm LeakCanary no longer reports OSWebView retaining Activity A
• Sanity-check IAM still displays/dismisses correctly across close button, drag-to-dismiss, timed dismiss, and activity rotation while showing

[github-actions]

📊 Diff Coverage Report

Diff Coverage Report (Changed Lines Only)

Gate: aggregate coverage on changed executable lines must be ≥ 80% (JaCoCo line data for lines touched in the diff).

Changed Files Coverage

• InAppDisplayer.kt: 11/11 touched executable lines (100.0%) (19 touched lines in diff)
• WebViewManager.kt: 137/166 touched executable lines (82.5%) (286 touched lines in diff)

Overall (aggregate gate)

148/177 touched executable lines covered (83.6% — requires ≥ 80%)

📥 View workflow run

[fadi-george]

Possible lifecycle regressions:

1. Dismissing while messageView == null runs cleanup without messageWasDismissed, leaving IAM state stuck as showing and blocking future IAMs.
2. Cleanup can race WebView setup, causing webView!! crashes or creating an orphaned WebView after ownership is cleared.

Dismissal also isn’t thread-safe or fully idempotent, so concurrent or normal paths may run cleanup twice. Could we serialize setup/dismissal behind a terminal dismissed state and add tests for early dismissal, lifecycle completion, and concurrent dismissal?

[abdulraqeeb33]

@fadi-george Good catches — addressed in 8bf28ab:

1. Early dismiss (messageView == null) now always fires messageWillDismiss + messageWasDismissed via a single-flight finishDismiss(), so IAM queue state cannot stay stuck as showing.
2. Setup/dismiss races: dismissed is terminal; setupWebView / createNewInAppMessageView / render-complete bail out after dismiss and destroy any in-flight WebView instead of touching webView!!.
3. Idempotency / concurrency: finishDismiss() and lifecycle notification run exactly once under a lock; added tests for early dismiss, lifecycle completion, concurrent dismiss, post-dismiss setup no-op, and InAppDisplayer.lastInstance clearing.

[fadi-george]

Thanks, the lifecycle, setup race, and single-flight cleanup issues appear addressed.

Two test gaps remain:

The concurrent dismissal test inherits runBlocking’s context, so the dismissals execute sequentially rather than exercising a real race.
The setup test calls setup after dismissal instead of dismissing while waitUntilActivityReady() is suspended.
CI also currently fails Detekt and the 80% diff coverage gate (55.8%).

[abdulraqeeb33]

Addressed in cf82995:

1. Concurrent dismiss — races on Dispatchers.Default (not runBlocking’s sequential context).
2. Setup race — dismisses while waitUntilActivityReady() is suspended (CompletableDeferred), with setup on Dispatchers.Main.
3. CI — Detekt cleaned up in WebViewManager (extracted helpers / exception typing). Local diff coverage is 81.8% (≥80%).

[fadi-george]

one blocker remains in InAppDisplayer.showMessageContent: lastInstance can now be cleared by the background onDismissed callback between the null check and lastInstance!!, causing an NPE when a preview arrives during dismissal

could we capture a local snapshot and synchronize or confine all lastInstance access? @volatile alone would not make the check-and-use atomic.

[abdulraqeeb33]

Good catch — that was mine. Making onDismissed clear lastInstance from a background thread turned the existing lastInstance!! into a live NPE, and I didn't audit the other readers when I changed the field's lifetime.

Fixed in 6ab4c56. lastInstance is now an AtomicReference and the preview path claims it in one step:

if (message.isPreview) {
    lastInstance.getAndSet(null)?.dismissAndAwaitNextMessage()
}

onDismissed uses compareAndSet(webViewManager, null) so it only clears its own instance, and dismissCurrentInAppMessage reads a local snapshot. Agreed that @Volatile wouldn't help here — getAndSet removes the check-and-use pair entirely rather than relying on a lock plus reviewer vigilance.

I also swept WebViewManager for the same pattern and fixed three more latent derefs on messageView, which finishDismiss likewise nulls from a background thread: calculateHeightAndShowWebViewAfterNewActivity now snapshots it, and onActivityAvailable / onActivityStopped use safe calls instead of !!.

On the tests: I tried to write a regression test for the NPE and it passed against the pre-fix code, so I removed it rather than leave something that can't fail. The window is two adjacent field reads and isn't reachable from outside the class, so the guarantee here is structural (getAndSet, no !! on shared state) plus deterministic coverage of the preview-takeover and already-cleared paths.

CI: Detekt clean; diff coverage 83.3% (WebViewManager 82.1%, InAppDisplayer 100%). Correcting my earlier number — the 81.8% I quoted was measured before the Detekt refactor was committed, so the script scored the new code against the old line set. Against the real origin/main...HEAD range that reading was 72.8%, and it's 83.3% now.

[abdulraqeeb33]

Ran a self-review pass over the branch before asking for another look. It found a real bug of my own making, fixed in 901a630, plus one behavior change I'd like your call on.

The bug I introduced

When I extracted local snapshots in showMessageView, I removed protection the original code had by accident. origin/main re-read the fields at each use:

messageView?.setWebView(webView!!)
messageView?.showView(activity)

so once setMessageView(null) ran, an in-flight show became a no-op. My version snapshotted both into locals and never re-validated them, so:

1. showMessageView passes the dismissed check and snapshots messageView + webView.
2. A dismiss completes on another thread: finishDismiss destroys the OSWebView, nulls messageView, and unregisters the lifecycle handler.
3. showMessageView resumes and calls setWebView(destroyedWebView) — re-arming the field, defeating the if (webView == null) return bail-out inside InAppMessageView — then showView attaches the PopupWindow.

The result is a popup holding a destroyed WebView attached to the live Activity with messageView == null, so nothing is left that can call removeAllViews() on it. For non-banner positions it's MATCH_PARENT and focusable, so it also silently eats all input. That is the exact failure mode this PR is supposed to eliminate.

Fix: claim both views under lifecycleLock (the same lock finishDismiss cleans up under) and publish setWebView there, re-check dismissed before attaching, and have finishDismiss tear the view down rather than only dereferencing it — the system-dismiss path (PopupWindowListener.onDismiss) calls onMessageWasDismissed without running removeAllViews first, so that teardown was missing entirely.

Also in the same commit:

• onMessageWasDisplayed now returns early when dismissed. The show animation ending after a dismiss fired onDidDisplay after onDidDismiss and POSTed an impression for a message the user never saw.
• setupWebView claims the WebView (webView = null) before destroying it, so a dismiss landing mid-setup can't destroy the same instance twice.
• webView, messageView, closing, lastPageHeight are @Volatile; they're written under the lock but read from the main, IO and JS-bridge threads.

Lock ordering came back clean: no suspend calls under lifecycleLock, only one order exists (messageViewMutex → lifecycleLock), and onDismissed fires after the lock is released.

Your call: never-displayed IAMs are now consumed

Worth flagging because it's a deliberate behavior change from origin/main and I don't think it should land silently.

Always firing messageWasDismissed is what unsticks the queue, but InAppMessagesManager.messageWasDismissed(message, failed = false) also persists dismissedMessagesId, sets lastTimeInAppDismissed, and calls persistInAppMessage — its own comment reads "Only increase IAM display quantity if IAM was truly displayed."

So an IAM dismissed before rendering_complete — IAMs paused (InAppMessagesManager:185), a prompt starting (:910), or a preview taking over (InAppDisplayer:111) — is now recorded as displayed-and-dismissed and never shows again. origin/main didn't consume it, but left inAppMessageIdShowing set and wedged the queue for the session, so I think this is a net improvement either way.

The manager already has the right mechanism (failed = true = retry next session). Routing never-rendered dismissals through it needs a "was displayed" signal threaded through IInAppLifecycleService and its handler, roughly five files. Happy to do that here or in a follow-up — which would you prefer?

Same gap exists if setupWebView throws (MissingWebViewPackageException, swallowed at InAppDisplayer:150-163): finishDismiss never runs, so lastInstance keeps the Activity and the queue stays stuck. Pre-existing, but contrary to this PR's invariant, and it wants the same decision.

Tests

Added a regression test for the orphaned view, and verified it by breaking the fix — it fails without it. My first attempt at that test passed against the broken code (dismissAndAwaitNextMessage already dereferences the view on the path I used), so I rewrote it to go through the system-dismiss path where finishDismiss is the only teardown.

167 IAM tests pass, Detekt clean, diff coverage 83.6% (WebViewManager 82.5%, InAppDisplayer 100%).