From acadb4c1f4f9095b8044c92f3ffbb4c864713ecf Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 21:03:19 -0500 Subject: [PATCH] Stop the playlist and the OTA checker when the Activity is destroyed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit onDestroy already shuts down the wall and group controllers, and its comment says exactly why: those Handlers are on the main looper, which outlives the Activity, so a surviving tick "would keep broadcasting sync frames against the released player forever". Three other things on that same looper were never stopped. PlaylistController kept advancing after the Activity was gone. Every tick wrote the resume index and emitted play_start/play_end through the still-live WebSocketService, so after any relaunch — the "launch" command, Relauncher after OTA or boot, a re-pair, or a config change outside the ones the manifest handles — two controllers were reporting playback for one screen. That inflates Total Plays and Hours in Reports for that panel, and races over the resume position #234 depends on. Widget items also re-entered showWidget on a WebView nobody owned any more. UpdateChecker was never stopped either, and its install receiver was never unregistered: installReceiverRegistered is per-instance, so each recreate added another checker polling /api/update/check and another receiver for INSTALL_COMPLETE. N of those turns one STATUS_PENDING_USER_ACTION into N confirm dialogs stacked over customer content, and concurrent checkers race in tryPackageInstaller — which starts by abandoning ALL of the app's installer sessions, so one can abandon another's staged session mid-flight and the update never completes. shutdown() now does both, and the receiver is held so it can actually be unregistered. The Activity's own posted callbacks (the 30s failure-check loop among them) are cleared too. 134 Android JVM tests green. The effect is a leak and a duplicate reporting stream rather than a wrong value on a screen, so it is verified by reading the lifecycle rather than by a unit test. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL --- .../com/remotedisplay/player/MainActivity.kt | 13 +++++++++++ .../player/service/UpdateChecker.kt | 22 +++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/android/app/src/main/java/com/remotedisplay/player/MainActivity.kt b/android/app/src/main/java/com/remotedisplay/player/MainActivity.kt index d9b9039..88f5b88 100644 --- a/android/app/src/main/java/com/remotedisplay/player/MainActivity.kt +++ b/android/app/src/main/java/com/remotedisplay/player/MainActivity.kt @@ -1328,6 +1328,19 @@ class MainActivity : AppCompatActivity() { override fun onDestroy() { remoteStreaming = false + // Everything below this line exists for the same reason the wall/group shutdown does, and + // was missing: these Handlers are on the MAIN LOOPER, which outlives the Activity. + // + // PlaylistController kept advancing after the Activity was destroyed. Each tick wrote the + // resume index and emitted play_start/play_end through the still-live WebSocketService, so + // after a relaunch (the "launch" command, Relauncher after OTA/boot, a re-pair, or a config + // change outside the ones we handle) TWO controllers were reporting playback for one screen + // — inflating Total Plays and Hours in Reports, and racing over the resume position that + // #234 relies on. Widget items also re-entered showWidget on a WebView nobody owned. + if (::playlistController.isInitialized) playlistController.stop() + if (::updateChecker.isInitialized) updateChecker.shutdown() + // The 30s failure-check loop and anything else this Activity posted. + handler.removeCallbacksAndMessages(null) // Kill the wall/group leader tick BEFORE releasing media. The Handler is on the main looper // (outlives this Activity), so a surviving tick would keep broadcasting sync frames against // the released player forever — the zombie-leader / split-brain / garbage-position leak. diff --git a/android/app/src/main/java/com/remotedisplay/player/service/UpdateChecker.kt b/android/app/src/main/java/com/remotedisplay/player/service/UpdateChecker.kt index 8933a06..d29af58 100644 --- a/android/app/src/main/java/com/remotedisplay/player/service/UpdateChecker.kt +++ b/android/app/src/main/java/com/remotedisplay/player/service/UpdateChecker.kt @@ -38,6 +38,8 @@ class UpdateChecker(private val context: Context) { private val CHECK_INTERVAL = 30 * 60 * 1000L private var installReceiverRegistered = false + // Held so shutdown() can unregister it; without a handle the receiver outlives the Activity. + private var installReceiver: BroadcastReceiver? = null // #139: report OTA status to the dashboard (device:log, tag "ota"). Wired by MainActivity // to WebSocketService.sendLog; null until then. Read lazily so binding order doesn't matter. @@ -92,6 +94,7 @@ class UpdateChecker(private val context: Context) { @Suppress("UnspecifiedRegisterReceiverFlag") context.registerReceiver(receiver, filter) } installReceiverRegistered = true + installReceiver = receiver } fun startPeriodicCheck() { @@ -113,6 +116,25 @@ class UpdateChecker(private val context: Context) { checkTimer = null } + /** + * Full teardown for an Activity that is going away. + * + * stopPeriodicCheck alone leaves the install receiver registered against a dead Context, and + * installReceiverRegistered is per-instance — so each Activity recreate produced another + * checker polling /api/update/check and another receiver for INSTALL_COMPLETE. N of those means + * one STATUS_PENDING_USER_ACTION fires N confirm dialogs over customer content, and concurrent + * checkers race in tryPackageInstaller, which begins by abandoning ALL of this app's installer + * sessions — so one can abandon another's staged session mid-flight and the update never lands. + */ + fun shutdown() { + stopPeriodicCheck() + if (installReceiverRegistered) { + installReceiver?.let { r -> try { context.unregisterReceiver(r) } catch (_: Throwable) { /* already gone */ } } + installReceiver = null + installReceiverRegistered = false + } + } + /** * [forced] = an operator pressed "force update" on this specific device, rather than the * 30-minute timer firing. A forced run differs in three ways, all because a human aimed it at