Pre-push review: T-1541/bugfix-svgwebviewtests-offmain-crash

diff --git a/prismTests/SVGWebViewTests.swift b/prismTests/SVGWebViewTests.swiftindex c6800e7..9f0d567 100644--- a/prismTests/SVGWebViewTests.swift+++ b/prismTests/SVGWebViewTests.swift@@ -21,15 +21,31 @@ import WebKit /// - 3.1: SVGWebView implements NSViewRepresentable on macOS /// - 3.2: SVGWebView implements UIViewRepresentable on iOS ///-/// `@MainActor` here is load-bearing, not decoration. `createWebView()` builds a-/// `WKWebView` on `WKWebsiteDataStore.nonPersistentDataStore`, and WebKit traps-/// (SIGTRAP inside `WebKit::allDataStores()`) when a data store is constructed off-/// the main thread. Swift Testing runs suites in parallel and off the main actor-/// unless they are isolated, so without this the suite crashed its own test host —-/// which takes every test still queued in the whole run down with it and reports-/// them as failures at 0.000 seconds. That cascade is what made local runs-/// unreadable (T-1541), and this suite was its largest single source. Every other-/// live-WebKit suite in this target is already `@MainActor` for the same reason.+/// WebKit's one-time init (`InitializeWebKit2`) and its data-store bookkeeping+/// RELEASE_ASSERT the main thread, so constructing a `WKWebView` anywhere else+/// SIGTRAPs the shared test host — which takes every test still queued in the+/// whole run down with it as 0.000-second "failures" (T-1541). This suite was+/// that cascade's last surviving source.+///+/// `@MainActor` on the suite is NOT sufficient to prevent that for a+/// *synchronous* test. A sync function has no hop-on-entry: its isolation is+/// realized only if the caller hops first, and the caller here is+/// swift-testing's generated `nonisolated(nonsending)` thunk — compiled in this+/// target under Swift 5 language mode with Approachable Concurrency and no+/// default actor isolation — which inherits the runner's cooperative-pool+/// executor. Under full-suite parallel load the hop doesn't happen, Swift 5+/// mode has no runtime enforcement to catch it, and the sync body runs WebKit+/// init off-main. That is why the `@MainActor` added in PR #330 only moved the+/// trap (`allDataStores()` → `runInitializationCode()`), and why the suite+/// passes in isolation but crashed full runs.+///+/// The rule that follows, for every test in this target: WebKit may only be+/// touched from an `async` test (actor-isolated async functions hop on entry as+/// ABI, independent of the caller), and this suite additionally wraps the+/// construction in `MainActor.run`, which is runtime-enforced regardless of+/// language mode or caller. Every other live-WebKit suite already touches+/// WebKit exclusively through async code — that asymmetry is how this suite+/// was the only one still crashing. @MainActor struct SVGWebViewTests { @@ -69,49 +85,51 @@ struct SVGWebViewTests {      // MARK: - WKWebView Configuration -    @Test("createWebView returns a WKWebView")-    func createWebViewReturnsWKWebView() {-        let view = SVGWebView(svg: "<svg></svg>")-        let webView = view.createWebView()-        #expect(webView is WKWebView)+    /// The single entry point for constructing a live web view in this suite.+    /// Every WebKit-touching test goes through here so the main-thread+    /// requirement is runtime-enforced in exactly one place (see the suite doc+    /// comment): the test is async (hop-on-entry) and the construction plus the+    /// caller's assertions run inside `MainActor.run`, so the `WKWebView` never+    /// leaves the main-actor closure.+    private func withWebView(_ body: @MainActor (WKWebView) -> Void) async {+        await MainActor.run {+            body(SVGWebView(svg: "<svg></svg>").createWebView())+        }     }      @Test("createWebView uses non-persistent data store")-    func createWebViewUsesNonPersistentDataStore() {-        let view = SVGWebView(svg: "<svg></svg>")-        let webView = view.createWebView()--        // Non-persistent data store should be used for security-        // We can verify by checking the configuration-        let config = webView.configuration-        #expect(config.websiteDataStore.isPersistent == false)+    func createWebViewUsesNonPersistentDataStore() async {+        await withWebView { webView in+            // Non-persistent data store should be used for security+            // We can verify by checking the configuration+            let config = webView.configuration+            #expect(config.websiteDataStore.isPersistent == false)+        }     }      // MARK: - Platform-Specific Configuration      #if os(iOS)     @Test("iOS: createWebView sets transparent background")-    func iOSCreateWebViewSetsTransparentBackground() {-        let view = SVGWebView(svg: "<svg></svg>")-        let webView = view.createWebView()--        #expect(webView.isOpaque == false)-        #expect(webView.backgroundColor == .clear)-        #expect(webView.scrollView.backgroundColor == .clear)-        #expect(webView.scrollView.isScrollEnabled == false)+    func iOSCreateWebViewSetsTransparentBackground() async {+        await withWebView { webView in+            #expect(webView.isOpaque == false)+            #expect(webView.backgroundColor == .clear)+            #expect(webView.scrollView.backgroundColor == .clear)+            #expect(webView.scrollView.isScrollEnabled == false)+        }     }     #endif      #if os(macOS)     @Test("macOS: createWebView sets transparent background via KVC")-    func macOSCreateWebViewSetsTransparentBackground() {-        let view = SVGWebView(svg: "<svg></svg>")-        let webView = view.createWebView()--        // On macOS, WKWebView background transparency is set via KVC-        // We verify the webView was created - the actual KVC setting cannot be easily verified-        // without using the same KVC approach-        #expect(webView.frame == .zero, "WebView should be created with zero frame initially")+    func macOSCreateWebViewSetsTransparentBackground() async {+        await withWebView { webView in+            // On macOS, WKWebView background transparency is set via KVC+            // We verify the webView was created - the actual KVC setting cannot be easily verified+            // without using the same KVC approach+            #expect(webView.frame == .zero, "WebView should be created with zero frame initially")+        }     }     #endif @@ -130,20 +148,6 @@ struct SVGWebViewTests {         #expect(coordinator.lastSVG == "")     } -    // MARK: - HTML Generation--    @Test("HTML generation includes SVG content")-    func htmlGenerationIncludesSVGContent() {-        let svg = "<svg><circle cx=\"50\" cy=\"50\" r=\"40\"/></svg>"-        let view = SVGWebView(svg: svg)-        let webView = view.createWebView()-        let coordinator = view.makeCoordinator()--        // Create a mock context to test updateWebView-        // We can verify the SVG is set in coordinator-        #expect(coordinator.lastSVG.isEmpty, "Coordinator should start with empty SVG")-    }-     // MARK: - Transform-Application Decision (decision 11, Req 7.1)      @Test("transform is not applied before the page has loaded")

diff --git a/specs/bugfixes/svgwebviewtests-offmain-crash/report.md b/specs/bugfixes/svgwebviewtests-offmain-crash/report.mdnew file mode 100644index 0000000..a361f61--- /dev/null+++ b/specs/bugfixes/svgwebviewtests-offmain-crash/report.md@@ -0,0 +1,200 @@+# Bugfix Report: SVGWebViewTests Off-Main WebKit Init Crash++**Date:** 2026-07-28+**Status:** Fixed++## Description of the Issue++Transit ticket: T-1541 (remaining scope after PRs #330/#331).++In full `make test-quick` runs (~3,900 tests), the test host process dies with a+SIGTRAP inside WebKit initialization — `WebKit::runInitializationCode()` via+`std::__call_once` / `WebKit::InitializeWebKit2()` / `WKWebViewConfiguration.init()`+at `SVGWebView.swift:90`, invoked from `SVGWebViewTests`. Because the whole unit-test+target shares one host process, every test still queued behind the crash is reported+as a failure with no recorded duration (~1,900 cascade entries per run).++The suite passes 20/20 in isolation and in a 10-suite WebKit-heavy subset. Only the+full run reproduces the crash. The faulting thread in every crash report is+`com.apple.root.user-initiated-qos.cooperative` — never the main thread — despite the+suite being annotated `@MainActor` since PR #330.++**Reproduction steps:**+1. On main (17b3b1b), run `make test-quick` (no `-skip-testing` flag)+2. Wait for the run to reach `SVGWebViewTests` under full parallel load+3. Observe SIGTRAP in WebKit init and a cascade of never-ran "failures"; `Tools/check-test-results.sh` separates the genuine failures from the cascade++**Impact:** Suite-level verdicts from full local runs are unusable unless+`prismTests/SVGWebViewTests` is skipped. Measured cost before the T-1541 fixes: ~10–20+minutes of baseline-comparison work per change, on every change.++## Investigation Summary++- **Symptoms examined:** Crash reports in `~/Library/Logs/DiagnosticReports/prism-*.ips`+  (four survive locally; all four predate the #331 merge and belong to crashers #331+  already fixed — the SVGWebViewTests stacks are recorded on the Transit ticket from+  the 2026-07-27 15:22/16:02 reports). Ticket history across PRs #330/#331.+- **Code inspected:** `prismTests/SVGWebViewTests.swift`, `prism/Views/SVGWebView.swift`,+  `prism.xcodeproj/project.pbxproj` build settings, `Makefile` test targets, and the+  other live-WebKit suites (`WebViewPoolTests`, `SVGRendererTests`, `MermaidRendererTests`).+- **Hypotheses tested:**+  - *"`@MainActor` on the suite keeps tests on the main thread"* — refuted by the+    post-#330 crash reports: the faulting thread is still the cooperative pool.+  - *"All WebKit suites should crash equally"* — refuted by inspection: SVGWebViewTests+    is the **only** suite in the target that constructs `WKWebViewConfiguration` /+    `WKWebView` directly inside **synchronous** test bodies. Every other live-WebKit+    suite touches WebKit from `async` code.+  - *"The target isolation asymmetry is the trigger"* — consistent: the app target sets+    `SWIFT_DEFAULT_ACTOR_ISOLATION = MainActor`, `prismTests` does not, and all targets+    build in Swift 5 language mode with `SWIFT_APPROACHABLE_CONCURRENCY = YES`.++## Discovered Root Cause++**Defect type:** Race condition / unenforced actor isolation (test-only).++A synchronous `@MainActor` function has no hop-on-entry: its isolation is realized+only if the **caller** hops to the main actor before the call. The callers of these+test bodies are swift-testing's macro-generated async thunks, compiled in the test+target — Swift 5 language mode, no default actor isolation, Approachable Concurrency+enabled. Those thunks are `nonisolated(nonsending)` (the `Builtin.ImplicitActor`+frames in the crash stacks) and run on the *caller's* executor — swift-testing's+runner on the cooperative pool. Under full-run parallel load the expected hop to the+main actor does not happen before the synchronous body executes, and Swift 5 language+mode has no runtime enforcement to catch the mis-hop. The body then calls+`SVGWebView.createWebView()` → `WKWebViewConfiguration.init()` off-main, and WebKit's+one-time init `RELEASE_ASSERT`s that it is on the main thread, killing the host.++`async` members of a `@MainActor` type are immune: the switch to the actor's executor+on entry is emitted in the callee itself and is part of the ABI, independent of the+caller's module or language mode. That is why every other live-WebKit suite — which+touches WebKit only from `async` tests or `async` renderer APIs — survives the same+full runs.++**Why it occurred:** PR #330 fixed the crash class by annotation (`@MainActor` on the+suite), which is a compile-time promise the surrounding build configuration cannot+keep for synchronous functions. It moved the trap (from+`WebsiteDataStore::createNonPersistent` to `runInitializationCode`) without changing+the mechanism.++**Contributing factors:**+- `SWIFT_VERSION = 5.0` everywhere: isolation mis-hops are not runtime-enforced.+- App target has `SWIFT_DEFAULT_ACTOR_ISOLATION = MainActor`, test target does not.+- `SWIFT_APPROACHABLE_CONCURRENCY = YES` makes the generated nonisolated async thunks+  caller-inheriting (`nonisolated(nonsending)`), so which executor a sync test body+  runs on depends on runner scheduling — which is load-dependent, matching the+  "only in full runs" signature.++## Resolution for the Issue++**Changes made:**+- `prismTests/SVGWebViewTests.swift` — every test that constructs a live+  `WKWebView` (`createWebViewUsesNonPersistentDataStore`,+  `iOSCreateWebViewSetsTransparentBackground`, `macOSCreateWebViewSetsTransparentBackground`)+  is now `async` and routes construction through one suite-private helper,+  `withWebView`, whose body runs inside `await MainActor.run { }`. An+  actor-isolated `async` function hops to its executor on entry as part of the+  ABI (independent of the caller's module or language mode), and `MainActor.run`+  additionally enforces the main-actor executor at runtime — so WebKit+  construction in this suite can no longer execute off the main thread under any+  scheduling. The web view never leaves the main-actor closure. Assertions are+  unchanged.+- `prismTests/SVGWebViewTests.swift` — two tests were removed during the+  pre-push review of this fix: `createWebViewReturnsWKWebView` asserted+  `webView is WKWebView`, which is statically always true given the return type+  (zero assertion value at the cost of a live web view; construction success is+  proven by the data-store test), and `htmlGenerationIncludesSVGContent` —+  after its dead `createWebView()` call was removed — was an exact functional+  duplicate of `coordinatorInitializesWithEmptyLastSVG` under a name describing+  something it never tested.+- `prismTests/SVGWebViewTests.swift` — the suite doc comment now records the+  actual mechanism (sync `@MainActor` has no hop-on-entry; the PR #330+  annotation could only move the trap) and states the rule: WebKit may only be+  touched from `async` tests in this target.++**Approach rationale:** After two annotation-level attempts failed, the fix had+to be one whose guarantee is enforced at runtime rather than promised at compile+time. `MainActor.run` submits the closure to the main-actor executor no matter+which thread the test body landed on, so the crash becomes impossible by+construction. The change is confined to the one test file; production code is+untouched (production always constructs these views from SwiftUI representable+callbacks on the main thread).++**Alternatives considered:**+- *Set `SWIFT_DEFAULT_ACTOR_ISOLATION = MainActor` on `prismTests` (fix the+  target asymmetry)* — plausibly the "real" fix, but it changes the default+  isolation of ~3,900 tests at once: unknown compile fallout across the target+  and a scheduling shift for every suite. Too much blast radius for this bug;+  worth considering separately alongside a Swift 6 language-mode migration.+- *Delete the WebKit-touching tests / keep skipping the suite* — loses real+  coverage of the security-relevant configuration (non-persistent data store)+  for no benefit over the runtime-enforced hop.+- *`try #require(Thread.isMainThread)` guard before construction* — fails+  legibly instead of crashing, but leaves the tests unable to run their WebKit+  assertions whenever the mis-hop occurs (flaky failures instead of a fix).+  `MainActor.run` both fixes and protects.++## Regression Test++The defect is a scheduling race that only manifests under full-suite parallel load;+a deterministic small red test cannot exist (the suite passes 20/20 in isolation by+construction of the bug). Red/green is therefore run-level:++- **Red:** full `make test-quick` on main with SVGWebViewTests included crashes the+  host and cascades (baseline run recorded below).+- **Green:** the same full run after the fix completes with zero cascade entries.++Structural regression protection is added instead of a probabilistic test: WebKit+construction in the suite is routed through a helper that runtime-enforces the main+thread, so any future mis-hop becomes a clean, legible test failure instead of a+host crash.++**Run command:** `make test-quick` (full run, no skip flag)++## Affected Files++| File | Change |+|------|--------|+| `prismTests/SVGWebViewTests.swift` | WebKit-touching tests made `async` + `MainActor.run`; dead web-view construction removed from a coordinator-only test; suite doc comment rewritten with the real mechanism |++## Verification++**Automated:**+- [x] Reworked SVGWebViewTests pass in isolation (20/20 before the pre-push+      review's test consolidation; 18/18 after — re-verified)+- [x] Full `make test-quick` (SVGWebViewTests included) completes with zero cascade+      entries and zero failures: total=3935, passed=3896, failed=0, skipped=37+      (the two flaky long-timeout scroll tests from the baseline passed this run);+      no new crash report generated+- [x] `make lint` passes (0 violations in 505 files)++**Manual verification:**+- Baseline full run on pristine main (17b3b1b), SVGWebViewTests included: did NOT+  crash this time (total=3935, passed=3894, failed=2 — both long-timeout live-WebKit+  scroll tests unrelated to this crasher, `WebScrollNavigationTests.visibleBlockSuppressedDuringProgrammaticScroll`+  and `WebScrollPositionRetentionTests.initialLoadRestoreSurvivesAutoReport`).+  This confirms the crash is a probabilistic scheduling race, not a deterministic+  failure: a green full run is necessary but not sufficient evidence. The proof of+  the fix is structural — after it, WebKit construction in this suite can only+  execute on the main actor executor, so the crash becomes impossible by+  construction rather than unlikely by scheduling.++## Prevention++**Recommendations to avoid similar bugs:**+- In this test target's build configuration, never touch WebKit (or any API with a+  hard main-thread `RELEASE_ASSERT`) from a synchronous test body — `@MainActor` on+  the suite is not a runtime guarantee for sync functions. Use an `async` test and/or+  `await MainActor.run { }` around the construction.+- A test must never be able to trap (established on T-1541): route+  main-thread-required setup through helpers that fail legibly.+- Longer term: migrating the test target to Swift 6 language mode (or giving it+  `SWIFT_DEFAULT_ACTOR_ISOLATION = MainActor`) would make mis-hops compile-time or+  runtime errors; tracked as an alternative, not done here (blast radius spans the+  whole target).++## Related++- Transit T-1541 (this ticket), T-1983/T-1984 (siblings from the same cascade work)+- PR #330 (`81c1636`) — cascade scaffolding fix, added the suite `@MainActor`+- PR #331 (`17b3b1b`) — fixed the other named crashers; left SVGWebViewTests open+- `Tools/check-test-results.sh` — cascade/genuine failure discriminator

diff --git a/docs/agent-notes/development-tooling.md b/docs/agent-notes/development-tooling.mdindex 88e7ab5..37464bf 100644--- a/docs/agent-notes/development-tooling.md+++ b/docs/agent-notes/development-tooling.md@@ -7,7 +7,7 @@  ## Test-suite gotchas found while clearing the T-1541/T-1983 backlog -- **`SVGWebViewTests` still crashes the host in a FULL run** (T-1541 remains open). The `@MainActor` added in 81c1636 did not fix it — it only moved the trap, from `WebKit::allDataStores()` to `WebKit::runInitializationCode()`. The faulting thread is still `com.apple.root.user-initiated-qos.cooperative` (never thread 0), so the isolation is not taking effect under full-run conditions. It does NOT reproduce in isolation (20/20 pass, and a `Thread.isMainThread` probe passes there), nor with a WebKit-heavy subset — only in the whole suite. Until it is fixed, get a legible run with `-skip-testing:prismTests/SVGWebViewTests`, which takes the suite from ~1000 passing to the full ~3900.+- **`SVGWebViewTests` no longer needs skipping** (fixed on the T-1541 bugfix branch; `specs/bugfixes/svgwebviewtests-offmain-crash/report.md`). Root cause of the full-run host crash: the suite was the only one constructing `WKWebView` in *synchronous* test bodies. A sync `@MainActor` function has no hop-on-entry — its isolation depends on the caller hopping, and swift-testing's `nonisolated(nonsending)` thunks (Swift 5 mode + Approachable Concurrency, no default isolation in the test target) run on the runner's cooperative pool under full-suite load. WebKit's init `RELEASE_ASSERT(main thread)` then killed the host. The `@MainActor` added in 81c1636 could only move the trap. The rule for this target: **touch WebKit only from `async` tests** (actor-isolated async functions hop on entry as ABI); the suite additionally wraps construction in `MainActor.run`, which is runtime-enforced. - **The app and test targets have different actor-isolation defaults.** `SWIFT_DEFAULT_ACTOR_ISOLATION = MainActor` is set on the app target only; `prismTests` and `prismUITests` have no default. Production code is therefore implicitly main-actor while the same type called from a test is not, which silently turns "safe because everything is on the main actor" into a real race in tests (see `RecentFileEntry.relativeDateFormatter`). Do not assume a production type's isolation holds inside a test. - **`XCTExpectFailure` breaks `check-test-results.sh`'s arithmetic check.** An expected failure is counted in neither `passedTests` nor `failedTests`, so the script prints `WARN: passed+failed+skipped != total`. That warning is benign when the difference equals the number of `XCTExpectFailure`s (currently 2, T-1985 and T-1986); it does not gate the build. - **One `MockURLProtocol` used to be shared by five suites** across `URLDocumentLoaderTests`, `ImageLoaderTests`, and `SVGSourceLoaderTests`. `.serialized` only orders tests *within* a suite, so the suites overwrote each other's static handler and served each other's payloads. Handlers are now registered per scope (`MockURLScope`), carried as a request header set from the session configuration. If you add a networked suite, give it its own scope rather than a global handler.