From 1a99ab237e750472683f19a7cbe96cb51d739d39 Mon Sep 17 00:00:00 2001 From: jpolo1224 Date: Sun, 23 Aug 2026 01:43:59 -0400 Subject: [PATCH] Fix animated background locking users out of the app A saver that dies natively made ARMSX2 unlaunchable. The choice is a persisted pref read on the library screen -- the first screen -- so the crash repeated on every launch and Settings was never reachable to turn it off. The only escape was clearing app data, which takes memory cards and save states with it. A user lost their saves that way. Cause: gl1's state is a file-scope global holding GL object names, and gl1_init() early-returns on g.ready. Skyrocket and Lattice defer initSaver() to port_resize, so a create-then-teardown with no surface size left g_started false and their port_free returned BEFORE gl1_shutdown(); flux, plasma and solarwinds leaked it the same way when initSaver() left readyToDraw clear, since returning 0 means port_free is never called. Either way g.ready stayed set with names from a destroyed EGL context, and the next saver -- new view, new context -- drew against them. Drivers answer that with anything from a black screen to a segfault. Each port now gives gl1 back on every path out, and nativeInit calls gl1_lost() as the invariant: a new context never inherits old GL names. Contained separately, because native GL can always find a new way to die: the setting arms itself with a synchronous commit() before the render thread starts and disarms when that thread exits in an orderly way. Still armed at startup means the last run died with a saver up, so the background switches off and the user is told which one. runCatching was never going to catch a SIGSEGV. Also guards Thread.start(): it asks for a 16MB stack (Skyrocket declares a 3MB starmap as a local) and an OutOfMemoryError there is an uncaught throw on the main thread -- the same lockout with no native crash involved. --- .../src/main/cpp/savers/flux/flux_port.cpp | 9 ++- .../main/cpp/savers/lattice/lattice_port.cpp | 18 ++++-- .../main/cpp/savers/plasma/plasma_port.cpp | 9 ++- .../app/src/main/cpp/savers/savers_jni.cpp | 16 ++++++ .../cpp/savers/skyrocket/skyrocket_port.cpp | 18 ++++-- .../cpp/savers/solarwinds/solarwinds_port.cpp | 9 ++- .../java/com/armsx2/ui/home/HomeScreen.kt | 13 +++++ .../com/armsx2/ui/home/LibraryBackground.kt | 55 +++++++++++++++++++ .../java/com/armsx2/ui/home/SaverGlView.kt | 37 ++++++++++++- 9 files changed, 170 insertions(+), 14 deletions(-) diff --git a/platforms/android/app/src/main/cpp/savers/flux/flux_port.cpp b/platforms/android/app/src/main/cpp/savers/flux/flux_port.cpp index cf4767fa68..8c299e2765 100644 --- a/platforms/android/app/src/main/cpp/savers/flux/flux_port.cpp +++ b/platforms/android/app/src/main/cpp/savers/flux/flux_port.cpp @@ -40,7 +40,14 @@ int flux_port_new(int preset) saver_flux::initSaver(); g_started = saver_flux::readyToDraw != 0; - return g_started ? 1 : 0; + if (!g_started) { + /* Returning 0 means the JNI never calls port_free, so this is the only chance to give + * gl1 back. Leaving it up would strand g.ready with names from a context that is about + * to die, and gl1_init() early-returns on g.ready -- poisoning the NEXT saver. */ + gl1_shutdown(); + return 0; + } + return 1; } void flux_port_resize(int width, int height) diff --git a/platforms/android/app/src/main/cpp/savers/lattice/lattice_port.cpp b/platforms/android/app/src/main/cpp/savers/lattice/lattice_port.cpp index 44fe6f911b..bcd52d2ad5 100644 --- a/platforms/android/app/src/main/cpp/savers/lattice/lattice_port.cpp +++ b/platforms/android/app/src/main/cpp/savers/lattice/lattice_port.cpp @@ -53,11 +53,21 @@ void lattice_port_draw() void lattice_port_free() { - if (!g_started) return; - saver_lattice::cleanUp(); + /* NOT "if (!g_started) return": this saver waits for a surface size before it initialises, + * so it can be created and torn down having never started. See the note below. */ + if (g_started) { + saver_lattice::cleanUp(); + saver_lattice::readyToDraw = 0; + g_started = false; + } + /* gl1_init() ran in port_new, and everything it holds -- the shader program, the vertex + * buffers -- belongs to the EGL context that is about to be destroyed. Returning without + * gl1_shutdown() leaves gl1's g.ready set with GL names from a DEAD context, and gl1_init() + * early-returns on g.ready. The next saver, in a NEW context, would then run against those + * dead names: undefined behaviour that some drivers answer with a segfault rather than a GL + * error, which takes the whole app down. So gl1 is torn down whether or not this saver's own + * init ever got as far as running. gl1_shutdown() is idempotent. */ gl1_shutdown(); - saver_lattice::readyToDraw = 0; - g_started = false; } } /* extern "C" */ diff --git a/platforms/android/app/src/main/cpp/savers/plasma/plasma_port.cpp b/platforms/android/app/src/main/cpp/savers/plasma/plasma_port.cpp index 33645edacb..2d4f5a1e18 100644 --- a/platforms/android/app/src/main/cpp/savers/plasma/plasma_port.cpp +++ b/platforms/android/app/src/main/cpp/savers/plasma/plasma_port.cpp @@ -42,7 +42,14 @@ int plasma_port_new(int preset) saver_plasma::initSaver(); g_started = saver_plasma::readyToDraw != 0; - return g_started ? 1 : 0; + if (!g_started) { + /* Returning 0 means the JNI never calls port_free, so this is the only chance to give + * gl1 back. Leaving it up would strand g.ready with names from a context that is about + * to die, and gl1_init() early-returns on g.ready -- poisoning the NEXT saver. */ + gl1_shutdown(); + return 0; + } + return 1; } void plasma_port_resize(int width, int height) diff --git a/platforms/android/app/src/main/cpp/savers/savers_jni.cpp b/platforms/android/app/src/main/cpp/savers/savers_jni.cpp index 85dff4c2bb..8e1e13b07b 100644 --- a/platforms/android/app/src/main/cpp/savers/savers_jni.cpp +++ b/platforms/android/app/src/main/cpp/savers/savers_jni.cpp @@ -7,6 +7,7 @@ #include #include +#include "gl1.h" #include #include @@ -82,6 +83,21 @@ Java_com_armsx2_ui_home_SaverNative_nativeInit(JNIEnv *, jobject, jint effect, j /* A previous saver may still be up if the outgoing view has not torn down yet. */ if (g_active) g_active->destroy(); + /* Every nativeInit arrives on a freshly created EGL context -- one view, one render thread, + * one context, one create. So whatever GL names gl1 is still holding belong to a context + * that no longer exists, and gl1_init() early-returns on g.ready, which would hand those + * dead names to the saver we are about to start. gl1_lost() drops them without calling GL + * on them (gl1_shutdown() would try to delete them, in the wrong context). + * + * The ports above each give gl1 back on their own failure and teardown paths, so this + * should already be a no-op. It is here because it is the invariant that actually matters + * -- a saver added later that forgets, or an upstream cleanup that returns early, would + * otherwise poison the NEXT saver rather than fail visibly in its own. Drivers answer a + * draw against a dead program name with anything from a black screen to a segfault, and a + * segfault here is unrecoverable: the library is the first screen, so the app would crash + * on every launch. */ + gl1_lost(); + g_active = &k_savers[effect]; if (!g_active->create(preset)) { LOGE("%s failed to start (preset %d)", g_active->name, preset); diff --git a/platforms/android/app/src/main/cpp/savers/skyrocket/skyrocket_port.cpp b/platforms/android/app/src/main/cpp/savers/skyrocket/skyrocket_port.cpp index 596561b105..9f21c55872 100644 --- a/platforms/android/app/src/main/cpp/savers/skyrocket/skyrocket_port.cpp +++ b/platforms/android/app/src/main/cpp/savers/skyrocket/skyrocket_port.cpp @@ -69,11 +69,21 @@ void skyrocket_port_draw() void skyrocket_port_free() { - if (!g_started) return; - saver_skyrocket::cleanup(); + /* NOT "if (!g_started) return": this saver waits for a surface size before it initialises, + * so it can be created and torn down having never started. See the note below. */ + if (g_started) { + saver_skyrocket::cleanup(); + saver_skyrocket::readyToDraw = 0; + g_started = false; + } + /* gl1_init() ran in port_new, and everything it holds -- the shader program, the vertex + * buffers -- belongs to the EGL context that is about to be destroyed. Returning without + * gl1_shutdown() leaves gl1's g.ready set with GL names from a DEAD context, and gl1_init() + * early-returns on g.ready. The next saver, in a NEW context, would then run against those + * dead names: undefined behaviour that some drivers answer with a segfault rather than a GL + * error, which takes the whole app down. So gl1 is torn down whether or not this saver's own + * init ever got as far as running. gl1_shutdown() is idempotent. */ gl1_shutdown(); - saver_skyrocket::readyToDraw = 0; - g_started = false; } } /* extern "C" */ diff --git a/platforms/android/app/src/main/cpp/savers/solarwinds/solarwinds_port.cpp b/platforms/android/app/src/main/cpp/savers/solarwinds/solarwinds_port.cpp index e664bd4272..b9eb11cf47 100644 --- a/platforms/android/app/src/main/cpp/savers/solarwinds/solarwinds_port.cpp +++ b/platforms/android/app/src/main/cpp/savers/solarwinds/solarwinds_port.cpp @@ -27,7 +27,14 @@ int solarwinds_port_new(int preset) saver_solarwinds::initSaver(); g_started = saver_solarwinds::readyToDraw != 0; - return g_started ? 1 : 0; + if (!g_started) { + /* Returning 0 means the JNI never calls port_free, so this is the only chance to give + * gl1 back. Leaving it up would strand g.ready with names from a context that is about + * to die, and gl1_init() early-returns on g.ready -- poisoning the NEXT saver. */ + gl1_shutdown(); + return 0; + } + return 1; } void solarwinds_port_resize(int width, int height) diff --git a/platforms/android/app/src/main/java/com/armsx2/ui/home/HomeScreen.kt b/platforms/android/app/src/main/java/com/armsx2/ui/home/HomeScreen.kt index 3a8e1fa6d1..6087330426 100644 --- a/platforms/android/app/src/main/java/com/armsx2/ui/home/HomeScreen.kt +++ b/platforms/android/app/src/main/java/com/armsx2/ui/home/HomeScreen.kt @@ -146,6 +146,19 @@ fun HomeScreen( var showClearRecentsConfirm by remember { mutableStateOf(false) } // #9 custom library background — inert until the user picks an image. LaunchedEffect(Unit) { LibraryBackground.ensureLoaded(); CoverArtStyle.load() } + // The animated background switched itself off because the last run died with it on screen + // (LibraryBackground.armSaver). Say so -- silently reverting a setting the user chose reads + // as the setting being broken, and the name tells them which one to avoid. + LaunchedEffect(LibraryBackground.crashedSaver.value) { + LibraryBackground.crashedSaver.value?.let { kind -> + LibraryBackground.crashedSaver.value = null + Toast.makeText( + context, + "Animated background turned off: ${LibraryBackground.saverName(kind)} crashed last time.", + Toast.LENGTH_LONG, + ).show() + } + } val backgroundPicker = rememberLauncherForActivityResult(ActivityResultContracts.OpenDocument()) { picked -> picked?.let { LibraryBackground.set(context, it) } } diff --git a/platforms/android/app/src/main/java/com/armsx2/ui/home/LibraryBackground.kt b/platforms/android/app/src/main/java/com/armsx2/ui/home/LibraryBackground.kt index 44289afabf..d21025c688 100644 --- a/platforms/android/app/src/main/java/com/armsx2/ui/home/LibraryBackground.kt +++ b/platforms/android/app/src/main/java/com/armsx2/ui/home/LibraryBackground.kt @@ -21,6 +21,12 @@ object LibraryBackground { private const val PREF_FLURRY_PRESET = "library.background.flurry.preset" private const val PREF_SAVER_KIND = "library_saver_kind" private const val PREF_RSS_PRESET = "library_rss_preset" + + /** + * Which saver is CURRENTLY running, written synchronously before its GL thread starts and + * cleared when that thread exits in an orderly way. See [armSaver]. + */ + private const val PREF_ARMED = "library.saver.armed" val uri = mutableStateOf(null) /** @@ -61,6 +67,47 @@ object LibraryBackground { /** Preset for whichever Really Slick saver is selected, 1..6. 99 = pick one each time. */ val rssPreset = mutableStateOf(99) + /** + * Set at startup when the previous run died with a saver on screen. Holds the [saverKind] that + * was running so the library can say which one, and so the user knows their background was + * turned off deliberately rather than forgotten. Read once and cleared by the reader. + */ + val crashedSaver = mutableStateOf(null) + + /** Display name for a [saverKind], for the message above. Matches the settings list. */ + fun saverName(kind: Int): String = when (kind) { + 1 -> "Flux"; 2 -> "Plasma"; 3 -> "SolarWinds" + 4 -> "Hyperspace"; 5 -> "Lattice"; 6 -> "Skyrocket" + else -> "Flurry" + } + + /** + * Crash-loop breaker. + * + * The savers are native GL code, and native GL code can take the process down in ways no + * `runCatching` can see -- a SIGSEGV in a driver, or a hang that Android resolves by killing + * us. Because the choice is persisted and the library is the FIRST screen, a saver that dies + * on startup dies again on every launch: the app never gets far enough for anyone to reach + * Settings and switch it off. The only escape is clearing app data, which on Android takes + * the memory cards and save states with it. A user hit exactly that and lost their saves. + * + * So the setting arms itself before the GL thread starts and disarms when that thread exits + * normally. Finding it still armed at startup means last run ended while a saver was on + * screen -- the background is switched off and the user is told which one did it. The write + * must be commit() rather than apply(): apply() is asynchronous, and the whole point is that + * the process may be about to die. + */ + fun armSaver() { + runCatching { + MainActivityRuntime.prefs.edit().putInt(PREF_ARMED, saverKind.value).commit() + } + } + + /** Orderly teardown -- the saver ran without taking the process with it. Idempotent. */ + fun disarmSaver() { + runCatching { MainActivityRuntime.prefs.edit().remove(PREF_ARMED).apply() } + } + private var loaded = false fun ensureLoaded() { @@ -72,6 +119,14 @@ object LibraryBackground { flurryPreset.value = runCatching { MainActivityRuntime.prefs.getInt(PREF_FLURRY_PRESET, 99) }.getOrDefault(99) saverKind.value = runCatching { MainActivityRuntime.prefs.getInt(PREF_SAVER_KIND, 0) }.getOrDefault(0) rssPreset.value = runCatching { MainActivityRuntime.prefs.getInt(PREF_RSS_PRESET, 99) }.getOrDefault(99) + + // Still armed = the previous run died with a saver up. Break the loop (see armSaver). + val armed = runCatching { MainActivityRuntime.prefs.getInt(PREF_ARMED, -1) }.getOrDefault(-1) + if (armed >= 0) { + crashedSaver.value = armed + setFlurry(false) + disarmSaver() + } } fun setAnimated2D(on: Boolean) { diff --git a/platforms/android/app/src/main/java/com/armsx2/ui/home/SaverGlView.kt b/platforms/android/app/src/main/java/com/armsx2/ui/home/SaverGlView.kt index 25a2a7ea89..09aeaf889d 100644 --- a/platforms/android/app/src/main/java/com/armsx2/ui/home/SaverGlView.kt +++ b/platforms/android/app/src/main/java/com/armsx2/ui/home/SaverGlView.kt @@ -97,8 +97,22 @@ class SaverGlView(context: Context, private val spec: SaverSpec) : } override fun onSurfaceTextureAvailable(st: SurfaceTexture, w: Int, h: Int) { - thread = RenderThread(st, w, h, spec) { ok -> post { onGlStatus?.invoke(ok) } } - .also { it.start() } + // Arm the crash-loop breaker for as long as native GL code is running on our behalf; the + // thread disarms it when it exits in an orderly way. See LibraryBackground.armSaver. + LibraryBackground.armSaver() + // start() asks for a 16MB stack (see STACK_BYTES) and can throw OutOfMemoryError on a + // constrained device. That would be an uncaught throw on the MAIN thread -- the process + // dies, and since this is the first screen the app would be unlaunchable. Fall back to the + // 2D backdrop instead, the same way an EGL failure does. + thread = runCatching { + RenderThread(st, w, h, spec) { ok -> post { onGlStatus?.invoke(ok) } } + .also { it.start() } + }.getOrElse { + Log.w(TAG, "saver thread failed to start", it) + LibraryBackground.disarmSaver() + onGlStatus?.invoke(false) + null + } } override fun onSurfaceTextureSizeChanged(st: SurfaceTexture, w: Int, h: Int) { @@ -152,9 +166,26 @@ class SaverGlView(context: Context, private val spec: SaverSpec) : private var saver: Saver? = null fun resize(w: Int, h: Int) { width = w; height = h; sizeDirty = true } - fun finish() { running = false; runCatching { join(500) } } + fun finish() { + running = false + runCatching { join(500) } + // join() is bounded, so the thread's own finally may not have run yet. We asked it to + // stop and the process is still here, which is all the breaker needs to know. + LibraryBackground.disarmSaver() + } override fun run() { + // Reaching the end of this function at all -- however the saver did -- means the + // process survived it, which is the only thing the breaker is asking about. A native + // crash or a kill never gets here, and that is what leaves the flag set. + try { + render() + } finally { + LibraryBackground.disarmSaver() + } + } + + private fun render() { if (!initEgl()) { onStatus(false); teardown(); return } val s = spec.newSaver()