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/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/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 + 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/**']