From 97fdc49f013097a66a9dd7bd82c8113dab2934a0 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sat, 22 Aug 2026 19:36:33 -0500 Subject: [PATCH 1/2] Guard the native boundary against what it actually throws D14: picking a file died instead of reporting an unreadable one when FFmpegKit's native library could not load. `probeWithFFprobe` guarded its call with `catch (e: Exception)`, and `ConversionViewModel.onInputPicked` guarded nothing, so the failure escaped a `viewModelScope.launch` -- which has no handler, and on a device ends the process. All three of the obvious narrow guards catch nothing, which is why this needed reading the AAR rather than guessing. `NativeLoader.loadLibrary` catches the `UnsatisfiedLinkError` that `System.loadLibrary` raises and rethrows a *bare* `java.lang.Error` wrapping it, so `UnsatisfiedLinkError` never escapes and the escaping type carries no information at all. Every touch of the class after the first is a different type again -- `NoClassDefFoundError` -- so a guard written for the first shape lets the second pick onwards crash, which is the harder half to notice. Both are in the test output verbatim. `catch (Throwable)` was the wrong answer for the reason the audit gave: it would swallow a genuine `OutOfMemoryError` in a method that spawns a native process, turning "this device is out of memory" into "this file looks unreadable" and letting the app act on it. So the line is drawn by a named predicate, `isNativeLoadFailure`, rather than by the catch clause -- every class-loading shape is a `LinkageError`, and nothing that means the JVM is failing is one. That disjointness is what makes the guard narrow. This is consistent with the position `config/detekt/detekt.yml` already takes for `TooGenericExceptionCaught`: the boundary's failure types are undocumented, so guessing crashes the app on a file it could have reported. One level up the opposite mistake is available too, and the predicate is what lets both be avoided at once. `TooGenericExceptionThrown` is relaxed for the test source sets only. A test that reproduces a failed native load has to throw what the library throws, and a tidier subclass would leave it passing against a defect it no longer reproduces. Main source is untouched by that and throws nothing generic. `ConversionDependencies.probe`'s KDoc is rewritten rather than left. It said this hazard was "deliberately not fixed here ... its own commit, with its own test", which this is -- leaving it would have replaced one true comment with a false one, which is the same defect class as the D11 work. Both halves are covered independently: reverting the `MediaProbe` catch reds only the two `MediaProbeNativeLoadTest` cases, reverting the ViewModel guard reds only the two injected-seam cases, and widening the ViewModel guard to `Throwable` reds the OutOfMemoryError case -- so the narrowness is pinned, not just the catch. Audited the sibling boundaries named in the audit and left all three alone: `FFmpegEngine` and `ConcatEngine` both construct and run under `catch (e: Throwable)` in their workers, and `Media3Engine` has no native loader of this kind and already routes failures through `runCatching`. Co-Authored-By: Claude Opus 5 (1M context) --- .../convert/ConversionViewModel.kt | 34 +++- .../libremediaconverter/convert/MediaProbe.kt | 32 +++- .../convert/Transcoders.kt | 24 +-- .../ffmpeg/NativeLoadFailure.kt | 59 +++++++ .../ConversionViewModelProbeFailureTest.kt | 151 ++++++++++++++++++ .../convert/MediaProbeNativeLoadTest.kt | 93 +++++++++++ .../ffmpeg/NativeLoadFailureTest.kt | 89 +++++++++++ config/detekt/detekt.yml | 11 ++ 8 files changed, 475 insertions(+), 18 deletions(-) create mode 100644 app/src/main/java/org/libremediaconverter/ffmpeg/NativeLoadFailure.kt create mode 100644 app/src/test/java/org/libremediaconverter/convert/ConversionViewModelProbeFailureTest.kt create mode 100644 app/src/test/java/org/libremediaconverter/convert/MediaProbeNativeLoadTest.kt create mode 100644 app/src/test/java/org/libremediaconverter/ffmpeg/NativeLoadFailureTest.kt diff --git a/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt b/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt index 805cb0f..4d90045 100644 --- a/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt +++ b/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt @@ -3,6 +3,7 @@ package org.libremediaconverter.convert import android.app.Application import android.net.Uri import android.provider.OpenableColumns +import android.util.Log import androidx.lifecycle.AndroidViewModel import androidx.lifecycle.viewModelScope import androidx.media3.common.util.UnstableApi @@ -20,6 +21,7 @@ import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch import kotlinx.coroutines.withContext +import org.libremediaconverter.ffmpeg.isNativeLoadFailure import org.libremediaconverter.model.AudioCodec import org.libremediaconverter.model.Container import org.libremediaconverter.model.ContainerCapabilities @@ -220,7 +222,7 @@ class ConversionViewModel @JvmOverloads constructor( // would read as the app having ignored the tap. _state.value = ConversionState.Ready(file) - val probe = withContext(Dispatchers.IO) { ConversionDependencies.probe(getApplication(), uri) } + val probe = withContext(Dispatchers.IO) { probeOrUnreadable(uri) } // Only fill in the probe if the user has not moved on in the meantime. _state.update { current -> if (current is ConversionState.Ready && current.input.uri == uri) { @@ -232,6 +234,34 @@ class ConversionViewModel @JvmOverloads constructor( } } + /** + * Probing, with the one failure the pick must survive rather than propagate. + * + * This runs inside `viewModelScope.launch`, which has no exception handler, so anything + * that escapes here abandons the launch — the file card never fills in — and reaches the + * thread's default handler, which on a device takes the process down. Picking a file is + * not a place to crash from. + * + * The one condition that reaches this is FFmpegKit's native library failing to load, + * which arrives as an `Error` rather than an `Exception`; [MediaProbe] handles its own + * FFprobe call now, and this covers the seam and the platform extractor beside it. The + * answer is [MediaProbe.UNREADABLE] — the same value [MediaProbe.probe] returns when + * neither of its probes could read the file, because that is what has happened. + * + * Anything else is rethrown deliberately. An [OutOfMemoryError] here is about this + * process, not about this file, and reporting it as an unreadable video would let the app + * carry on in a state it cannot honour. See + * [org.libremediaconverter.ffmpeg.isNativeLoadFailure] for which is which and why the + * distinction is drawn by a predicate rather than by the catch clause. + */ + private fun probeOrUnreadable(uri: Uri): InputProbe = try { + ConversionDependencies.probe(getApplication(), uri) + } catch (e: Error) { + if (!isNativeLoadFailure(e)) throw e + Log.w(TAG, "Could not probe $uri; reporting it as unreadable.", e) + MediaProbe.UNREADABLE + } + /** * Enqueues the conversion rather than running it inline. * @@ -420,5 +450,7 @@ class ConversionViewModel @JvmOverloads constructor( * it "unknown" would read as an error rather than as a gap in what survived. */ const val UNKNOWN_INPUT_NAME = "Media file" + + const val TAG = "ConversionViewModel" } } diff --git a/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt b/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt index 0fa80ae..f98cfec 100644 --- a/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt +++ b/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt @@ -8,6 +8,7 @@ import android.util.Log import com.arthenica.ffmpegkit.FFmpegKitConfig import com.arthenica.ffmpegkit.FFprobeKit import com.arthenica.ffmpegkit.MediaInformation +import org.libremediaconverter.ffmpeg.isNativeLoadFailure import org.libremediaconverter.model.ConcatInput import org.libremediaconverter.model.Container import org.libremediaconverter.model.InputKind @@ -33,6 +34,21 @@ import org.libremediaconverter.model.InputProbe */ object MediaProbe { + /** + * What [probe] reports when nothing could read the input. + * + * Named rather than inlined because a caller that has to handle [probe] itself failing + * needs to land on the same answer — see `ConversionViewModel.onInputPicked`. Two + * different spellings of "unreadable" would be two different behaviours downstream, since + * the router keys off [InputProbe.UNPARSEABLE] and the source-info card off the kind. + */ + val UNREADABLE = InputProbe( + videoCodec = InputProbe.UNPARSEABLE, + hasVideo = true, + durationMs = 0, + kind = InputKind.UNPARSEABLE, + ) + fun probe(context: Context, uri: Uri): InputProbe { val extracted = probeWithExtractor(context, uri) val info = probeWithFFprobe(context, uri) @@ -45,12 +61,7 @@ object MediaProbe { // Not a failure: an unparseable input is a strong signal that this job belongs on // FFmpeg. Reporting an unknown codec makes the router say so. Log.i(TAG, "Neither MediaExtractor nor FFprobe could read $uri; routing to FFmpeg.") - return InputProbe( - videoCodec = InputProbe.UNPARSEABLE, - hasVideo = true, - durationMs = 0, - kind = InputKind.UNPARSEABLE, - ) + return UNREADABLE } return InputProbe( @@ -145,6 +156,15 @@ object MediaProbe { } catch (e: Exception) { Log.i(TAG, "FFprobe could not read $uri.", e) null + } catch (e: Error) { + // Touching FFmpegKit at all loads its native library, and a failure there arrives as + // an Error, which the clause above cannot see -- so an unloadable library used to + // take the whole file pick down instead of reporting an unreadable file. Anything + // that is not that library failing to load is still this JVM's problem, not this + // file's, and is rethrown: see isNativeLoadFailure. + if (!isNativeLoadFailure(e)) throw e + Log.w(TAG, "FFmpegKit's native library could not be loaded; probing $uri without FFprobe.", e) + null } private fun readMediaInformation(path: String): FFprobeInfo? { diff --git a/app/src/main/java/org/libremediaconverter/convert/Transcoders.kt b/app/src/main/java/org/libremediaconverter/convert/Transcoders.kt index acdd654..0a0b407 100644 --- a/app/src/main/java/org/libremediaconverter/convert/Transcoders.kt +++ b/app/src/main/java/org/libremediaconverter/convert/Transcoders.kt @@ -80,18 +80,20 @@ object ConversionDependencies { * * Here for a reason the others are not, and the reason is worth recording rather than * just working around. [MediaProbe] spawns FFprobe, and when FFmpegKit's native library - * cannot load its initialiser throws a bare `java.lang.Error` — which - * `probeWithFFprobe`'s own `catch (e: Exception)` does not catch, and which - * `ConversionViewModel.onInputPicked` does not catch either. Picking a file would then - * fail with an uncaught error rather than the "could not read this file" the code was - * written to give. + * cannot load, the failure arrives as a `java.lang.Error` rather than an `Exception` — + * which is why `probeWithFFprobe`'s `catch (e: Exception)` did not see it, and why + * `ConversionViewModel.onInputPicked` used to abandon its `viewModelScope.launch` + * instead of reporting a file it could not read. * - * **That is a latent production hazard, found here and deliberately not fixed here.** - * It cannot fire on a device that ships the `.so` files, which is every real install, - * so making `MediaProbe` catch `Throwable` would be a behaviour change to the pick path - * on the strength of a condition no user meets — its own commit, with its own test. - * What this seam does is narrower: it keeps the JVM out of that path, which is what - * makes the ViewModel reachable from a unit test at all. + * **Both of those are guarded now**, by + * [org.libremediaconverter.ffmpeg.isNativeLoadFailure] — which also documents what the + * boundary actually throws, since all three of the obvious guesses turn out to be + * wrong. This seam is no longer what stands between a JVM test and an uncaught error. + * + * It still earns its place: injecting a probe is how a test reaches a *chosen* outcome + * for a file rather than the unreadable verdict the JVM has no libraries to improve on, + * and how the error path itself is forced — see `ConversionViewModelProbeFailureTest`, + * which drives an `OutOfMemoryError` through here to pin that the guard stays narrow. * * Instrumented tests and the app itself get the real probe, exactly as before. */ diff --git a/app/src/main/java/org/libremediaconverter/ffmpeg/NativeLoadFailure.kt b/app/src/main/java/org/libremediaconverter/ffmpeg/NativeLoadFailure.kt new file mode 100644 index 0000000..0134867 --- /dev/null +++ b/app/src/main/java/org/libremediaconverter/ffmpeg/NativeLoadFailure.kt @@ -0,0 +1,59 @@ +package org.libremediaconverter.ffmpeg + +/** + * Whether [error] is FFmpegKit failing to load its native library, rather than this JVM + * being in trouble. + * + * ## Why a predicate rather than a catch clause + * + * `config/detekt/detekt.yml` turns `TooGenericExceptionCaught` off with a written argument: + * the engine boundaries sit in front of native code whose failure types are undocumented, + * "enumerating it would mean guessing, and a guess that is wrong crashes the app on a file + * it could have simply reported as unreadable." That argument is about *exceptions*, and it + * applies unchanged one level up — except that on the `Error` side the opposite mistake is + * available too. `catch (Throwable)` at a boundary that spawns a native process would + * swallow a genuine [OutOfMemoryError] and let the app carry on pretending it had merely + * met an unreadable file. + * + * So this names the failure instead of the catch clause. Everything it does not recognise is + * rethrown. + * + * ## What the boundary actually throws + * + * Read off the shipped AAR and confirmed by `MediaProbeNativeLoadTest`, because all three of + * the obvious guesses are wrong: + * + * - `NativeLoader.loadLibrary` catches the `UnsatisfiedLinkError` that `System.loadLibrary` + * raises and rethrows `java.lang.Error(message, cause)` — a **bare** `Error`, which is + * neither an `Exception` nor a [LinkageError]. `catch (e: UnsatisfiedLinkError)` sees + * nothing. Its `cause` is the original `UnsatisfiedLinkError`, which is what identifies it + * here; matching on the message would be matching on a format string. + * - That throw happens under `FFmpegKitConfig.`, so what a caller sees also depends + * on how the runtime treats an initializer that fails: observed as + * `ExceptionInInitializerError` on the JVM under Robolectric, and recorded as the bare + * `Error` in `docs/defect-audit.md`. Both shapes are handled rather than either being + * assumed. + * - Every touch **after** the first is a third type again — `NoClassDefFoundError: Could not + * initialize class …`, the JVM's own record that the class is poisoned. A guard written + * for the first shape alone would let the second pick onwards crash, which is the harder + * half to notice. + * + * All of the class-loading shapes are [LinkageError]s, and none of the errors that mean this + * JVM is failing — [OutOfMemoryError], `StackOverflowError`, the rest of + * `VirtualMachineError` — is one. That disjointness is what makes this narrow rather than a + * blanket `catch (Throwable)`. + * + * ## When it can fire + * + * Not on a healthy install: the `.so` files ship in the APK. A corrupted install or an ABI + * mismatch is the realistic device path, and the JVM unit tests are the other, where the + * libraries are absent by construction. + */ +internal fun isNativeLoadFailure(error: Error): Boolean = when { + // NoClassDefFoundError, ExceptionInInitializerError, UnsatisfiedLinkError: the JVM's + // whole vocabulary for "the code could not be loaded". + error is LinkageError -> true + // FFmpegKit's own bare java.lang.Error, identified by what it wraps. + error.cause is UnsatisfiedLinkError -> true + else -> false +} diff --git a/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelProbeFailureTest.kt b/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelProbeFailureTest.kt new file mode 100644 index 0000000..df59aae --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelProbeFailureTest.kt @@ -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 + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/MediaProbeNativeLoadTest.kt b/app/src/test/java/org/libremediaconverter/convert/MediaProbeNativeLoadTest.kt new file mode 100644 index 0000000..7ca591f --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/MediaProbeNativeLoadTest.kt @@ -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") + } +} diff --git a/app/src/test/java/org/libremediaconverter/ffmpeg/NativeLoadFailureTest.kt b/app/src/test/java/org/libremediaconverter/ffmpeg/NativeLoadFailureTest.kt new file mode 100644 index 0000000..6dced12 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/ffmpeg/NativeLoadFailureTest.kt @@ -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)) + } +} diff --git a/config/detekt/detekt.yml b/config/detekt/detekt.yml index c4d98f7..c08914f 100644 --- a/config/detekt/detekt.yml +++ b/config/detekt/detekt.yml @@ -102,3 +102,14 @@ exceptions: # and handles -- falls back to FFmpeg, or fails the job with a reason -- and the # SwallowedException rule stays active to keep it that way. active: false + TooGenericExceptionThrown: + # Still on for main source, which throws nothing generic and should not start. + # + # Relaxed for tests only, and for one reason: a test that reproduces a failed native + # load has to throw what the library actually throws, and FFmpegKit throws a *bare* + # `java.lang.Error` -- `NativeLoader` catches the UnsatisfiedLinkError from + # System.loadLibrary and rethrows `Error(message, cause)`. That is not incidental, it + # is the whole finding `ffmpeg/NativeLoadFailure.kt` exists to handle, and the reason + # the obvious narrower guards catch nothing. Substituting a tidier subclass here would + # leave the test passing against a defect it no longer reproduces. + excludes: ['**/test/**', '**/androidTest/**'] From 7db320018a44431a77e4c9a3924536f915497b57 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sat, 22 Aug 2026 19:45:46 -0500 Subject: [PATCH 2/2] Correct the JDK claim and decide the backup rules D11's documentation and scaffold items, less the one row that belongs to another change stream. README's "Requires JDK 17+ (AGP 9 will not run on older)" was wrong twice over, and `f496291` already corrected the same claim in CLAUDE.md. The floor is not AGP's, and 17 is not what compiles anything: Gradle 9.7.1's own `SupportedJavaVersions` carries MINIMUM_CLIENT_JAVA_VERSION = 8 and MINIMUM_DAEMON_JAVA_VERSION = 17, and this repo then overrides the daemon upward to 25 in gradle-daemon-jvm.properties. So the honest statement is that the launcher floor is 8, the daemon is 25 whatever JAVA_HOME says, and the app's bytecode is 25 -- which is what `./gradlew --version` shows on this machine right now, launcher 21 against daemon 25. The data_extraction_rules TODO is filled in rather than deleted, because `android:allowBackup="true"` makes it a live question and the answer is not "nothing to say". The app stores nothing of its own -- no settings, no history -- so WorkManager's queue is the entire backup payload, and restoring it is wrong rather than merely useless: every row names a content:// grant and a cacheDir path that do not survive reaching another device, and cacheDir is not backed up at all. Since `ec969c4` the ViewModel queries WorkManager by tag on launch, so those rows would not sit inert either -- a fresh install would come up reattached to a job the user never ran on it. WorkManager declares no exclusion of its own, so nothing upstream prevents it. allowBackup stays true. The decision belongs in the rules file, where it is per-file and legible to whoever adds real user data later, rather than in an app-wide switch that would also turn off device-to-device transfer. Each file is named instead of excluding the "database" domain in one line. Lint's FullBackupContent detector skips an that carries no path without checking it, so the one-line spelling could have silently protected nothing; the enumerated paths are ones the gate actually verifies, and they are present in the built APK's compiled resource. backup_rules.xml and android:fullBackupContent are deleted rather than filled in. That attribute is only read on Android 11 and lower and minSdk is 33, so it could never have applied here -- an equally empty template that, unlike the other one, had no live question behind it. Not touched: OutputPublisher's hasSpaceFor KDoc, which the audit lists under D11. That code belongs to a parked branch and another change stream. The stale com/example/androidmediaconverter package directory needs no commit: it is empty, and git has never tracked it because git cannot track an empty directory. Removed from the working copy directly. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 8 ++- app/src/main/AndroidManifest.xml | 1 - app/src/main/res/xml/backup_rules.xml | 13 ----- .../main/res/xml/data_extraction_rules.xml | 50 ++++++++++++++----- 4 files changed, 45 insertions(+), 27 deletions(-) delete mode 100644 app/src/main/res/xml/backup_rules.xml diff --git a/README.md b/README.md index fbd72fd..6f1ae20 100644 --- a/README.md +++ b/README.md @@ -108,7 +108,13 @@ restored after a restart. ## Building -Requires JDK 17+ (AGP 9 will not run on older) and the Android SDK with API 37. +Requires the Android SDK with API 37. **Do not pick a JDK** — the repo does. +`gradle/gradle-daemon-jvm.properties` pins the daemon to Java 25 and carries foojay +download URLs per platform, so Gradle finds an installed Java 25 or downloads one on the +first build, whatever `JAVA_HOME` points at. `JAVA_HOME` only chooses the *launcher*, which +Gradle 9.7.1 will run on Java 8 or newer. Everything the build actually compiles is Java 25, +the app's own bytecode included. `./gradlew --version` prints the launcher and the daemon +separately, and they routinely differ. FFmpeg is committed as a prebuilt archive under [`bin/`](bin/README.md), so a clone builds without a cross-compile. That is deliberate: rebuilding it per CI run made test diff --git a/app/src/main/AndroidManifest.xml b/app/src/main/AndroidManifest.xml index ff07d03..c77fe2f 100644 --- a/app/src/main/AndroidManifest.xml +++ b/app/src/main/AndroidManifest.xml @@ -29,7 +29,6 @@ android:name=".LibreMediaConverterApp" android:allowBackup="true" android:dataExtractionRules="@xml/data_extraction_rules" - android:fullBackupContent="@xml/backup_rules" android:icon="@mipmap/ic_launcher" android:label="@string/app_name" android:roundIcon="@mipmap/ic_launcher_round" diff --git a/app/src/main/res/xml/backup_rules.xml b/app/src/main/res/xml/backup_rules.xml deleted file mode 100644 index 4df9255..0000000 --- a/app/src/main/res/xml/backup_rules.xml +++ /dev/null @@ -1,13 +0,0 @@ - - - - \ No newline at end of file diff --git a/app/src/main/res/xml/data_extraction_rules.xml b/app/src/main/res/xml/data_extraction_rules.xml index 9ee9997..c890639 100644 --- a/app/src/main/res/xml/data_extraction_rules.xml +++ b/app/src/main/res/xml/data_extraction_rules.xml @@ -1,19 +1,45 @@ - + + + + - + + + + - --> - \ No newline at end of file +