From 80199f42f454129d418a09522aa7a4a84978d638 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20T=C3=B6rcsv=C3=A1ri?= Date: Mon, 10 Aug 2026 09:58:28 +0200 Subject: [PATCH] Phase F: restore the doc-19 bounce (footprint chooser dead-app) + regression spec Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Td4ujboGuAw26jvbDQzehj --- .ci-cache-epoch | 2 +- .../features/async/22-absorbing-libcontext.md | 40 +++++ kicad | 2 +- tests/kicad/footprint-chooser-close.spec.ts | 140 ++++++++++++++++++ wasm/bindings/eeschema_embind.cpp | 1 + wasm/bindings/gerbview_embind.cpp | 1 + wasm/bindings/kicad_editor_embind.cpp | 1 + wasm/bindings/main_stack_runner.h | 86 +++++++++++ wasm/bindings/pcbnew_embind.cpp | 1 + wasm/bindings/pl_editor_embind.cpp | 1 + wxwidgets | 2 +- 11 files changed, 274 insertions(+), 3 deletions(-) create mode 100644 tests/kicad/footprint-chooser-close.spec.ts create mode 100644 wasm/bindings/main_stack_runner.h diff --git a/.ci-cache-epoch b/.ci-cache-epoch index 3c03207..d6b2404 100644 --- a/.ci-cache-epoch +++ b/.ci-cache-epoch @@ -1 +1 @@ -18 +19 diff --git a/docs/features/async/22-absorbing-libcontext.md b/docs/features/async/22-absorbing-libcontext.md index 6fdbc86..8d61d97 100644 --- a/docs/features/async/22-absorbing-libcontext.md +++ b/docs/features/async/22-absorbing-libcontext.md @@ -1323,6 +1323,46 @@ provider (the web-e2e-rot specs that would are `fixme`), so this raced-resolve shape had no automated stager — a synthetic "resolve races a context-wait park" lever is owed as its regression pin. +### F4 was WRONG — the doc-19 bounce is restored (2026-08-10) + +The gap-1 deferred-wake fix above did NOT fix the footprint chooser; the user +retested and it still froze. Reproduced live (the public `demo` board, real +fp-lib-table) and read the recorder — the mechanism is NOT gap 1: + +``` +inplace-park-on-fiber-stack ctx=21 n=175 +fcs … rf=dynCall_iiii +[wx-asyncify] fiber-resume-refused: fiber=2649686808 is asyncify-parked mid-body +``` + +The footprint chooser is a **quasi-modal with a NESTED event loop, opened from +the place-footprint TOOL COROUTINE**. Its nested wait runs on the tool +coroutine's fiber stack, where `can_yield_here()` is false (the running star +context is dispatch, not the fiber) — so `wxWasmYieldUntil` falls back to an +**in-place Asyncify park on the fiber**, the doc-19 disease. On Cancel the +resume into that parked-in-place fiber is refused by the quarantine and +dropped → the loop never resumes → dead app. + +**This is exactly what the doc-19 mainstack bounce prevented, and F4 removed +it.** F4's reasoning ("post-flip the context-park replaces the bounce") was +wrong for the tool-fiber case: the context-park only works once the frame is +already on the dispatch context, and the bounce is what moves a nested loop +opened from a tool fiber ONTO the dispatch context's stack (via `RunMainStack` +→ libcontext root, which post-flip IS the running dispatch context — so +`can_yield_here()` then holds and the wait context-parks correctly). F4's gate +missed it because the footprint chooser has no automated coverage. + +**F4 reverted** (kicad `452eb5260d`, wx `a8d9ece31f`, pcbjam `6a0a20f`): the +bounce, `s_mainStackRunner`/`wxWasmRunOnMainStack`, `mainstack.h`, +`main_stack_runner.h` + its 5 embind includes, and +`RunOnMainStackIfActiveTool` are all back. Verified: footprint chooser +Cancel/close green live (frames 2→1, loop keeps ticking) AND headless — the +new `tests/kicad/footprint-chooser-close.spec.ts` opens the chooser over a +board and cancels it, the regression pin F4 lacked. Gates: kicad 142/1, +battery unchanged. The gap-1 deferred-wake queue stays (it is correct and +inert here). **Lesson: never delete a doc-19 mitigation without a spec that +drives a tool-opened quasi-modal.** + ### Prod-provider smoke (2026-08-08) — done, with one pre-existing red bisected Against the live web stack (playwright-web config, reference backend :3060): diff --git a/kicad b/kicad index ea4a37d..99a7a36 160000 --- a/kicad +++ b/kicad @@ -1 +1 @@ -Subproject commit ea4a37d9f81cecc34b1e0a15a6aa619c693824eb +Subproject commit 99a7a368a6b36f2cd300d29bb136450e3c935cd7 diff --git a/tests/kicad/footprint-chooser-close.spec.ts b/tests/kicad/footprint-chooser-close.spec.ts new file mode 100644 index 0000000..20c3db1 --- /dev/null +++ b/tests/kicad/footprint-chooser-close.spec.ts @@ -0,0 +1,140 @@ +import { test, expect, type Page } from './fixtures'; +import { loadBoard } from './utils/threed-viewer'; +import { waitForPcbnew } from './utils/pcbnew-ready'; +import { clickMenuBarItem, clickMenuItemByText } from '../e2e/utils/element-tracker'; + +/** + * Repro for the user-reported dead-app: Add Footprint → the footprint chooser + * (a quasi-modal running a NESTED event loop, opened from the place-footprint + * TOOL COROUTINE) → click Cancel → the whole UI freezes. + * + * Root cause (recorder, live demo board): the nested loop parks IN PLACE on the + * tool coroutine's fiber stack (`inplace-park-on-fiber-stack`), then the resume + * that would close it is refused by the stale-fiber quarantine + * (`fiber-resume-refused: … asyncify-parked mid-body`) and dropped — the + * doc-19 disease. It was masked by the mainstack bounce until Phase F4 removed + * it; the footprint chooser has no automated coverage so the removal went + * unnoticed. This spec is that coverage. + * + * Mechanics that matter: the chooser is a wxFrame (not a wxDialog), so detect + * it by a second top-level frame + a "nested" scheduler wait; drive the canvas + * with synthetic emscripten mouse events (a Playwright click is intercepted by + * the wx scrollbar overlay); read the Cancel button's real coords from the + * element registry. + */ + +const TRAP = + /Aborted\(|index out of bounds|unreachable executed|indirect call signature|null function|memory access out of bounds/; + +function frameCount(page: Page): Promise { + return page.evaluate( + () => + (window.wxElementRegistry?.findAll({ visible: true }) ?? []).filter((e) => + /Frame$/.test(e.typeName || ''), + ).length, + ); +} + +async function synthClick(page: Page, x: number, y: number): Promise { + await page.evaluate( + ([cx, cy]) => { + const c = document.querySelector('#canvas') as HTMLCanvasElement; + const opt = (b: number) => ({ + clientX: cx, + clientY: cy, + bubbles: true, + cancelable: true, + view: window, + button: 0, + buttons: b, + }); + c.dispatchEvent(new MouseEvent('mousemove', opt(0))); + c.dispatchEvent(new MouseEvent('mousedown', opt(1))); + c.dispatchEvent(new MouseEvent('mouseup', opt(0))); + c.dispatchEvent(new MouseEvent('click', opt(0))); + }, + [x, y], + ); +} + +async function assertResponsive(page: Page, label: string): Promise { + const before = await page.evaluate( + () => Number((window.__wxAsyncifyDump?.().match(/fcsTotal=(\d+)/) || [])[1] || 0), + ); + await page.waitForTimeout(1500); // eslint-disable-line -- sampling the loop counter across a fixed window + const after = await page.evaluate( + () => Number((window.__wxAsyncifyDump?.().match(/fcsTotal=(\d+)/) || [])[1] || 0), + ); + if (after === before) { + const dump = await page.evaluate(() => + typeof window.__wxAsyncifyDump === 'function' ? window.__wxAsyncifyDump() : 'no dump', + ); + console.log(`[TEST] RECORDER after ${label} (loop STALLED, fcs=${after}):\n${dump}`); + } + expect(after, `main loop still advancing after ${label}`).toBeGreaterThan(before); +} + +test.describe('Add Footprint chooser close (doc-19 dead-app repro)', () => { + test('footprint chooser opens over a board and cancels without freezing', async ({ + page, + testLogger, + }) => { + test.setTimeout(180000); + await page.goto('/kicad/pcbnew.html'); + await waitForPcbnew(page); + await loadBoard(page, testLogger); + await assertResponsive(page, 'board load'); + + const framesBefore = await frameCount(page); + + // Arm Add Footprint, then click the canvas to open the chooser. + expect(await clickMenuBarItem(page, 'Place'), 'Place menu findable').toBe(true); + await clickMenuItemByText(page, 'Place Footprints'); + const canvas = await page.locator('#canvas').boundingBox(); + if (!canvas) throw new Error('canvas not found'); + await synthClick(page, Math.round(canvas.width * 0.35), Math.round(canvas.height * 0.45)); + + // The chooser is a second top-level frame + a "nested" scheduler wait. + await page + .waitForFunction( + (n) => + (window.wxElementRegistry?.findAll({ visible: true }) ?? []).filter((e) => + /Frame$/.test(e.typeName || ''), + ).length > n, + framesBefore, + { timeout: 40000 }, + ) + .catch(() => {}); + const framesOpen = await frameCount(page); + console.log(`[TEST] frames: ${framesBefore} → ${framesOpen}`); + expect(framesOpen, 'footprint chooser frame opened').toBeGreaterThan(framesBefore); + await assertResponsive(page, 'chooser open'); + + // Click the real Cancel button (coords from the element registry). + const cancel = await page.evaluate(() => { + const b = (window.wxElementRegistry?.findAll({ visible: true }) ?? []).find( + (e) => /Button/i.test(e.typeName || '') && /cancel/i.test(e.label || ''), + ); + return b ? { x: b.centerX, y: b.centerY } : null; + }); + expect(cancel, 'Cancel button found').not.toBeNull(); + await synthClick(page, cancel!.x, cancel!.y); + + // The chooser must close AND the app must stay alive. On the broken + // build the loop stalls here (the dropped fiber resume). + await page + .waitForFunction( + (n) => + (window.wxElementRegistry?.findAll({ visible: true }) ?? []).filter((e) => + /Frame$/.test(e.typeName || ''), + ).length <= n, + framesBefore, + { timeout: 20000 }, + ) + .catch(() => {}); + await assertResponsive(page, 'chooser cancel'); + + const traps = [...testLogger.consoleLogs, ...testLogger.errors].filter((l) => TRAP.test(l)); + expect(traps, 'no wasm trap after cancel').toEqual([]); + }); +}); diff --git a/wasm/bindings/eeschema_embind.cpp b/wasm/bindings/eeschema_embind.cpp index 67b616b..31175db 100644 --- a/wasm/bindings/eeschema_embind.cpp +++ b/wasm/bindings/eeschema_embind.cpp @@ -60,6 +60,7 @@ #include #include "collab_common.h" #include "open_gate.h" +#include "main_stack_runner.h" #include "collab_presence_core.h" #include "collab_presence_style.h" #include "pcbjam_theme.h" diff --git a/wasm/bindings/gerbview_embind.cpp b/wasm/bindings/gerbview_embind.cpp index 47b0d13..52b3701 100644 --- a/wasm/bindings/gerbview_embind.cpp +++ b/wasm/bindings/gerbview_embind.cpp @@ -25,6 +25,7 @@ #include #include #include "open_gate.h" +#include "main_stack_runner.h" using namespace emscripten; using json = nlohmann::json; diff --git a/wasm/bindings/kicad_editor_embind.cpp b/wasm/bindings/kicad_editor_embind.cpp index 598891e..75d66c9 100644 --- a/wasm/bindings/kicad_editor_embind.cpp +++ b/wasm/bindings/kicad_editor_embind.cpp @@ -42,6 +42,7 @@ #include "pcbjam_libs_reload.h" #include "open_gate.h" +#include "main_stack_runner.h" #include "timer_park.h" #include "fiber_park.h" diff --git a/wasm/bindings/main_stack_runner.h b/wasm/bindings/main_stack_runner.h new file mode 100644 index 0000000..1e4da5f --- /dev/null +++ b/wasm/bindings/main_stack_runner.h @@ -0,0 +1,86 @@ +/* + * Main-stack runner: the KiCad half of wx's nested-loop bounce + * (pcbjam docs/features/async/19, 20 D3). + * + * A quasi-modal's nested event loop parks its whole stack for the dialog's + * lifetime. When that stack is a TOOL coroutine's, the park suspends the + * fiber's body where the fiber layer cannot see it: the stale-fiber guard + * quarantines the fiber, then REFUSES its own resume, and the dialog stops + * responding to clicks (only the titlebar x still works, because that path is + * ungated). That is the Symbol Properties hang. + * + * wx detects "this nested loop is about to park on a non-main stack" — it can, + * cheaply and exactly, by comparing a frame address against + * emscripten_stack_get_base()/end() — but it must not know what a coroutine + * is. So it calls this runner, and KiCad's TOOL_MANAGER moves the loop onto + * the main stack via its own RunMainStack mechanism, which suspends the + * coroutine the legitimate way: a fiber swap the layer records + * (swap_suspended = true), so no quarantine, no refused resume. + * + * This lives in pcbjam's binding layer rather than in KiCad or wx precisely + * because it is the only place that may know about both. + */ +#pragma once + +#include + +#include +#include + +#include + +namespace pcbjam_main_stack +{ + +/** + * Run aFunc on the main stack if a tool coroutine is currently active. + * + * Returns 1 when the body has been run (bounced, or run inline because there + * was no coroutine to bounce off), 0 when there was nothing to run it with — + * no frame or no tool manager — in which case wx parks in place exactly as it + * did before this hook existed. + */ +inline int run_on_main_stack( void ( *aFunc )( void* ), void* aArg ) +{ + if( !wxTheApp ) + return 0; + + auto* frame = dynamic_cast( wxTheApp->GetTopWindow() ); + + if( !frame ) + return 0; + + TOOL_MANAGER* toolMgr = frame->GetToolManager(); + + if( !toolMgr ) + return 0; + + // RunOnMainStackIfActiveTool runs the body inline when no coroutine is + // running. Either way the body HAS run, so report it as handled — letting + // wx fall through would run the nested loop a second time. + bool ran = false; + + toolMgr->RunOnMainStackIfActiveTool( + [aFunc, aArg, &ran]() + { + ran = true; + aFunc( aArg ); + } ); + + return ran ? 1 : 0; +} + +/** + * Installs at static-init time. Only stores a function pointer, so it is safe + * before wx exists; every lookup above happens lazily per call, since frames + * come and go. Included by more than one binding TU — setting the same pointer + * twice is idempotent. + */ +struct INSTALLER +{ + INSTALLER() { wxWasmSetMainStackRunner( &run_on_main_stack ); } +}; + +inline INSTALLER g_installer; + +} // namespace pcbjam_main_stack diff --git a/wasm/bindings/pcbnew_embind.cpp b/wasm/bindings/pcbnew_embind.cpp index bc4788a..62d276f 100644 --- a/wasm/bindings/pcbnew_embind.cpp +++ b/wasm/bindings/pcbnew_embind.cpp @@ -53,6 +53,7 @@ #include "collab_common.h" #include "collab_presence_core.h" #include "open_gate.h" +#include "main_stack_runner.h" #include "timer_park.h" #include "fiber_park.h" #include "collab_presence_style.h" diff --git a/wasm/bindings/pl_editor_embind.cpp b/wasm/bindings/pl_editor_embind.cpp index ec6f8f6..211fb4b 100644 --- a/wasm/bindings/pl_editor_embind.cpp +++ b/wasm/bindings/pl_editor_embind.cpp @@ -18,6 +18,7 @@ #include #include #include "open_gate.h" +#include "main_stack_runner.h" #include #include #include diff --git a/wxwidgets b/wxwidgets index aa2cd03..4d479cb 160000 --- a/wxwidgets +++ b/wxwidgets @@ -1 +1 @@ -Subproject commit aa2cd035473f6367ac1de7e8b9e950ae09c9f7eb +Subproject commit 4d479cb3095f6b6cbc9550257c8f0c5b1deb102d