fix(webview): harden SafeWebViewLayout for renderer death after teardown

[mikehardy]

Note

Assisted-by: Cursor / Composer

Purpose / Description

ACRA on 2.25beta1 (#21952): after a WebView renderer death, SafeWebViewLayout recreated the WebView and notified the fragment even when that fragment's view was already gone. MultimediaImageFragment then hit requireView() via viewBinding and crashed — ironic, since we added this path so the default client would stop killing the whole process.

Fixing that skip-recreation path exposed a second, subtler problem: once we destroy the terminated WebView without replacing it, the layout still looks "alive" to every caller. destroy(), safeDestroy(), loadUrl, and friends all kept talking to a destroyed WebView, which Android forbids. The worst concrete case is the original teardown path itself — PageFragment.onDestroyView → safeDestroy() after a late onRenderProcessGone — which would double-destroy.

This PR therefore does more than "don't recreate after teardown." It gives SafeWebViewLayout an explicit state machine so crash cleanup and intentional owner teardown are no longer the same signal.

Fixes

• Fixes Fragment MultimediaImageFragment did not return a View from onCreateView() #21952

The signalling problem (why WebViewState exists)

What was conflated before

SafeWebViewLayout had no notion of "the inner WebView is gone." After a renderer death it either:

1. destroyed + recreated + notified the listener, or
2. (after the first fix attempt) destroyed and early-returned, leaving this.webView pointing at a destroyed instance while every public method still delegated to it.

Those two situations look identical from the outside: "no usable WebView right now." They are not the same:

Situation	How we got here	Should we ever recreate?
Crash cleanup while the layout is temporarily unusable (detached / fragment view gone / not in a fragment)	onRenderProcessGone skipped recreation after destroying the dead renderer	Yes, if the layout comes back under a live fragment view that can configure it
Owner teardown	destroy() / safeDestroy() (e.g. CardViewer stopping audio, PageFragment destroying on onDestroyView)	No — the owner ended the WebView's life on purpose

A single boolean isDestroyed conflates those. Attach-time recovery then either:

• never recovers after a crash skip (blank layout forever), or
• resurrects after intentional destroy() (new WebView nobody will tear down, possibly after onDestroyView already ran).

How the conflation caused specific errors

1. Fragment MultimediaImageFragment did not return a View from onCreateView() #21952 crash — recreate + onWebViewRecreated while fragment.view == null → requireView() / viewBinding crash.
2. Double-destroy — skip path destroys the WebView; later safeDestroy()/destroy() calls WebView.destroy() again → platform misuse (and a real crash risk on some devices).
3. Use-after-destroy — loadUrl / evaluateJavascript / etc. on the destroyed instance after a skip.
4. False resurrection (if we recovered from a boolean) — CardViewerFragment.onDestroyView calls destroy() to stop audio; a later detach/reattach would recreate a WebView into a fragment that already tore down.
5. Wrong-hierarchy recreate — fragment.view != null after the fragment's view was destroyed and reinflated, while the callback still holds the old layout → recreate into a discarded hierarchy.
6. Stale callback — onRenderProcessGone reports a WebView that is no longer the current child → remove/destroy the wrong instance.

How the three states solve it

private enum class WebViewState {
    ACTIVE,                 // live inner WebView; normal operation
    DESTROYED_RECOVERABLE,  // crash cleanup skipped recreation; may recover on reattach
    DESTROYED_TERMINAL,     // owner called destroy()/safeDestroy(); never recover
}

• Crash skip paths (findFragment fails, fragment.view == null, !isAttachedToWindow) → DESTROYED_RECOVERABLE, and all delegating APIs become logged no-ops.
• destroy() / safeDestroy() → DESTROYED_TERMINAL, same no-ops, no attach-time recovery.
• onAttachedToWindow recovery runs only when all of: state is DESTROYED_RECOVERABLE, fragment implements OnWebViewRecreatedListener, and this layout is a descendant of fragment.view. That last check stops orphaned layouts from a recreated fragment view from recovering into the wrong place.
• Successful recreate (immediate or on reattach) → ACTIVE again.

So the "is it destroyed?" signal is no longer one bit — it carries why, which is what decide-whether-to-recover needs.

Approach (by commit)

1. fix(webview): handle renderer death after fragment view is gone

   • Always removeView + destroy the terminated WebView first (Android requires cleanup even when not recreating).
   • Skip recreation when the host fragment is missing or fragment.view == null.
   • Introduce WebView state + post-destroy no-ops (including nullable createPrintDocumentAdapter; Statistics export skips with a log instead of crashing).
   • Regression: detach fragment → late onRenderProcessGone → assert destroy/remove/no replacement; also call safeDestroy() so the PageFragment teardown path cannot double-destroy.

2. fix(webview): ignore stale WebView instances in onRenderProcessGone

   • If the callback's WebView is not the current child, destroy that stale instance and leave the live child alone.

3. fix(webview): skip WebView recreation while detached from window

   • fragment.view != null is not enough after view destroy/reinflate; also skip when !isAttachedToWindow.

4. feat(webview): recover a crash-destroyed WebView on layout reattach

   • Split destroyed into recoverable vs terminal; recover on attach when safe (listener + live hierarchy).
   • Intentional destroy() stays terminal (no resurrection).

How Has This Been Tested?

• Robolectric MultimediaImageFragmentTest — issue Fragment MultimediaImageFragment did not return a View from onCreateView() #21952 path (detach + late onRenderProcessGone + safeDestroy).
• Robolectric SafeWebViewLayoutTest — double-destroy both call orders, stale WebView identity, detached-window skip, reattach recovery, intentional-destroy non-recovery, orphaned-layout non-recovery.
• Confirmed tests run once on the default Robolectric SDK (targetSdk). Do not use @Config(minSdk = O) here: it fans each test across every SDK ≥ 26 and forces android-all jar downloads that robolectricDownloader.gradle deliberately does not pre-cache. @SdkSuppress is inert under Robolectric anyway.
• ./gradlew :AnkiDroid:ktlintMainSourceSetCheck :AnkiDroid:ktlintTestSourceSetCheck

Learning (optional)

Same lifecycle class as #21893 (callback after view destroyed), different trigger. Real renderer death still needs an instrumented chrome://crash test; this PR covers the post-teardown and state-machine paths on the JVM.

Accepted tradeoffs left on purpose: settings / scale stay unguarded (no safe no-op return); Statistics PDF skip on a destroyed WebView is Timber-only (no fitting existing snackbar string without adding translations).

Checklist

• You have a descriptive commit message with a short title (first line, max 50 chars).
• You have commented your code, particularly in hard-to-understand areas
• You have performed a self-review of your own code
• UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
• UI Changes: You have tested your change using the Google Accessibility Scanner

[mikehardy]

stubbed toe on a number of testing issues that I'll pursue separately, related to #21739 mostly but also a bit of steering missing in the repo on how agents should pursue their work so they have all the backpressure they need based on what we've learned over time and encoded in CI / our wiki

[david-allison]

Subject: [PATCH] test(webview): assert cleanup after view teardown
---
Index: AnkiDroid/src/test/java/com/ichi2/anki/multimedia/MultimediaImageFragmentTest.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/multimedia/MultimediaImageFragmentTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/multimedia/MultimediaImageFragmentTest.kt
--- a/AnkiDroid/src/test/java/com/ichi2/anki/multimedia/MultimediaImageFragmentTest.kt	(revision 4ebd4ce2ad64f7421f4940f22c27a09b2dc3a290)
+++ b/AnkiDroid/src/test/java/com/ichi2/anki/multimedia/MultimediaImageFragmentTest.kt	(revision 96f5febde2fa0019748521f8492a496d2b7deaf1)
@@ -26,6 +26,7 @@
 import org.hamcrest.Matchers.nullValue
 import org.junit.Test
 import org.junit.runner.RunWith
+import org.robolectric.Shadows.shadowOf
 import java.io.File
 
 /** A picked or shared `file://` must only resolve to a file inside our own cache. */
@@ -74,15 +75,21 @@
 
     @Test
     @SdkSuppress(minSdkVersion = Build.VERSION_CODES.O)
-    fun `onRenderProcessGone after view is destroyed does not crash - issue 21952`() {
+    fun `onRenderProcessGone after view is destroyed cleans up the dead WebView - issue 21952`() {
         launchFragmentInContainer<MultimediaImageFragment>(
             bundleOf(EXTRA_MEDIA_OPTIONS to MultimediaImageFragment.ImageOptions.GALLERY),
         ).use { scenario ->
             scenario.onFragment { fragment ->
                 val layout = fragment.requireView().findViewById<SafeWebViewLayout>(R.id.multimedia_web_view)
+                val webView = layout.getChildAt(0) as WebView
                 fragment.parentFragmentManager.commitNow { detach(fragment) }
                 assertThat(fragment.view, nullValue())
-                layout.onRenderProcessGone(layout.getChildAt(0) as WebView)
+
+                layout.onRenderProcessGone(webView)
+
+                assertThat("the dead WebView is destroyed", shadowOf(webView).wasDestroyCalled(), equalTo(true))
+                assertThat("the dead WebView is removed from its parent", webView.parent, nullValue())
+                assertThat("no replacement WebView is created", layout.childCount, equalTo(0))
             }
         }
     }

[david-allison]

Unreviewed for correctness; as a nitpick, I'd like the tests to use view bindings

diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/workarounds/SafeWebViewLayoutTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/workarounds/SafeWebViewLayoutTest.kt
index 92107f6750..da222b4df0 100644
--- a/AnkiDroid/src/test/java/com/ichi2/anki/workarounds/SafeWebViewLayoutTest.kt
+++ b/AnkiDroid/src/test/java/com/ichi2/anki/workarounds/SafeWebViewLayoutTest.kt
@@ -149,6 +149,16 @@ class SafeWebViewLayoutTest : RobolectricTest() {
         }
     }
 
+    @Test
+    fun `destroy after crash cleanup prevents recovery on reattach`() {
+        assertOwnerTeardownPreventsRecovery { destroy() }
+    }
+
+    @Test
+    fun `safeDestroy after crash cleanup prevents recovery on reattach`() {
+        assertOwnerTeardownPreventsRecovery { safeDestroy() }
+    }
+
     @Test
     fun `recovery is skipped when orphaned layout reattaches outside fragment view`() {
         launchFragmentInContainer<SafeWebViewLayoutHostFragment>().use { scenario ->
@@ -218,4 +228,28 @@ class SafeWebViewLayoutTest : RobolectricTest() {
             }
         }
     }
+
+    private fun assertOwnerTeardownPreventsRecovery(teardown: SafeWebViewLayout.() -> Unit) {
+        launchFragmentInContainer<MultimediaImageFragment>(
+            bundleOf(EXTRA_MEDIA_OPTIONS to MultimediaImageFragment.ImageOptions.GALLERY),
+        ).use { scenario ->
+            scenario.onFragment { fragment ->
+                val layout = fragment.requireView().findViewById<SafeWebViewLayout>(R.id.multimedia_web_view)
+                val webView = layout.getChildAt(0) as WebView
+                val parent = layout.parent as ViewGroup
+                parent.removeView(layout)
+                assertThat(layout.isAttachedToWindow, equalTo(false))
+
+                layout.onRenderProcessGone(webView)
+                assertThat(shadowOf(webView).wasDestroyCalled(), equalTo(true))
+                assertThat(layout.childCount, equalTo(0))
+
+                layout.teardown()
+                parent.addView(layout)
+
+                assertThat(layout.isAttachedToWindow, equalTo(true))
+                assertThat("owner teardown must prevent crash recovery", layout.childCount, equalTo(0))
+            }
+        }
+    }
 }

[mikehardy]

your comments were both good catches - re-pushed