diff --git a/amethyst/plans/2026-06-21-napplet-code-audit.md b/amethyst/plans/2026-06-21-napplet-code-audit.md new file mode 100644 index 0000000000..1673a401ed --- /dev/null +++ b/amethyst/plans/2026-06-21-napplet-code-audit.md @@ -0,0 +1,63 @@ +# Napplet subsystem code audit β€” bugs, performance, refactor, placement + +**Date:** 2026-06-21. Scope: the napplet/nsite subsystem (`amethyst/.../napplet/`, +`commons/.../napplet/`, `quartz/.../nip5aStaticWebsites` + `nip5dNapplets`). + +## Fixed in this pass + +| # | Category | Issue | Fix | +|---|---|---|---| +| 1 | πŸ› correctness | **Live subscription unsubscribed from the wrong client after an account switch** β€” `closeLiveSubscription`/`onDestroy` used the *current* account's client, leaking the sub on the original. | `LiveSub` holder stores the exact `INostrClient` that opened the sub; teardown uses it. | +| 2 | πŸ› correctness | **Multi-relay subscriptions emitted N `relay.eose`** (one per relay) β€” the SDK expects one. | An `eoseSent` latch (`compareAndSet`) emits a single `relay.eose`. | +| 3 | ⚑ perf | **Broker rebuilt on every request** (new gateways/prompt each call). | `broker()` caches per account (reference identity), rebuilt only on switch. | +| 4 | ⚑ perf | **A fresh `OkHttpClient` per blob fetch** (no connection pooling). | `blobHttpClient()` caches the client keyed by Tor port (`@Synchronized`). | +| 5 | ♻️ refactor | **105-line `SHIM_JS` string constant** in `NappletHostActivity.kt` (no highlighting, hard to edit). | Moved to `assets/napplet/shim.js`, loaded once like `shell.html`. | +| 6 | πŸ“ docs | Stale shim comments (`onChanged` "follow-up", `subscribe` "snapshot…follow-up"). | Rewritten to match reality (no-op onChanged; live tail). | + +All compile; `commons:jvmTest` + the amethyst napplet suite stay green. + +## Deferred β€” with rationale (not silently dropped) + +- **Duplicate events across relays** β€” the same event id can arrive from multiple relays, so + `relay.event` is pushed more than once. Deduping needs a per-subscription seen-id set (unbounded + memory for long subs); napplets already dedupe by id. Left as-is; documented. +- **Background teardown of live subscriptions** β€” a backgrounded-but-alive napplet keeps its relay + subscription open (the WebView is paused, but the service keeps streaming). It is *not* a + permanent leak: the service is bind-only, so closing the napplet β†’ `unbindService` β†’ service + `onDestroy` β†’ all subs torn down. A proper pause/resume (unsubscribe on background, re-`REQ` on + foreground) is a real optimization but needs an IPC pause/resume signal + device verification. +- **Request ordering** β€” requests are handled concurrently (`scope.launch` per message), so + `storage.set` then `storage.get` aren't guaranteed in-order. Matches the SDK's async model; + serializing would hurt throughput. Documented, not changed. +- **`runBlocking` in `shouldInterceptRequest`** β€” this runs on a WebView *background* worker thread + (not the UI thread), so blocking there during a blob fetch is acceptable; WebView fans out + resource loads across workers. Left as-is. +- **Over-flags from the sweep that aren't real:** `pendingRequests` / `bridgeReplyProxy` "races" β€” + both the `WebMessageListener` callback and the reply `Handler` run on the **main looper**, so + there is no cross-thread access. A null `bridgeReplyProxy` only drops a reply to an + already-gone WebView (the applet is gone too) β€” harmless. + +## Recommended moves to commons / quartz + +- **quartz (protocol-only): correct as-is.** NIP-5A/5D events, `NappletManifest`, + `StaticSiteResolver` + `StaticSitePathLookup` (`sniffContentType`), `SiteAggregateHash` are all in + `commonMain`. No app policy leaked in. +- **commons (shared logic): mostly correct.** Broker, capability, identity, request/response, + permissions ledger/store, and gateway interfaces are in `commonMain` β€” right home. +- **Recommended move: `NappletProtocolJson` β†’ `commons/jvmAndroid`.** It's pure wire-marshalling + (kotlinx.serialization + `java.util.Base64`) over the commons protocol types; commons already has + a `jvmAndroid` source set and depends on `kotlinx.serialization.json`. Moving it (and its test) + co-locates the codec with the protocol and lets a future **desktop** napplet host reuse it. Modest + churn (the test is JUnit4 β†’ would move to commons `jvmTest`), no functional gain today, so it's a + *recommended*, not urgent, refactor. (Left in `amethyst/` for now.) +- **amethyst (Android-only): correctly platform-bound.** `NappletHostActivity` (WebView/process), + `NappletBrokerService` (Service/Messenger/account), the gateway *implementations* (account, + `BlossomUploader`, DataStore, NWC), `NappletLauncher`, consent UI, `NappletIpc`, and the screens + all belong here. + +## Lower-priority refactors (not done) + +- Extract a shared Tor-aware OkHttp builder (host `buildHttpClient` vs service `blobHttpClient` + duplicate the proxy logic) into a small util. +- `summaryFor`'s long `when` could become a per-request-type method, and `encodeResponse`'s big + `when` is repetitive β€” both are readability, not correctness. diff --git a/amethyst/src/main/assets/napplet/shim.js b/amethyst/src/main/assets/napplet/shim.js new file mode 100644 index 0000000000..d9e1dc56e1 --- /dev/null +++ b/amethyst/src/main/assets/napplet/shim.js @@ -0,0 +1,107 @@ +// Injected window.napplet.* client shim. Namespaced to match the @napplet/shim SDK over the +// {type:"domain.action", id} envelope, so an applet built against that SDK runs unchanged here. +// This is loaded as an asset and injected into the applet document by NappletHostActivity. +(function(){ + if (window.__nappletShimInstalled) return; window.__nappletShimInstalled = true; + var seq = 0, pending = {}, subs = {}, actions = {}; + function send(env){ env.id = env.id || ('r' + (seq++)); parent.postMessage(JSON.stringify(env), '*'); return env.id; } + function call(type, fields){ + return new Promise(function(resolve, reject){ + var env = { type: type }; if (fields) for (var k in fields) env[k] = fields[k]; + var id = send(env); + pending[id] = { resolve: resolve, reject: reject }; + }); + } + // Fire-and-forget (no .result awaited), used for subscribe/unsubscribe. + function post(type, fields){ var env = { type: type }; if (fields) for (var k in fields) env[k] = fields[k]; send(env); } + window.addEventListener('message', function(e){ + if (e.source !== parent) return; + var msg; if (typeof e.data === 'string') { try { msg = JSON.parse(e.data); } catch (_) { return; } } else { msg = e.data; } + if (!msg) return; + // Subscription pushes are keyed by subId, not a request id. + if (msg.type === 'relay.event' || msg.type === 'relay.eose' || msg.type === 'relay.closed') { + var sub = subs[msg.subId]; if (!sub) return; + if (msg.type === 'relay.event') { if (sub.onEvent) sub.onEvent(msg.event); } + else if (msg.type === 'relay.eose') { if (sub.onEose) sub.onEose(); } + else { delete subs[msg.subId]; if (sub.onClosed) sub.onClosed(msg.reason); } + return; + } + // keys.action push: the shell triggers a registered keyboard/command action. + if (msg.type === 'keys.action') { var cb = actions[msg.actionId]; if (cb) cb(); return; } + if (!msg.id) return; + var p = pending[msg.id]; if (!p) return; delete pending[msg.id]; + if (msg.ok) p.resolve(msg); + else { var err = new Error(msg.reason || msg.operation || msg.error || 'napplet error'); err.napplet = msg; p.reject(err); } + }); + function field(promise, name){ return promise.then(function(m){ return m[name]; }); } + function normFilters(filters){ return Array.isArray(filters) ? { filters: filters } : { filter: filters || {} }; } + function bytesToB64(bytes){ var u = bytes instanceof Uint8Array ? bytes : new Uint8Array(bytes); var s=''; for (var i=0;i client subId, so relay.close (and teardown) can stop them. - private val liveSubs = ConcurrentHashMap() + // The broker for the current account, rebuilt only on account switch (see broker()). + private var cachedBroker: Pair? = null + + // Reused blob HTTP client, keyed by the active Tor port (see blobHttpClient()). + private var cachedHttp: Pair? = null + + // Live relay subscriptions, keyed by the applet's subId. Holds the exact client that opened each + // one so teardown unsubscribes from the right account even after a switch, and an eose latch so + // a multi-relay subscription emits a single relay.eose. + private val liveSubs = ConcurrentHashMap() private val liveSeq = AtomicInteger(0) + private class LiveSub( + val clientSubId: String, + val client: INostrClient, + ) { + val eoseSent = AtomicBoolean(false) + } + override fun onBind(intent: Intent?): IBinder? { // Defense in depth on top of exported=false: only our own UID may bind. if (Binder.getCallingUid() != Process.myUid()) return null @@ -121,11 +138,7 @@ class NappletBrokerService : Service() { } override fun onDestroy() { - val client = - Amethyst.instance.sessionManager - .loggedInAccount() - ?.client - liveSubs.values.forEach { runCatching { client?.unsubscribe(it) } } + liveSubs.values.forEach { sub -> runCatching { sub.client.unsubscribe(sub.clientSubId) } } liveSubs.clear() scope.cancel() super.onDestroy() @@ -197,14 +210,23 @@ class NappletBrokerService : Service() { runCatching { NappletProtocolJson.decodeRequest(payload) }.getOrNull() ?: return NappletResponse.Failed("Malformed or unsupported request.") - val broker = buildBroker() ?: return NappletResponse.Failed("No account is signed in.") + val broker = broker() ?: return NappletResponse.Failed("No account is signed in.") return broker.handle(identity, request, declared) } - /** Builds a broker bound to the *currently* signed-in account, so account switches are honored. */ - private fun buildBroker(): NappletBroker? { + /** + * The broker for the *currently* signed-in account, cached and rebuilt only when the account + * changes (reference identity). The gateways capture the account and read its flows live, so a + * cached broker stays correct across requests without per-request allocation. + */ + @Synchronized + private fun broker(): NappletBroker? { val account = Amethyst.instance.sessionManager.loggedInAccount() ?: return null + cachedBroker?.let { (acc, broker) -> if (acc === account) return broker } + return buildBroker(account).also { cachedBroker = account to it } + } + private fun buildBroker(account: Account): NappletBroker { val relay = object : NappletRelayGateway { override suspend fun publish(event: Event): List { @@ -364,14 +386,22 @@ class NappletBrokerService : Service() { } } - /** Tor-routed OkHttp client for host-side blob fetches (the applet has no direct network). */ + /** + * Tor-routed OkHttp client for host-side blob fetches (the applet has no direct network). + * Cached and reused for connection pooling; rebuilt only when the Tor proxy port changes. + */ + @Synchronized private fun blobHttpClient(): OkHttpClient { val port = Amethyst.instance.torManager.activePortOrNull.value ?: -1 - return if (port > 0) { - OkHttpClient.Builder().proxy(Proxy(Proxy.Type.SOCKS, InetSocketAddress("127.0.0.1", port))).build() - } else { - OkHttpClient() - } + cachedHttp?.let { (cachedPort, client) -> if (cachedPort == port) return client } + val client = + if (port > 0) { + OkHttpClient.Builder().proxy(Proxy(Proxy.Type.SOCKS, InetSocketAddress("127.0.0.1", port))).build() + } else { + OkHttpClient() + } + cachedHttp = port to client + return client } /** @@ -482,8 +512,10 @@ class NappletBrokerService : Service() { } closeLiveSubscription(nappletSubId) - val clientSubId = "napplet-$nappletSubId-${liveSeq.incrementAndGet()}" - liveSubs[nappletSubId] = clientSubId + // liveSeq guarantees a unique client subId, so a rapid re-open of the same applet subId + // can't collide with the subscription it's replacing. + val sub = LiveSub("napplet-$nappletSubId-${liveSeq.incrementAndGet()}", account.client) + liveSubs[nappletSubId] = sub val listener = object : SubscriptionListener { @@ -494,10 +526,14 @@ class NappletBrokerService : Service() { forFilters: List?, ) = push(replyTo, NappletProtocolJson.encodeRelayEvent(nappletSubId, event)) + // A subscription fans out to several relays; collapse their EOSEs into the single + // relay.eose the SDK expects (fired when the first relay finishes its stored events). override fun onEose( relay: NormalizedRelayUrl, forFilters: List?, - ) = push(replyTo, NappletProtocolJson.encodeRelayEose(nappletSubId)) + ) { + if (sub.eoseSent.compareAndSet(false, true)) push(replyTo, NappletProtocolJson.encodeRelayEose(nappletSubId)) + } override fun onClosed( message: String, @@ -506,17 +542,14 @@ class NappletBrokerService : Service() { ) = push(replyTo, NappletProtocolJson.encodeRelayClosed(nappletSubId, message)) } - runCatching { account.client.subscribe(clientSubId, relays.associateWith { filters }, listener) } + runCatching { sub.client.subscribe(sub.clientSubId, relays.associateWith { filters }, listener) } } - /** Stops the live subscription for [nappletSubId], if any. */ + /** Stops the live subscription for [nappletSubId], unsubscribing from the client that opened it. */ private fun closeLiveSubscription(nappletSubId: String) { - val clientSubId = liveSubs.remove(nappletSubId) ?: return + val sub = liveSubs.remove(nappletSubId) ?: return runCatching { - Amethyst.instance.sessionManager - .loggedInAccount() - ?.client - ?.unsubscribe(clientSubId) + sub.client.unsubscribe(sub.clientSubId) } } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/napplet/NappletHostActivity.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/napplet/NappletHostActivity.kt index 4f116da5e9..2f220de985 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/napplet/NappletHostActivity.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/napplet/NappletHostActivity.kt @@ -330,10 +330,13 @@ class NappletHostActivity : ComponentActivity() { ) } + // The injected window.napplet shim, read once from assets (see assets/napplet/shim.js). + private val shimJs by lazy { assets.open("napplet/shim.js").use { it.readBytes() }.decodeToString() } + /** Inserts the `window.napplet` client shim into the applet's HTML document. */ private fun injectShim(html: ByteArray): ByteArray { val text = html.decodeToString() - val script = "" + val script = "" val headIdx = text.indexOf("