From 64512021022960c64ac3238940e7332cd1c1da14 Mon Sep 17 00:00:00 2001 From: jpolo1224 Date: Sun, 23 Aug 2026 02:20:36 -0400 Subject: [PATCH] Savers: make every port_free and port_new path unconditional Follow-up to 7aa4547e60. Four ports -- flux, plasma, solarwinds and hyperspace -- still opened port_free with "if (!g_started) return;". That is unreachable today, since the JNI only calls destroy after a successful create, but unreachability is a property of the current call graph rather than of the function, and it contradicts the rule the fix established: gl1 is given back on every path out, because whatever it holds belongs to an EGL context that is about to die. All six are now unconditional. Same argument one level up. port_new opened with "if (g_started) return 1", which was defensible when gl1's state was whatever the last saver left, but nativeInit now calls gl1_lost() first -- so gl1 is guaranteed DOWN on entry and reporting success there would hand the caller a saver with no shim under it. A stale run is torn down instead, and gl1_init always runs. No behaviour change on any path reachable today. The point is that a saver added later, or an upstream cleanup that returns early, fails in its own saver instead of poisoning the next one. --- .../src/main/cpp/savers/flux/flux_port.cpp | 23 +++++++++++++++---- .../cpp/savers/hyperspace/hyperspace_port.cpp | 23 +++++++++++++++---- .../main/cpp/savers/lattice/lattice_port.cpp | 8 ++++++- .../main/cpp/savers/plasma/plasma_port.cpp | 23 +++++++++++++++---- .../cpp/savers/skyrocket/skyrocket_port.cpp | 8 ++++++- .../cpp/savers/solarwinds/solarwinds_port.cpp | 23 +++++++++++++++---- 6 files changed, 86 insertions(+), 22 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 8c299e2765..9a31caa922 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 @@ -23,10 +23,16 @@ namespace { bool g_started = false; } extern "C" { +void flux_port_free(); /* defined below; port_new tears down a stale run */ + /* preset is 1..6, matching the saver's own DEFAULTS1..DEFAULTS6. */ int flux_port_new(int preset) { - if (g_started) return 1; + /* NOT "return 1". g_started can only be set here if a previous run was never + * freed, and nativeInit has already called gl1_lost() -- so gl1 is DOWN, and + * reporting success would hand the caller a saver with no shim under it. Tear the + * stale run down and start clean. */ + if (g_started) flux_port_free(); if (!gl1_init()) return 0; if (preset < 1 || preset > 6) preset = 1; @@ -65,11 +71,18 @@ void flux_port_draw() void flux_port_free() { - if (!g_started) return; - saver_flux::cleanUp(); + if (g_started) { + saver_flux::cleanUp(); + saver_flux::readyToDraw = 0; + g_started = false; + } + /* Unconditional, and NOT guarded by g_started. 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. Leaving g.ready set with names from a dead context poisons the NEXT + * saver, because gl1_init() early-returns on g.ready. That the JNI currently only calls this + * after a successful create is a property of today's call graph, not of this function. + * gl1_shutdown() is idempotent. */ gl1_shutdown(); - saver_flux::readyToDraw = 0; - g_started = false; } } /* extern "C" */ diff --git a/platforms/android/app/src/main/cpp/savers/hyperspace/hyperspace_port.cpp b/platforms/android/app/src/main/cpp/savers/hyperspace/hyperspace_port.cpp index 3f8bf14b90..689607996c 100644 --- a/platforms/android/app/src/main/cpp/savers/hyperspace/hyperspace_port.cpp +++ b/platforms/android/app/src/main/cpp/savers/hyperspace/hyperspace_port.cpp @@ -24,10 +24,16 @@ namespace { bool g_started = false; } extern "C" { +void hyperspace_port_free(); /* defined below; port_new tears down a stale run */ + int hyperspace_port_new(int preset) { (void) preset; /* No presets upstream; every knob was a registry value. */ - if (g_started) return 1; + /* NOT "return 1". g_started can only be set here if a previous run was never + * freed, and nativeInit has already called gl1_lost() -- so gl1 is DOWN, and + * reporting success would hand the caller a saver with no shim under it. Tear the + * stale run down and start clean. */ + if (g_started) hyperspace_port_free(); if (!gl1_init()) return 0; saver_hyperspace::setDefaults(); @@ -59,11 +65,18 @@ void hyperspace_port_draw() void hyperspace_port_free() { - if (!g_started) return; - saver_hyperspace::cleanUp(); + if (g_started) { + saver_hyperspace::cleanUp(); + saver_hyperspace::readyToDraw = 0; + g_started = false; + } + /* Unconditional, and NOT guarded by g_started. 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. Leaving g.ready set with names from a dead context poisons the NEXT + * saver, because gl1_init() early-returns on g.ready. That the JNI currently only calls this + * after a successful create is a property of today's call graph, not of this function. + * gl1_shutdown() is idempotent. */ gl1_shutdown(); - saver_hyperspace::readyToDraw = 0; - g_started = false; } } /* extern "C" */ 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 bcd52d2ad5..a6a9cc9193 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 @@ -23,9 +23,15 @@ int g_preset = 1; extern "C" { +void lattice_port_free(); /* defined below; port_new tears down a stale run */ + int lattice_port_new(int preset) { - if (g_started) return 1; + /* NOT "return 1". g_started can only be set here if a previous run was never + * freed, and nativeInit has already called gl1_lost() -- so gl1 is DOWN, and + * reporting success would hand the caller a saver with no shim under it. Tear the + * stale run down and start clean. */ + if (g_started) lattice_port_free(); if (!gl1_init()) return 0; g_preset = (preset >= 1 && preset <= 6) ? preset : 1; 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 2d4f5a1e18..b1aebdd1e0 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 @@ -16,9 +16,15 @@ namespace { bool g_started = false; } extern "C" { +void plasma_port_free(); /* defined below; port_new tears down a stale run */ + int plasma_port_new(int preset) { - if (g_started) return 1; + /* NOT "return 1". g_started can only be set here if a previous run was never + * freed, and nativeInit has already called gl1_lost() -- so gl1 is DOWN, and + * reporting success would hand the caller a saver with no shim under it. Tear the + * stale run down and start clean. */ + if (g_started) plasma_port_free(); if (!gl1_init()) return 0; saver_plasma::setDefaults(); @@ -66,11 +72,18 @@ void plasma_port_draw() void plasma_port_free() { - if (!g_started) return; - saver_plasma::cleanUp(); + if (g_started) { + saver_plasma::cleanUp(); + saver_plasma::readyToDraw = 0; + g_started = false; + } + /* Unconditional, and NOT guarded by g_started. 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. Leaving g.ready set with names from a dead context poisons the NEXT + * saver, because gl1_init() early-returns on g.ready. That the JNI currently only calls this + * after a successful create is a property of today's call graph, not of this function. + * gl1_shutdown() is idempotent. */ gl1_shutdown(); - saver_plasma::readyToDraw = 0; - g_started = false; } } 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 9f21c55872..9b2bc97ad2 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 @@ -26,10 +26,16 @@ bool g_started = false; extern "C" { +void skyrocket_port_free(); /* defined below; port_new tears down a stale run */ + int skyrocket_port_new(int preset) { (void) preset; /* No presets upstream; every knob was a registry value. */ - if (g_started) return 1; + /* NOT "return 1". g_started can only be set here if a previous run was never + * freed, and nativeInit has already called gl1_lost() -- so gl1 is DOWN, and + * reporting success would hand the caller a saver with no shim under it. Tear the + * stale run down and start clean. */ + if (g_started) skyrocket_port_free(); if (!gl1_init()) return 0; saver_skyrocket::setDefaults(); 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 b9eb11cf47..1e379140f8 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 @@ -15,11 +15,17 @@ namespace { bool g_started = false; } extern "C" { +void solarwinds_port_free(); /* defined below; port_new tears down a stale run */ + /* preset is 1..6 from the UI. Upstream's DEFAULTS1..DEFAULTS6 is a zero-based ENUM here, not * the 1-based #defines Flux uses, so the UI value is shifted down. */ int solarwinds_port_new(int preset) { - if (g_started) return 1; + /* NOT "return 1". g_started can only be set here if a previous run was never + * freed, and nativeInit has already called gl1_lost() -- so gl1 is DOWN, and + * reporting success would hand the caller a saver with no shim under it. Tear the + * stale run down and start clean. */ + if (g_started) solarwinds_port_free(); if (!gl1_init()) return 0; if (preset < 1 || preset > 6) preset = 1; @@ -52,11 +58,18 @@ void solarwinds_port_draw() void solarwinds_port_free() { - if (!g_started) return; - saver_solarwinds::cleanUp(); + if (g_started) { + saver_solarwinds::cleanUp(); + saver_solarwinds::readyToDraw = 0; + g_started = false; + } + /* Unconditional, and NOT guarded by g_started. 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. Leaving g.ready set with names from a dead context poisons the NEXT + * saver, because gl1_init() early-returns on g.ready. That the JNI currently only calls this + * after a successful create is a property of today's call graph, not of this function. + * gl1_shutdown() is idempotent. */ gl1_shutdown(); - saver_solarwinds::readyToDraw = 0; - g_started = false; } }