mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-10-05 11:18:24 +00:00
fix: release the broker Messenger so a destroyed sandbox Activity can be freed
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>
-> 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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
6420117f04
commit
018c693077
@@ -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) {
|
||||
|
||||
+47
-1
@@ -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<Message>()
|
||||
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 {
|
||||
|
||||
+40
-1
@@ -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<Message>()
|
||||
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()
|
||||
|
||||
@@ -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"
|
||||
|
||||
|
||||
Reference in New Issue
Block a user