From 018c6930770eb7fbd5ecc957bc3881a5b13068e3 Mon Sep 17 00:00:00 2001 From: Vitor Pamplona Date: Sun, 16 Aug 2026 22:35:33 -0400 Subject: [PATCH] fix: release the broker Messenger so a destroyed sandbox Activity can be freed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A full-screen napplet/browser surface ran onDestroy cleanly and was gone from ActivityManager, yet the :napplet process kept the Activity, its window and its WebView alive through repeated forced GCs. A heap dump gives the chain: ROOT(JNI_GLOBAL) android.os.Handler$MessengerImpl -> MessengerImpl.this$0 = android.os.Handler -> Handler.mCallback = -> lambda.f$0 = NappletBrowserActivity `replyMessenger = Messenger(Handler(mainLooper, ::onBrokerReply))` makes the Activity the handler's callback (a bound method reference captures `this`), and a Messenger sent over IPC is a binder — so while the broker holds it, ART keeps a JNI global reference to that Handler here in the sandbox. One retained Messenger therefore pinned Activity -> PhoneWindow -> DecorView -> WebView, and no GC in the sandbox could reclaim it; only killing the process could. The broker keeps replyTo in long-lived structures (incBus subscriptions, liveSubscriptions, identityWatch, foregroundLeases) and onDestroy only called unbindService, which releases none of them. Fix both halves: - MSG_RELEASE_CLIENT, sent first thing in onDestroy, so the broker drops the Messenger's inc-bus subscriptions and this surface's foreground lease. It goes directly on brokerMessenger rather than through sendToBroker, which queues while unbound — a queued release would never be sent. - The reply handler now holds the Activity through a WeakReference, so even a broker that never processes the release cannot pin a surface again. Verified on an emulator: opening one full-screen page and pressing back left Activities:1 WebViews:2 across three forced GCs before, and settles to Activities:0 WebViews:1 after. Heap dump: NappletBrowserActivity instances drop from 13 (the leaked Activity plus its captured lambdas) to 1 — the Companion, which is a static singleton and correctly retained. Note it takes two GC cycles to settle; one of the reference paths runs through a Cleaner chain, so a single forced GC still shows the old numbers. Co-Authored-By: Claude Opus 5 (1M context) --- .../amethyst/napplet/NappletBrokerService.kt | 15 ++++++ .../napplethost/NappletBrowserActivity.kt | 48 ++++++++++++++++++- .../napplethost/NappletHostActivity.kt | 41 +++++++++++++++- .../amethyst/napplethost/NappletIpc.kt | 14 ++++++ 4 files changed, 116 insertions(+), 2 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/napplet/NappletBrokerService.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/napplet/NappletBrokerService.kt index f49b497538..8d4aa5ae67 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/napplet/NappletBrokerService.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/napplet/NappletBrokerService.kt @@ -154,6 +154,21 @@ class NappletBrokerService : Service() { private var foregroundLeaseWatchdog: Job? = null private fun handleMessage(msg: Message): Boolean { + // A sandbox surface is being destroyed: drop every reference we hold to its Messenger. Holding a + // client's Messenger keeps a binder alive, which pins that surface's whole Activity (and its + // WebView) in the `:napplet` process past onDestroy — reclaimable only by killing the process. + if (msg.what == NappletIpc.MSG_RELEASE_CLIENT) { + msg.replyTo?.let { incBus.removeAll(it) } + // Release its foreground lease too; otherwise a destroyed surface keeps the main process + // pinned resumed until the lease watchdog expires it. + msg.data?.getString(NappletIpc.KEY_LAUNCH_TOKEN)?.let { token -> + synchronized(foregroundLeases) { + if (foregroundLeases.remove(token) != null) SandboxForegroundHold.release() + } + } + return true + } + // A sandbox surface (full-screen :napplet host) entered, renewed, or left the foreground. Hold the // main process resumed while at least one is foreground, so opening it doesn't tear down Tor/relays. if (msg.what == NappletIpc.MSG_SET_FOREGROUND) { diff --git a/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletBrowserActivity.kt b/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletBrowserActivity.kt index 26f91c3780..bd8cf3f48c 100644 --- a/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletBrowserActivity.kt +++ b/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletBrowserActivity.kt @@ -65,6 +65,7 @@ import com.vitorpamplona.amethyst.commons.browser.OmniboxInput import com.vitorpamplona.amethyst.commons.napplet.NappletWebContract import org.json.JSONObject import java.io.ByteArrayOutputStream +import java.lang.ref.WeakReference import java.util.concurrent.Executor import com.vitorpamplona.amethyst.commons.R as CommonsR @@ -107,7 +108,33 @@ class NappletBrowserActivity : ComponentActivity() { // ---- broker bridge (per-origin NIP-07 tokens; identical to NappletBrowserService) ---- private var brokerMessenger: Messenger? = null - private val replyMessenger = Messenger(Handler(Looper.getMainLooper(), ::onBrokerReply)) + + /** + * Reply channel handed to the broker. It MUST NOT hold this Activity strongly. + * + * A [Messenger] sent over IPC is a binder: while the main process holds it, ART keeps a **JNI global + * reference** to the backing [Handler] here in `:napplet`. A `Handler(looper, ::onBrokerReply)` makes + * the Activity the handler's `mCallback` (a bound method reference captures `this`), so that one + * retained binder pinned Activity → PhoneWindow → DecorView → WebView past `onDestroy`, and no GC in + * this process could ever reclaim it — only killing the process could. Measured: one full-screen page + * opened and closed left a destroyed Activity plus its WebView alive through repeated forced GCs. + * + * Holding the Activity weakly severs that chain at the source, so even a broker that never processes + * [NappletIpc.MSG_RELEASE_CLIENT] (see [onDestroy]) cannot leak a surface. Messages arriving after + * destruction are dropped, which is correct: there is nothing left to deliver them to. + */ + private val replyMessenger = Messenger(WeakBrokerReplyHandler(this)) + + private class WeakBrokerReplyHandler( + activity: NappletBrowserActivity, + ) : Handler(Looper.getMainLooper()) { + private val ref = WeakReference(activity) + + override fun handleMessage(msg: Message) { + ref.get()?.onBrokerReply(msg) + } + } + private val pendingBrokerRequests = mutableListOf() private var bridgeReplyProxy: JavaScriptReplyProxy? = null private var fireSeq = 0 @@ -259,6 +286,10 @@ class NappletBrowserActivity : ComponentActivity() { } override fun onDestroy() { + // Tell the broker to drop every reference to our reply Messenger BEFORE unbinding — a retained + // Messenger is a binder, and it would pin this Activity (and its WebView) in `:napplet` for the + // life of the process. `unbindService` alone does not release it. See [replyMessenger]. + releaseFromBroker() runCatching { unbindService(brokerConnection) } if (this::webView.isInitialized) { // Detach from the view tree BEFORE destroy(). Destroying a WebView while it is still attached to @@ -287,6 +318,21 @@ class NappletBrowserActivity : ComponentActivity() { if (brokerMessenger != null) sendToBroker(msg) } + /** + * Asks the broker to drop every reference it holds to [replyMessenger] (inc-bus subscriptions and this + * surface's foreground lease). Sent directly rather than through [sendToBroker] because that queues + * when the broker is unbound — and we are being destroyed, so a queued release would never be sent. + */ + private fun releaseFromBroker() { + val broker = brokerMessenger ?: return + val msg = + Message.obtain(null, NappletIpc.MSG_RELEASE_CLIENT).apply { + replyTo = replyMessenger + data = Bundle().apply { putString(NappletIpc.KEY_LAUNCH_TOKEN, startUrl) } + } + runCatching { broker.send(msg) } + } + @Suppress("SetJavaScriptEnabled") private fun configureWebView(wv: WebView) { wv.settings.apply { diff --git a/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletHostActivity.kt b/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletHostActivity.kt index 85f0803cf2..6239737305 100644 --- a/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletHostActivity.kt +++ b/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletHostActivity.kt @@ -78,6 +78,7 @@ import kotlinx.coroutines.delay import kotlinx.coroutines.launch import kotlinx.coroutines.withContext import org.json.JSONObject +import java.lang.ref.WeakReference import java.util.concurrent.Executor import com.vitorpamplona.amethyst.commons.R as CommonsR @@ -140,7 +141,26 @@ class NappletHostActivity : ComponentActivity() { // Messenger to the main-process broker, bound lazily; requests queue until connected. private var brokerMessenger: Messenger? = null - private val replyMessenger = Messenger(Handler(Looper.getMainLooper(), ::onBrokerReply)) + + /** + * Reply channel handed to the broker. It MUST NOT hold this Activity strongly — a [Messenger] sent + * over IPC is a binder, so while the main process holds it ART keeps a JNI global reference to the + * backing [Handler] here in `:napplet`. With a bound method reference as the handler callback, that + * one retained binder pins Activity → window → WebView past `onDestroy`, reclaimable only by killing + * the process. See [NappletBrowserActivity.replyMessenger] for the measured case. + */ + private val replyMessenger = Messenger(WeakBrokerReplyHandler(this)) + + private class WeakBrokerReplyHandler( + activity: NappletHostActivity, + ) : Handler(Looper.getMainLooper()) { + private val ref = WeakReference(activity) + + override fun handleMessage(msg: Message) { + ref.get()?.onBrokerReply(msg) + } + } + private val pendingRequests = mutableListOf() private var bridgeReplyProxy: JavaScriptReplyProxy? = null @@ -389,7 +409,23 @@ class NappletHostActivity : ComponentActivity() { foregroundHeartbeat = null } + /** + * Asks the broker to drop every reference it holds to [replyMessenger] (inc-bus subscriptions and this + * surface's foreground lease). Sent directly rather than through [sendToBroker], which queues while + * unbound — we are being destroyed, so a queued release would never leave. + */ + private fun releaseFromBroker() { + val broker = brokerMessenger ?: return + val msg = + Message.obtain(null, NappletIpc.MSG_RELEASE_CLIENT).apply { + replyTo = replyMessenger + data = Bundle().apply { putString(NappletIpc.KEY_LAUNCH_TOKEN, launchToken) } + } + runCatching { broker.send(msg) } + } + /** Reports this surface's foreground state to the broker so it can hold the main process resumed. */ + private fun setBrokerForeground(foreground: Boolean) { val msg = Message.obtain(null, NappletIpc.MSG_SET_FOREGROUND).apply { @@ -407,6 +443,9 @@ class NappletHostActivity : ComponentActivity() { override fun onDestroy() { uiScope.cancel() + // Drop the broker's references to our reply Messenger BEFORE unbinding — a retained Messenger is a + // binder and would pin this Activity (and its WebView) for the life of the `:napplet` process. + releaseFromBroker() // unbind is in runCatching: if the index never resolved we never bound the broker. runCatching { unbindService(brokerConnection) } keyActions.clear() diff --git a/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletIpc.kt b/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletIpc.kt index 9f72f7c471..8611a0d8cc 100644 --- a/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletIpc.kt +++ b/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletIpc.kt @@ -106,6 +106,20 @@ object NappletIpc { */ const val MSG_OPEN_PERMISSIONS = 12 + /** + * Host → broker: this surface is being destroyed — drop every reference the broker holds to its + * `replyTo` [android.os.Messenger] (inc-bus topic subscriptions, and its foreground lease when + * [KEY_LAUNCH_TOKEN] is supplied). + * + * A `Messenger` handed to the broker is a **binder**, so the main process holding it keeps a JNI + * global reference alive in `:napplet`. Because the sandbox's reply handler is a bound method + * reference on the Activity, that one retained Messenger pins the whole Activity → window → WebView, + * and no GC in the sandbox can ever reclaim it (only process death can). Sending this on destroy is + * the release half of the fix; the reply handler holding the Activity weakly is the other half, so a + * missed or dropped release can never pin a surface again. Fire-and-forget; no reply needed. + */ + const val MSG_RELEASE_CLIENT = 13 + const val KEY_REQUEST_ID = "requestId" const val KEY_PAYLOAD = "payload"