Merge branch 'fix/native-boundary-guards' into scratch/integrate-d2-d3

2026-08-22 19:57:11 -05:00
parent a6baf41866 7db320018a
commit 07933c80c5
12 changed files with 520 additions and 45 deletions
@@ -0,0 +1,151 @@
+package org.libremediaconverter.convert
+
+import android.app.Application
+import android.net.Uri
+import android.os.Looper
+import androidx.media3.common.util.UnstableApi
+import androidx.work.workDataOf
+import kotlinx.coroutines.Dispatchers
+import org.junit.After
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertNotNull
+import org.junit.Assert.assertNull
+import org.junit.Before
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.libremediaconverter.model.InputKind
+import org.libremediaconverter.model.InputProbe
+import org.libremediaconverter.work.ConversionWorker
+import org.robolectric.RobolectricTestRunner
+import org.robolectric.RuntimeEnvironment
+import org.robolectric.Shadows.shadowOf
+import java.util.concurrent.TimeUnit
+
+/**
+ * That a probe which throws leaves a screen the user can act on, not a dead coroutine.
+ *
+ * [MediaProbeNativeLoadTest] covers the boundary itself. This covers the other half of the
+ * same defect: `onInputPicked` runs inside `viewModelScope.launch`, so anything the probe
+ * throws and does not handle leaves the launch with no result at all — the file card never
+ * fills in, and on a device the default handler takes the process down.
+ *
+ * The seam is what makes that testable. Injecting a prober that throws reproduces the
+ * condition exactly, without depending on which types FFmpegKit happens to throw today.
+ */
+@UnstableApi
+@RunWith(RobolectricTestRunner::class)
+class ConversionViewModelProbeFailureTest {
+
+    private lateinit var app: Application
+
+    @Before
+    fun setUp() {
+        app = RuntimeEnvironment.getApplication()
+        installTestWorkManager(app, workDataOf(ConversionWorker.KEY_OUTPUT_PATH to "/dev/null"))
+    }
+
+    @After
+    fun tearDown() {
+        ConversionDependencies.reset()
+    }
+
+    /**
+     * The exact observed failure: FFmpegKit's loader rethrows a bare [Error] whose cause is
+     * the `UnsatisfiedLinkError` `System.loadLibrary` raised.
+     */
+    @Test
+    fun `a native load failure during the probe reports an unreadable file`() {
+        ConversionDependencies.probe = { _, _ ->
+            throw Error(
+                "FFmpegKit failed to start on brand: robolectric.",
+                UnsatisfiedLinkError("dlopen failed: library \"libffmpegkit.so\" not found"),
+            )
+        }
+
+        val probe = pickedProbe()
+
+        assertNotNull("the pick must finish; a thrown Error used to abandon the launch", probe)
+        assertEquals(InputKind.UNPARSEABLE, probe?.kind)
+        assertEquals(InputProbe.UNPARSEABLE, probe?.videoCodec)
+    }
+
+    /** Every touch after the first throws this instead, so the guard has to cover it too. */
+    @Test
+    fun `a NoClassDefFoundError from a poisoned class reports an unreadable file`() {
+        ConversionDependencies.probe = { _, _ ->
+            throw NoClassDefFoundError("Could not initialize class com.arthenica.ffmpegkit.FFmpegKitConfig")
+        }
+
+        assertEquals(InputKind.UNPARSEABLE, pickedProbe()?.kind)
+    }
+
+    /**
+     * The line the guard must not cross.
+     *
+     * `MediaProbe` spawns a native process, so an [OutOfMemoryError] raised in it is a real
+     * one about this JVM, not a report about the file. Swallowing it would turn "the device
+     * is out of memory" into "this video looks unreadable" and let the app carry on in a
+     * state it cannot honour — which is the regression a blanket `catch (Throwable)` would
+     * have introduced, and the reason this defect was left open rather than fixed carelessly.
+     */
+    @Test
+    fun `an OutOfMemoryError is not swallowed`() {
+        ConversionDependencies.probe = { _, _ -> throw OutOfMemoryError("Failed to allocate 512 MB") }
+
+        val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
+        viewModel.onInputPicked(INPUT)
+
+        // The observable difference, and the reason this is asserted on state rather than on a
+        // caught throwable: the probe hop is on Dispatchers.IO, so an error that escapes lands
+        // on that thread's handler rather than at this call. What must not happen is the card
+        // filling in with an "unreadable" verdict the app would then act on.
+        val settled = settle(viewModel)
+        assertEquals(ConversionState.Ready(InputFile(INPUT, "input", 0L)), settled)
+        assertNull("an OOM must not be reported as a probe result", (settled as ConversionState.Ready).input.probe)
+    }
+
+    /** A working probe is untouched by any of this. */
+    @Test
+    fun `a probe that succeeds still fills the card in`() {
+        ConversionDependencies.probe = { _, _ -> InputProbe(videoCodec = "h264", kind = InputKind.VIDEO) }
+
+        assertEquals("h264", pickedProbe()?.videoCodec)
+    }
+
+    /**
+     * Drives a real pick and returns the probe the card ended up with.
+     *
+     * Asserting on the probe rather than merely on `Ready` is deliberate: `onInputPicked`
+     * sets `Ready` *before* it probes, so a test that only checked the state would have
+     * passed against the unguarded code.
+     */
+    private fun pickedProbe(): InputProbe? {
+        val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
+        viewModel.onInputPicked(INPUT)
+        val ready = awaitState(viewModel.state, "Ready with a probe") {
+            it is ConversionState.Ready && it.input.probe != null
+        }
+        assertNull("nothing here should reach a terminal failure", (ready as? ConversionState.Failed))
+        return (ready as ConversionState.Ready).input.probe
+    }
+
+    /**
+     * Pumps the looper the way [awaitState] does, but for a fixed span and without requiring
+     * anything to happen — here "the pick never came back" is the expected outcome, so there
+     * is no predicate to wait on.
+     */
+    private fun settle(viewModel: ConversionViewModel): ConversionState {
+        val deadline = System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(SETTLE_MS)
+        while (System.nanoTime() < deadline) {
+            shadowOf(Looper.getMainLooper()).idle()
+            Thread.sleep(POLL_MS)
+        }
+        return viewModel.state.value
+    }
+
+    private companion object {
+        val INPUT: Uri = Uri.parse("content://test/holiday.mp4")
+        const val SETTLE_MS = 500L
+        const val POLL_MS = 5L
+    }
+}
@@ -0,0 +1,93 @@
+package org.libremediaconverter.convert
+
+import android.net.Uri
+import com.arthenica.ffmpegkit.FFmpegKitConfig
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertTrue
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.libremediaconverter.model.InputKind
+import org.libremediaconverter.model.InputProbe
+import org.robolectric.RobolectricTestRunner
+import org.robolectric.RuntimeEnvironment
+
+/**
+ * That a failed native load is reported, not thrown.
+ *
+ * The JVM is the only place this is reachable: there are no `.so` files here by
+ * construction, which is exactly the shape a corrupted install or an ABI mismatch has on a
+ * device. So the condition that cannot be provoked on working hardware is free here, and
+ * these tests are the only ones that can exercise it at all.
+ */
+@RunWith(RobolectricTestRunner::class)
+class MediaProbeNativeLoadTest {
+
+    /**
+     * What the boundary actually throws, pinned against the library rather than assumed.
+     *
+     * This is the test that justifies the shape of the guard, and it contradicts the obvious
+     * guess. `NativeLoader.loadLibrary` catches `UnsatisfiedLinkError` from
+     * `System.loadLibrary` and rethrows `java.lang.Error(message, cause)` — so
+     * `UnsatisfiedLinkError` never escapes, and because a bare `Error` *is* an `Error`, JLS
+     * 12.4.2 propagates it out of the static initialiser unwrapped rather than boxing it in
+     * `ExceptionInInitializerError`. Catching either of those two named types would catch
+     * nothing at all.
+     *
+     * The second touch of the class is a different type again — `NoClassDefFoundError`, the
+     * JVM's own "this class already failed to initialise" — so a guard written for one shape
+     * lets the other through. Both are asserted, in whichever order this classloader reaches
+     * them.
+     */
+    @Test
+    fun `loading FFmpegKit without its native library throws an Error, not an Exception`() {
+        val thrown: Throwable? = runCatching { FFmpegKitConfig.getLogLevel() }.exceptionOrNull()
+
+        // The whole defect in one assertion: `catch (e: Exception)` could never have seen this.
+        assertTrue(
+            "expected the native load to fail with something no catch (e: Exception) can see, got $thrown",
+            thrown !is Exception,
+        )
+        assertTrue("expected an Error, got $thrown", thrown is Error)
+        val error = thrown as Error
+        // Either the first touch (bare Error wrapping UnsatisfiedLinkError) or a later one
+        // (NoClassDefFoundError). Both are native-load failures; neither is a VirtualMachineError.
+        assertTrue(
+            "expected a bare Error caused by UnsatisfiedLinkError or a NoClassDefFoundError, got $error",
+            error is NoClassDefFoundError || error.cause is UnsatisfiedLinkError,
+        )
+    }
+
+    /**
+     * The defect itself: picking a file must not die because FFprobe could not start.
+     *
+     * `probeWithFFprobe` guarded its call with `catch (e: Exception)`, which an `Error` walks
+     * straight through. With neither probe able to read the file, the designed answer is the
+     * unparseable probe — "nothing could read it, route it to FFmpeg" — not a throw.
+     */
+    @Test
+    fun `probe reports an unreadable input instead of throwing when FFprobe cannot start`() {
+        val probe = MediaProbe.probe(RuntimeEnvironment.getApplication(), CONTENT_URI)
+
+        assertEquals(InputKind.UNPARSEABLE, probe.kind)
+        assertEquals(InputProbe.UNPARSEABLE, probe.videoCodec)
+    }
+
+    /**
+     * The second call takes the other branch — `NoClassDefFoundError` rather than the bare
+     * `Error` — so a guard that covered only the first shape would still crash every pick
+     * after the first one.
+     */
+    @Test
+    fun `a second probe is guarded too, though the JVM throws a different Error by then`() {
+        val first = MediaProbe.probe(RuntimeEnvironment.getApplication(), CONTENT_URI)
+        val second = MediaProbe.probe(RuntimeEnvironment.getApplication(), CONTENT_URI)
+
+        assertEquals(InputKind.UNPARSEABLE, first.kind)
+        assertEquals(InputKind.UNPARSEABLE, second.kind)
+    }
+
+    private companion object {
+        /** `content://` so the probe takes the SAF branch, which is what a real pick does. */
+        val CONTENT_URI: Uri = Uri.parse("content://test/holiday.mp4")
+    }
+}
@@ -0,0 +1,89 @@
+package org.libremediaconverter.ffmpeg
+
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertTrue
+import org.junit.Test
+
+/**
+ * Where the guard draws its line.
+ *
+ * The whole point of naming the predicate was that "catch what a failed native load throws"
+ * and "do not swallow an OutOfMemoryError in a method that spawns a native process" are two
+ * requirements a catch clause cannot express together. These are that pair, written down.
+ */
+class NativeLoadFailureTest {
+
+    // --- Recognised: the installation is broken, not this JVM ------------------------------
+
+    /**
+     * FFmpegKit's own shape. `NativeLoader.loadLibrary` catches the `UnsatisfiedLinkError`
+     * that `System.loadLibrary` raises and rethrows a bare `java.lang.Error` wrapping it, so
+     * the type carries no information and the cause is what identifies it.
+     */
+    @Test
+    fun `a bare Error wrapping an UnsatisfiedLinkError is a native load failure`() {
+        val error = Error("FFmpegKit failed to start on brand: robolectric.", UnsatisfiedLinkError("dlopen failed"))
+
+        assertTrue(isNativeLoadFailure(error))
+    }
+
+    /** Every touch of the class after the first one, which is the easier half to miss. */
+    @Test
+    fun `a NoClassDefFoundError is a native load failure`() {
+        val error = NoClassDefFoundError("Could not initialize class com.arthenica.ffmpegkit.FFmpegKitConfig")
+
+        assertTrue(isNativeLoadFailure(error))
+    }
+
+    /** What the first touch looked like when the JVM wrapped the failing initializer. */
+    @Test
+    fun `an ExceptionInInitializerError is a native load failure`() {
+        assertTrue(isNativeLoadFailure(ExceptionInInitializerError("Exception java.lang.Error: FFmpegKit failed")))
+    }
+
+    /** If a later FFmpegKit stops wrapping, the raw error is recognised on its own. */
+    @Test
+    fun `a plain UnsatisfiedLinkError is a native load failure`() {
+        assertTrue(isNativeLoadFailure(UnsatisfiedLinkError("dlopen failed: libffmpegkit.so not found")))
+    }
+
+    // --- Not recognised: this JVM is in trouble and must be allowed to say so ---------------
+
+    /**
+     * The regression the narrow guard exists to prevent. `catch (Throwable)` here would report
+     * "out of memory" to the user as "this file looks unreadable".
+     */
+    @Test
+    fun `an OutOfMemoryError is not a native load failure`() {
+        assertFalse(isNativeLoadFailure(OutOfMemoryError("Failed to allocate a 512 MB allocation")))
+    }
+
+    @Test
+    fun `a StackOverflowError is not a native load failure`() {
+        assertFalse(isNativeLoadFailure(StackOverflowError()))
+    }
+
+    @Test
+    fun `an AssertionError is not a native load failure`() {
+        assertFalse(isNativeLoadFailure(AssertionError("a broken invariant is not a broken install")))
+    }
+
+    /**
+     * A bare `Error` on its own says nothing. Only the `UnsatisfiedLinkError` underneath it
+     * makes it FFmpegKit's, so matching the type alone would be a blanket catch wearing a
+     * predicate's clothes.
+     */
+    @Test
+    fun `a bare Error with no cause is not a native load failure`() {
+        assertFalse(isNativeLoadFailure(Error("something else went wrong")))
+    }
+
+    /** An OOM does not become catchable by acquiring a cause. */
+    @Test
+    fun `an OutOfMemoryError caused by something else is still not a native load failure`() {
+        val error = OutOfMemoryError("Java heap space")
+        error.initCause(IllegalStateException("some unrelated cause"))
+
+        assertFalse(isNativeLoadFailure(error))
+    }
+}