CEF bridges: hold the runtime their Task uses, instead of re-reading an unowned one

Merged
TE
9d ago
+161-20

CEFTab.runtime is @ObservationIgnored unowned let runtime: CEFRuntime (CEFTab.swift:189). CEFMediaBridge.run and CEFVerificationBridge.run each spawned a Task { [weak tab] in ... } that re-read tab.runtime on every iteration of its batch. The weak capture guards the tab, never the runtime behind it: the in-flight runtime.devTools(...) call holds the runtime for its own duration, but nothing holds it between two calls of the batch, so once the runtime's last owner went, the next iteration read a destroyed object and trapped.

Fatal error: Attempted to read an unowned reference but object 0x845381000 was already destroyed
*** Program crashed: Signal 6: Aborted ***
Thread 0 crashed:
 4 swift::swift_abortRetainUnowned(void const*) + 48   in libswiftCore.dylib
 5 swift_unownedRetainStrong + 144                     in libswiftCore.dylib
 6 closure #1 in CEFMediaBridge.run(_:_:) + 636        in CmuxNextPackageTests
 7 partial apply for closure #1 in CEFMediaBridge.run(_:_:)
10 completeTaskWithClosure

An unowned trap aborts the process rather than failing one DevTools call: the helper dies on signal 6 with no Test run with summary line, and every test after it goes unreported (first seen in CI run 40357027098; unattributed since Oct 22 because the backtracer that symbolicates it only arrived with cmux#25391).

Five sites now read the runtime where it is known alive and hold it across the awaits that follow, instead of re-reading an unowned reference after a suspension:

sitethe gap
CEFMediaBridge.runbetween two calls of the batch
CEFVerificationBridge.runbetween two calls of the batch
CEFTab.evaluate (CEFTab+Events.swift)an isolated-world evaluate is four round trips
CEFCookieStore.deletebetween two cookies of the batch
CEFDesignModeBridge.prepareacross await tab.evaluate
CEFFileDragRelay.send and .leaveacross await previous?.value, before the call into the tab

That is the shape CEFFirstPaintBridge.attach already uses in this same directory (let runtime = tab.runtime before the Task), so this makes these consistent with it rather than inventing a pattern. The Tasks are short-lived and the runtime does not hold them, so there is no retain cycle.

Changelog

none

No capture: no visible change. Every changed line reads the same CEFRuntime one turn earlier so an unowned reference is not re-read after a suspension; the DevTools calls, their order and their arguments are unchanged, so there is nothing a still could show.

Red, then green

The regression test is committed before the fix, and the stack above is this repository's own test suite reproducing the CI crash frame for frame. Both runs are scripts/ci/run-swift-testing-suites.sh Packages/macOS/CmuxNext on the build fleet, same command, same suite filter:

commitcontents^CmuxNextBrowserTests\.CEFRendererPathsTests/
a351b100test onlyexit 1 — Fatal error: Attempted to read an unowned reference, signal 6, frame 6 closure #1 in CEFMediaBridge.run(_:_:)
e406aafbtest + the two bridgesexit 0 — ✔ Suite CEFRendererPathsTests passed after 0.045 seconds
cd2a2899three further sites, test hygienecompile green on the fleet (56s)
8174c4adthe drag relay, and the test's progress oraclecompile green on the fleet

The test (aBridgeSendsTheRestOfItsBatchAfterItsRuntimesLastOwnerIsGone) uses the Rig that CEFRendererPathsTests already has — no new scaffolding. It attaches a tab at fork API 24 (the DevTools path; from 29 CEFAgentRelay.browserAttached does not attach these bridges at all), lets each bridge Task park on the first unanswered call of its batch, drops the rig — the runtime's only owner — and then answers the parked calls so each Task runs on to the next call, which is the read that trapped. The answer takes its own reference inside a helper so the runtime is unowned again before the yield the Task resumes on; holding one at the top level would keep the runtime alive and prove nothing. It #requires that a call was really parked, so a future change that parks nothing fails the test instead of passing it vacuously.

It asserts progress, not just survival. An earlier round of this test drained until
Spy.unanswered was empty -- which answerParked itself causes -- so a batch that never resumed
would have passed. It now drains until a bridge has sent its next call and #expects that it
did, which is the read that trapped. Reading the static Spy is what the round before that removed
for making the verdict order-dependent, so this test attaches its own browser (77, where every
other test in the suite uses 7) and counts only calls recorded against it: a leaked unstructured
task from an earlier test in this serialized suite cannot satisfy the assertion. The shim records
each DevTools call's browser to make that countable.

Surviving to the end is the other half, and the half an expectation cannot express: before the fix
the read traps in swift_abortRetainUnowned, which aborts the process rather than failing a test.

This suite is the right home because its Rig does not retain the runtime, which is exactly why the abort lands here. CEFDeferredTabShowTests deliberately leaks its runtime (Unmanaged.passRetained, CEFDeferredTabShowTests.swift:206-214) with a comment describing this same hazard, so it could not have caught this.

Scope, and what this does not fix

CEFRuntime is only ever reached as static let shared (CEFRuntime.swift:14), which is never deallocated, so this trap is not reachable in the shipped app today — hence Changelog: none. It is reachable in tests, and it is one of the things truncating the swift test job.

It is not the only one, and this PR does not make the job report every test. A run on this PR's own head still ends early by a different route: a normal async-main shutdown, main → swift_task_asyncMainDrainQueue returning → exit → __cxa_finalize_ranges, with 2,722 tests started and never finished and swift test exited 0. No abort, no signal — which is why it reads as a pass. That is the symptom originally reported (tests such as HomeLaunchRevealTests never reporting), and it is a separate cause being tracked on its own. Two further pieces, neither in this PR:

  • Making a truncated run fail instead of reading as a pass: cmux#25391 fails the job when the log shows a test starting that never prints a result line, and when the summaries count fewer tests than the log shows starting. (cmux#25541 was an earlier attempt at the same gate, by me, opened because a context loss hid #25391 from me; I closed it in favour of #25391, whose parser handles the finish shapes #25541's missed.)
  • Root-causing the exit() from async main.

The same unowned class remains at roughly a dozen sites that read the runtime after an await or inside a callback, across BrowserMediaState+Scripts.swift, CEFFileDragRelay.swift, CEFSidePanelController.swift, CEFSpareWindow.swift, CEFPaneHost.swift, CEFPaneHost+Popups.swift, CEFTab+Events.swift's favicon Task and CEFTab+ResizeBackdrop.swift. They are left out deliberately: this PR is the one that clears the abort, and a sweep of the rest — which needs a per-site judgement about whether the tab can really outlive the runtime there — is a separate reviewable change rather than a bigger diff on a base-red fix.

Observed / Not run

Observed: the three fleet runs in the table above, the symbolicated signal-6 stack, cmux-ci compile-check --app green at this head, and python3 scripts/cmux-next/prepush-gate.py reporting no new violations.

Not run: the full CmuxNext suite at this head reporting every test — it does not, for the exit() reason above, which is why that claim is not made here. The five changed call sites have no behavioural difference to assert beyond not trapping: each reads the same object one turn earlier.

🤖 Generated with Claude Code

Agent session: agent session