Fix an unbounded blocked-thread leak in runJavaScript; ObjC over-retains + jsaddleWebViewInvalidate for wkwebview

[hamishmack]

Three commits. The first is a leak fix in jsaddle core that affects every transport; the other two are jsaddle-wkwebview.

1. Queue batch results instead of a one-slot MVar (aef9f3d) — jsaddle

runJavaScript's recvMVar is a single-slot MVar, so a delivery could block forever. The transports fork a thread per incoming batch of results, and any delivery with no batch thread waiting for it — a context whose pump has gone, or simply more results than batches sent — left that thread parked in putMVar for the life of the process.

With a busy page that is a runaway: ~5000 blocked threads per second, half a million threads and 12 GB RSS in one measured session (72 GB in a longer one). A TChan makes the deliverer's side non-blocking, so those threads finish and get collected. takeResult still skips results for earlier batches exactly as before, so the semantics are unchanged — including the "unexpected batch" error for a later one.

This is the one change here that is not macOS-specific.

2. Balance three ObjC over-retains (19dbbf5) — jsaddle-wkwebview

These files are compiled without ARC, so every alloc needs its release:

• completeSync built an NSString per synchronous round trip and never released it. That is the hot path for anything driving the page from Haskell: +140 MB resident per UI reload cycle in a ghci session, and a steady climb in ordinary use.
• loadHTMLStringWithBaseURL leaked the NSString it wrapped in an NSURL; it now uses autoreleased +stringWithCString, as the html argument beside it already did.
• AppDelegate's boot webview and its WKWebViewConfiguration were never released after the window took ownership, so that first webview — and its WebContent renderer process — outlived the window forever.

On that last one: webView is used again (callWithWebView) after [webView release], which is intentional and safe — [_window setContentView:webView] on the line above retains it, so the release just balances the alloc and leaves the window as sole owner. Happy to reorder the release below the callbacks if you'd rather it not read that way.

3. jsaddleWebViewInvalidate (fad5f96) — jsaddle-wkwebview

A host that owns the WKWebView itself (rather than letting run own it) had no way to take a jsaddle context back down. Two things kept it alive: jsaddle's threads go on calling evaluateJavaScript with the stored webview pointer, so releasing the webview is a use-after-free; and the three stable pointers the ObjC handler holds pin their closures — for a reflex application, the entire live network — for the life of the process. leksah reloads its whole UI inside a ghci session, where that means ~30 MB of live heap per reload and a WebContent process that never exits.

jsaddleWebViewInvalidate webView:

• Gates every use of the raw pointer behind an aliveVar MVar, so invalidation synchronises with an in-flight send: it blocks until the send's evaluateJavaScript (which copies the pointer into a retaining main-queue block) has returned, and no later send can start. After it returns the host may release the webview.
• An invalidated send throws rather than silently dropping the batch. This one is worth spelling out: runJavaScriptWithSerializer holds the shared batch lock across sendBatch and the takeResult waiting for that batch's reply, so a dropped batch would leave its pump waiting forever while holding the lock every other window needs — one invalidated context would deadlock the whole process. Throwing unwinds out of withMVar and kills only the dead context's own pump.
• Gates the inbound handlers too. The webview keeps its script-message handler until the host releases it, and a page whose JS is still running keeps posting results — each delivery forking a thread that would then block on the dead context's pump (see leak Build error with current version (4ce7398681ee) #1: this is where those 500k threads came from).
• Detaches the ObjC handler on the main queue — where all three callbacks are delivered from, so the detach cannot race one mid-flight — and frees the three stable pointers from a callback that runs after the detach. That is what finally makes an invalidated context collectable. addJSaddleHandler now returns the handler so it can be detached, and the handler methods check their stable pointers, covering deliveries WebKit had already queued behind the detach (a detached sync prompt gets completionHandler(@"") so the page is not left blocked).

The frees can't call hs_free_stable_ptr from the ObjC directly: under -objc-in-library (PR #170) that file is a plain dylib with no RTS to link against, so it goes back through the registered callback table.

Contract: call jsaddleWebViewInvalidate before releasing a webview you own. Nothing else changes for existing users — run/run' own their webview for the process lifetime and never invalidate.

One thing to decide before merging

onDetached writes a line to stderr when a context's stable pointers are freed. The detach is asynchronous, so that line is the only evidence of when (or whether) a context actually became collectable — invaluable while chasing this down, but it is unconditional output from a library. Happy to drop it, or gate it behind an env var, if you'd prefer that not ship.

Testing

• jsaddle and jsaddle-wkwebview build clean (no warnings) on macOS/aarch64, GHC 9.14.
• Merges cleanly into current master (checked with git merge-tree).
• All three fixes were found and measured in leksah, and are in daily use there: the TChan fix under a page that generates results continuously, and the invalidate path on every :reload of a ghci session — which is where the ~30 MB/reload heap retention and the never-exiting WebContent process were observed, and where they stopped.
• Not covered by CI: Haskell-CI here is Linux-only, so it exercises commit 1 but never compiles the wkwebview ObjC.

[Copilot]

🟡 Not ready to approve

The new invalidation path has a few concrete correctness/robustness issues (global lock held while blocking invalidators, potential forkOS startup thread leak, overly broad exception swallowing, and unconditional stderr output) that should be addressed before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR addresses long-lived resource leaks across jsaddle transports and adds explicit teardown support for jsaddle-wkwebview contexts, primarily to prevent runaway blocked threads, Objective-C memory leaks (non-ARC), and lingering stable pointers/webview processes in GHCi reload-heavy workflows.

Changes:

• Replace runJavaScript’s single-slot result MVar with a TChan queue so result delivery never blocks and leak-blocked deliverer threads.
• Fix several non-ARC Objective-C over-retains and add detachable handler plumbing so a host can tear down a WKWebView safely.
• Introduce jsaddleWebViewInvalidate to invalidate contexts tied to a host-owned WKWebView, gating outbound/inbound use and freeing stable pointers after handler detach.

File summaries

File	Description
jsaddle/src/Language/Javascript/JSaddle/Run.hs	Switch result delivery from a single-slot MVar to a TChan to prevent blocked-thread buildup on deliveries.
jsaddle-wkwebview/src/Language/Javascript/JSaddle/WKWebView/Internal.hs	Add context invalidation API, gate pointer usage, detach ObjC handler, and free stable pointers to allow context/webview teardown.
jsaddle-wkwebview/src-ghc/Language/Javascript/JSaddle/WKWebView.hs	Re-export jsaddleWebViewInvalidate for public use.
jsaddle-wkwebview/cbits/WKWebView-cbits.m	Return handler from addJSaddleHandler, add removeJSaddleHandler, guard callbacks after detach, and fix ObjC leaks.
jsaddle-wkwebview/cbits-cocoa/AppDelegate.m	Balance retains for the boot WKWebView and configuration so they don’t outlive the window.

Review details

Suppressed comments (2)

jsaddle-wkwebview/src/Language/Javascript/JSaddle/WKWebView/Internal.hs:216

• If the context is invalidated before didFinishNavigation fires, ready is never filled (the start handler is detached/cleared), so the forkOS thread blocks forever on takeMVar ready. Have the invalidator also tryPutMVar ready () to unblock startup so the thread can exit (it will then hit the withWebView gate and be caught).

    modifyMVar_ contextInvalidators $ return .
        ((ptr, modifyMVar_ aliveVar $ \_ -> do
                   mbDetach <- atomicModifyIORef' detachRef (\d -> (Nothing, d))
                   sequence_ mbDetach
                   return False) :)

jsaddle-wkwebview/src/Language/Javascript/JSaddle/WKWebView/Internal.hs:292

• Catching SomeException here will silently swallow unexpected failures (e.g. decode errors, async exceptions), making debugging much harder. Prefer catching only the expected IOException from withWebView (context invalidated) and let other exceptions still surface.

    void . forkOS . handle (\(_ :: SomeException) -> return ()) $ do
        takeMVar ready
        withWebView $ useAsCString (toStrict jsaddleJs) (evaluateJavaScript webView)
        void (forkIO start)

• Files reviewed: 5/5 changed files
• Comments generated: 4
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[hamishmack]

Fixed in d57f896 — and it was worse than described: jsaddleMain' registers on that same contextInvalidators MVar during startup, so an invalidation waiting on an in-flight send could hold up a new window booting, not just other invalidations. It now claims the matching entries under the lock and runs them outside it.

One consequence documented in the haddock: a second concurrent call for the same webview now finds nothing and returns without waiting for the first, so call it once per webview.

[hamishmack]

Both imported in d57f896: modifyMVar for the claim-then-run rework, and tryPutMVar to release the startup thread.

[hamishmack]

Agreed and removed in d57f896 — tracing when a context became collectable is the host application's business, not the library's.

[Copilot]

🟡 Human review recommended

The changes span concurrency semantics in core messaging plus manual Objective-C/FFI lifetime management, and warrant a final human review focused on edge-case teardown ordering and memory behavior under high callback volume.

Review details

Suppressed comments (1)

jsaddle/src/Language/Javascript/JSaddle/Run.hs:210

• Using an unbounded TChan means every inbound Callback/BatchResults message is retained until some future takeResult drains it. Because the JS runtime can emit many Callback messages that repeat lastResults for an already-consumed batch, bursts of callbacks can accumulate a large backlog (memory growth + takeResult doing O(backlog) reads to skip old batches). Consider dropping results for batches older than the current expected batch before enqueuing (e.g., track expected batch number in a TVar and ignore n < expected) or switching to a keyed store (e.g., TVar (IntMap BatchResults)) to dedupe by batch id.

        processResults :: Bool -> Results -> IO ()
        processResults syncCallbacks = \case
            (ProtocolError err) -> error $ "Protocol error : " <> T.unpack err
            (Callback n br (JSValueReceived fNumber) f this a) -> do
                atomically $ writeTChan recvChan (n, br)
                f' <- runReaderT (unJSM $ wrapJSVal f) ctx

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[hamishmack]

Re the suppressed note on the unbounded TChan (Run.hs) — recording the reasoning, since it is the one open question a human reviewer should weigh:

The backlog self-drains. takeResult loops discarding entries with n < nBatch, so the next batch send consumes every stale entry before it finds its own reply. The backlog is therefore bounded by "callbacks that arrive between two sends", and since a callback normally runs Haskell code that sends a batch in response, it is self-limiting in practice.

It is also strictly better than what it replaces: the single-slot MVar did not queue those deliveries, it blocked a thread per delivery, forever — ~24KB of stack each rather than a tuple, and the measured 500k threads / 12GB RSS this commit exists to fix.

The genuine residual case is a live context that keeps receiving callbacks and never sends another batch. (A dead context no longer applies: jsaddle-wkwebview now drops inbound deliveries at the ifAlive gate.) Dropping n < expected before enqueuing, or keying by batch id, would close that and make takeResult O(1) instead of O(backlog) — but it changes protocol-critical code in the sync/async interleaving, so I would rather it be its own change with tests behind it than a rider on a leak fix. Happy to do it as a follow-up if you want it.