From daaff1f973f06ef086d84e8bc28edc758097f480 Mon Sep 17 00:00:00 2001 From: Istvan Matejcsok <119620946+matejcsok-ee@users.noreply.github.com> Date: Tue, 25 Aug 2026 11:24:26 +0200 Subject: [PATCH] =?UTF-8?q?fix(3d):=20blank=20viewer=20after=20raytracing?= =?UTF-8?q?=20round-trip=20=E2=80=94=20owner-context=20FFP=20routing=20+?= =?UTF-8?q?=20VAO=20isolation=20(gl1)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Switching OpenGL → raytracing → OpenGL could leave the viewer showing only the background gradient, with mid-session "[gl1] WebGL context changed" thrash and INVALID_OPERATION storms on BOTH WebGL contexts. Traced mechanism: the shim's FFP draw routing keyed on one process-global client-array flag; an interrupted fixed-function window (MODEL_3D::BeginDrawMulti loops, or the switch-back reload re-recording display lists with the GL context lock released across JSPI suspensions) left it set, after which the raytracer blit's and the 2D GAL's glDrawArrays were routed through the FFP pipeline — and one misrouted draw permanently repointed the VICTIM's own VAO attributes at shim buffers (the blit's attribute 0 collides with ATTR_POSITION), so both stayed broken/blank even after the flag cleared. Shim fixes (wasm/gl1): - Owner-context routing gate: __wrap_glDrawArrays/Elements route into the FFP pipeline only under the shim's owner context (adopted at the first FFP client-state mutation or programSync in a context); foreign-context draws always pass through — the 2D GAL can never be misrouted and the context guard can never thrash. - VAO isolation (ScopedDefaultVAO): draw executors do their attribute setup on VAO 0 and restore the caller's binding — a misrouted draw can no longer corrupt the caller. - contextSync() resets the whole client-array mirror on a context change (enables/pointers/VBO names all described the dead context). kicad pointer bump (d6e3dc1a87a): blit preamble disables the four client arrays (same-context firewall) + DoRePaint hidden-parent early return now clears m_is_currently_painting like its six siblings (a standalone sufficient cause of a permanently blank viewer). TDD (each observed red before its fix, green after; harness = authoritative): - T1 VAO corruption, T2 foreign-context routing + guard thrash, T3 stale client-state surviving context recreation — tests/e2e/3d-webgl.spec.ts over new harness choreography (appQuad/ffpMakeStale/createSecondContext/ quadDrawFresh). Parity stays 47/47 with zero drift. - tests/kicad/3d-viewer-engine-toggle.spec.ts (new, CI-skipped like the deadlock spec): real round-trip happy-path gate — board re-renders, zero [gl1] lines, zero INVALID_OPERATION. (The raytraced image itself never displays on the wasm build — the pre-existing inert-toggle KNOWN ISSUE in 3d-viewer-deadlock.spec.ts, out of scope here; the engine switch and the poisoning reload path run regardless.) Co-Authored-By: Claude Fable 5 --- kicad | 2 +- tests/3d-regression/wasm/3d_webgl_test.cpp | 166 ++++++++++++++++++++ tests/3d-regression/wasm/3d_webgl_test.html | 7 + tests/3d-regression/wasm/Makefile | 2 +- tests/apps/3d-webgl/3d_webgl_test.html | 7 + tests/e2e/3d-webgl.spec.ts | 130 +++++++++++++++ tests/kicad/3d-viewer-engine-toggle.spec.ts | 126 +++++++++++++++ wasm/gl1/README.md | 27 +++- wasm/gl1/include/gl1_shim.h | 6 + wasm/gl1/src/gl1_draw.cpp | 31 ++++ wasm/gl1/src/gl1_entry_ffp.cpp | 10 ++ wasm/gl1/src/gl1_entry_wrapped.cpp | 19 +++ wasm/gl1/src/gl1_state.cpp | 17 +- 13 files changed, 542 insertions(+), 8 deletions(-) create mode 100644 tests/kicad/3d-viewer-engine-toggle.spec.ts diff --git a/kicad b/kicad index c2e0545..d6e3dc1 160000 --- a/kicad +++ b/kicad @@ -1 +1 @@ -Subproject commit c2e0545e4745d38eddc98da122dcf7c147fbf610 +Subproject commit d6e3dc1a87a0565fc5cfab55d42420e72fc48f87 diff --git a/tests/3d-regression/wasm/3d_webgl_test.cpp b/tests/3d-regression/wasm/3d_webgl_test.cpp index 530f5b7..a4f8c2d 100644 --- a/tests/3d-regression/wasm/3d_webgl_test.cpp +++ b/tests/3d-regression/wasm/3d_webgl_test.cpp @@ -109,6 +109,172 @@ int runScenario( int aIndex ) return 0; } +// --------------------------------------------------------------------------- +// Engine-toggle regression scaffolding: model a "modern-GL consumer" (the +// raytracer blit / the 2D GAL) sharing the process with the FFP shim. +// --------------------------------------------------------------------------- + +// A minimal GLSL quad drawer with a PERSISTENT VAO (the victim). Mirrors the +// raytracer blit: attribute location 0 (colliding with the shim's +// ATTR_POSITION), own VBO, own program. +static GLuint g_appVAO = 0, g_appVBO = 0, g_appProg = 0; + +static GLuint compileMini( GLenum type, const char* src ) +{ + GLuint s = glCreateShader( type ); + glShaderSource( s, 1, &src, nullptr ); + glCompileShader( s ); + return s; +} + +extern "C" +{ + +// (Re)build the persistent app quad in the CURRENT context. Green full-screen +// quad on attribute 0. Returns 0 on success. +EMSCRIPTEN_KEEPALIVE +int appQuadInit() +{ + static const char* VS = "#version 300 es\nlayout(location=0) in vec2 p;" + "void main(){ gl_Position = vec4(p,0.,1.); }"; + static const char* FS = "#version 300 es\nprecision mediump float;" + "out vec4 c; void main(){ c = vec4(0.,1.,0.,1.); }"; + + GLuint vs = compileMini( GL_VERTEX_SHADER, VS ); + GLuint fs = compileMini( GL_FRAGMENT_SHADER, FS ); + GLuint prog = glCreateProgram(); + glAttachShader( prog, vs ); + glAttachShader( prog, fs ); + glLinkProgram( prog ); + glDeleteShader( vs ); + glDeleteShader( fs ); + + GLint linked = GL_FALSE; + glGetProgramiv( prog, GL_LINK_STATUS, &linked ); + if( !linked ) + return -1; + + static const float quad[] = { -1.f, -1.f, 1.f, -1.f, -1.f, 1.f, + -1.f, 1.f, 1.f, -1.f, 1.f, 1.f }; + g_appProg = prog; + glGenVertexArrays( 1, &g_appVAO ); + glBindVertexArray( g_appVAO ); + glGenBuffers( 1, &g_appVBO ); + glBindBuffer( GL_ARRAY_BUFFER, g_appVBO ); + glBufferData( GL_ARRAY_BUFFER, sizeof( quad ), quad, GL_STATIC_DRAW ); + glEnableVertexAttribArray( 0 ); + glVertexAttribPointer( 0, 2, GL_FLOAT, GL_FALSE, 0, nullptr ); + glBindVertexArray( 0 ); + glBindBuffer( GL_ARRAY_BUFFER, 0 ); + return (int) glGetError(); +} + +// Draw the persistent app quad exactly like the blit does: bind ITS VAO, its +// program, glDrawArrays. aClearFirst=1 clears to black before drawing so the +// framebuffer afterwards shows only this draw's output. Returns glGetError. +EMSCRIPTEN_KEEPALIVE +int appQuadDraw( int aClearFirst ) +{ + while( glGetError() != GL_NO_ERROR ) {} + if( aClearFirst ) + { + glViewport( 0, 0, CAPTURE_WIDTH, CAPTURE_HEIGHT ); + glClearColor( 0.f, 0.f, 0.f, 1.f ); + glClear( GL_COLOR_BUFFER_BIT ); + } + glUseProgram( g_appProg ); + glBindVertexArray( g_appVAO ); + glDrawArrays( GL_TRIANGLES, 0, 6 ); + glBindVertexArray( 0 ); + glUseProgram( 0 ); + glFinish(); + return (int) glGetError(); +} + +// Leave the FFP mirror in the poisoned shape the engine switch can produce: +// GL_VERTEX_ARRAY enabled with a (small) VBO captured by glVertexPointer — +// the MODEL_3D::BeginDrawMulti window state. +EMSCRIPTEN_KEEPALIVE +void ffpMakeStale() +{ + static GLuint tinyVBO = 0; + static const float tri[] = { 0.f, 0.f, 0.f, 1.f, 0.f, 0.f, 0.f, 1.f, 0.f }; + if( !tinyVBO ) + { + glGenBuffers( 1, &tinyVBO ); + glBindBuffer( GL_ARRAY_BUFFER, tinyVBO ); + glBufferData( GL_ARRAY_BUFFER, sizeof( tri ), tri, GL_STATIC_DRAW ); + } + else + glBindBuffer( GL_ARRAY_BUFFER, tinyVBO ); + glVertexPointer( 3, GL_FLOAT, 0, nullptr ); // captures tinyVBO in the mirror + glEnableClientState( GL_VERTEX_ARRAY ); + glBindBuffer( GL_ARRAY_BUFFER, 0 ); +} + +EMSCRIPTEN_KEEPALIVE +void ffpClearStale() +{ + glDisableClientState( GL_VERTEX_ARRAY ); +} + +// A second live WebGL context on its own canvas — the harness model of the 2D +// editor's GAL context coexisting with the 3D viewer's. +static EMSCRIPTEN_WEBGL_CONTEXT_HANDLE g_context2 = 0; + +EMSCRIPTEN_KEEPALIVE +int createSecondContext() +{ + EM_ASM( { + if( !document.getElementById( "canvas2" ) ) + { + var c = document.createElement( "canvas" ); + c.id = "canvas2"; + c.width = 320; c.height = 240; + document.body.appendChild( c ); + if( typeof specialHTMLTargets !== 'undefined' ) + specialHTMLTargets['#canvas2'] = c; + } + } ); + + EmscriptenWebGLContextAttributes attrs; + emscripten_webgl_init_context_attributes( &attrs ); + attrs.majorVersion = 2; + attrs.minorVersion = 0; + attrs.preserveDrawingBuffer = true; + g_context2 = emscripten_webgl_create_context( "#canvas2", &attrs ); + return g_context2 > 0 ? 0 : -1; +} + +// Switch the current context: 1 = the main harness context, 2 = the second. +EMSCRIPTEN_KEEPALIVE +int useContext( int aWhich ) +{ + EMSCRIPTEN_WEBGL_CONTEXT_HANDLE h = ( aWhich == 2 ) ? g_context2 : g_context; + if( h <= 0 ) + return -1; + return emscripten_webgl_make_context_current( h ) == EMSCRIPTEN_RESULT_SUCCESS ? 0 : -2; +} + +// Build + draw + delete a fresh GLSL quad entirely in the CURRENT context (no +// persistent state — safe under any context). Returns glGetError after draw. +EMSCRIPTEN_KEEPALIVE +int quadDrawFresh() +{ + GLuint savedVAO = g_appVAO, savedVBO = g_appVBO, savedProg = g_appProg; + g_appVAO = g_appVBO = g_appProg = 0; + int rc = appQuadInit(); + if( rc == 0 ) + rc = appQuadDraw( 1 ); + glDeleteVertexArrays( 1, &g_appVAO ); + glDeleteBuffers( 1, &g_appVBO ); + glDeleteProgram( g_appProg ); + g_appVAO = savedVAO; g_appVBO = savedVBO; g_appProg = savedProg; + return rc; +} + +} // extern "C" + // Destroy the current WebGL context and mint a fresh one on a fresh canvas // element — the harness model of the app's 3D-viewer close/reopen (~wxGLCanvas // destroys its context AND its DOM canvas; a new frame creates new ones). diff --git a/tests/3d-regression/wasm/3d_webgl_test.html b/tests/3d-regression/wasm/3d_webgl_test.html index 63b70d0..7a0e293 100644 --- a/tests/3d-regression/wasm/3d_webgl_test.html +++ b/tests/3d-regression/wasm/3d_webgl_test.html @@ -24,6 +24,13 @@ getCanvasWidth: () => wasmModule.ccall('getCanvasWidth', 'number', [], []), getCanvasHeight: () => wasmModule.ccall('getCanvasHeight', 'number', [], []), recreateContext: () => wasmModule.ccall('recreateContext', 'number', [], []), + appQuadInit: () => wasmModule.ccall('appQuadInit', 'number', [], []), + appQuadDraw: (clear) => wasmModule.ccall('appQuadDraw', 'number', ['number'], [clear]), + ffpMakeStale: () => wasmModule.ccall('ffpMakeStale', null, [], []), + ffpClearStale: () => wasmModule.ccall('ffpClearStale', null, [], []), + createSecondContext: () => wasmModule.ccall('createSecondContext', 'number', [], []), + useContext: (n) => wasmModule.ccall('useContext', 'number', ['number'], [n]), + quadDrawFresh: () => wasmModule.ccall('quadDrawFresh', 'number', [], []), }; diff --git a/tests/3d-regression/wasm/Makefile b/tests/3d-regression/wasm/Makefile index 339cfc1..9af2843 100644 --- a/tests/3d-regression/wasm/Makefile +++ b/tests/3d-regression/wasm/Makefile @@ -69,7 +69,7 @@ CFLAGS = $(OPT_FLAGS) $(DEPS_EH_FLAGS) -I$(PROJECT_ROOT)/wasm/stubs BASE_LDFLAGS = $(DEPS_EH_FLAGS) \ -sALLOW_MEMORY_GROWTH=1 \ -sERROR_ON_UNDEFINED_SYMBOLS=0 \ - -sEXPORTED_FUNCTIONS=['_main','_runScenario','_getTotalScenarios','_getScenarioName','_getCanvasWidth','_getCanvasHeight','_recreateContext'] \ + -sEXPORTED_FUNCTIONS=['_main','_runScenario','_getTotalScenarios','_getScenarioName','_getCanvasWidth','_getCanvasHeight','_recreateContext','_appQuadInit','_appQuadDraw','_ffpMakeStale','_ffpClearStale','_createSecondContext','_useContext','_quadDrawFresh'] \ -sEXPORTED_RUNTIME_METHODS=['ccall','cwrap'] \ -sMODULARIZE=1 \ -sEXPORT_NAME='create3DTest' \ diff --git a/tests/apps/3d-webgl/3d_webgl_test.html b/tests/apps/3d-webgl/3d_webgl_test.html index 63b70d0..7a0e293 100644 --- a/tests/apps/3d-webgl/3d_webgl_test.html +++ b/tests/apps/3d-webgl/3d_webgl_test.html @@ -24,6 +24,13 @@ getCanvasWidth: () => wasmModule.ccall('getCanvasWidth', 'number', [], []), getCanvasHeight: () => wasmModule.ccall('getCanvasHeight', 'number', [], []), recreateContext: () => wasmModule.ccall('recreateContext', 'number', [], []), + appQuadInit: () => wasmModule.ccall('appQuadInit', 'number', [], []), + appQuadDraw: (clear) => wasmModule.ccall('appQuadDraw', 'number', ['number'], [clear]), + ffpMakeStale: () => wasmModule.ccall('ffpMakeStale', null, [], []), + ffpClearStale: () => wasmModule.ccall('ffpClearStale', null, [], []), + createSecondContext: () => wasmModule.ccall('createSecondContext', 'number', [], []), + useContext: (n) => wasmModule.ccall('useContext', 'number', ['number'], [n]), + quadDrawFresh: () => wasmModule.ccall('quadDrawFresh', 'number', [], []), }; diff --git a/tests/e2e/3d-webgl.spec.ts b/tests/e2e/3d-webgl.spec.ts index 3d90e48..6ff14af 100644 --- a/tests/e2e/3d-webgl.spec.ts +++ b/tests/e2e/3d-webgl.spec.ts @@ -159,6 +159,136 @@ test.describe('3D WebGL Regression', () => { ).toBeLessThan(0.001); }); + /** + * Engine-toggle regressions: the raytracing round-trip poisons the shim's global + * FFP routing state (a MODEL_3D BeginDrawMulti-style window leaves GL_VERTEX_ARRAY + * enabled with a VBO captured), after which modern-GL consumers (the raytracer + * blit, the 2D GAL) get their glDrawArrays misrouted through the FFP pipeline. + * Three deterministic reproductions of the traced failure modes: + * - T1: a misrouted draw must not corrupt the CALLER's VAO (blit collision). + * - T2: a draw under a FOREIGN context (the 2D GAL model) must pass through. + * - T3: FFP client-array state must die with its context. + */ + test.describe('FFP routing isolation (engine-toggle model)', () => { + const glLines = (arr: string[]) => arr.filter((l) => l.includes('[gl1] WebGL context changed')); + + test('T1: misrouted draw does not corrupt the victim VAO', async ({ page }) => { + await page.goto('/3d-webgl/3d_webgl_test.html'); + await page.waitForFunction(() => (window as any).threeDTest?.isReady(), undefined, { timeout: 60000 }); + + const rc = await page.evaluate(() => { + const t = (window as any).threeDTest; + const r: Record = {}; + r.init = t.appQuadInit(); + r.clean = t.appQuadDraw(1); // sanity: quad renders before poisoning + t.ffpMakeStale(); // the BeginDrawMulti-window leak shape + r.routed = t.appQuadDraw(0); // blit-style draw with victim VAO bound → misrouted today + t.ffpClearStale(); // later state cleanup (flag off again) + r.after = t.appQuadDraw(1); // the victim draws again — must still work + return r; + }); + await page.evaluate(() => new Promise(requestAnimationFrame)); + const green = await page.evaluate(() => { + const el = document.getElementById('canvas') as HTMLCanvasElement; + const c = document.createElement('canvas'); + c.width = el.width; c.height = el.height; + const x = c.getContext('2d', { willReadFrequently: true })!; + x.drawImage(el, 0, 0); + const d = x.getImageData(0, 0, el.width, el.height).data; + let g = 0, n = 0; + for (let i = 0; i < 16; i++) + for (let j = 0; j < 16; j++) { + const p = (Math.floor((el.height * j) / 16) * el.width + Math.floor((el.width * i) / 16)) * 4; + n++; + if (d[p] < 40 && d[p + 1] > 200 && d[p + 2] < 40) g++; + } + return g / n; + }); + console.log(`[TEST] T1 rc=${JSON.stringify(rc)} greenFraction=${green}`); + expect(rc.init, 'app quad init').toBe(0); + expect(rc.clean, 'app quad renders before poisoning').toBe(0); + expect(rc.after, + 'the victim VAO must survive a misrouted draw (nonzero = the shim scribbled its attributes)') + .toBe(0); + expect(green, + 'the victim quad must still render green after the misroute (blank = corrupted VAO)') + .toBeGreaterThan(0.9); + }); + + test('T2: stale FFP flag must not route draws under a foreign context', async ({ page }) => { + const consoleLines: string[] = []; + page.on('console', (m) => consoleLines.push(m.text())); + + await page.goto('/3d-webgl/3d_webgl_test.html'); + await page.waitForFunction(() => (window as any).threeDTest?.isReady(), undefined, { timeout: 60000 }); + + const rc = await page.evaluate((idx) => { + const t = (window as any).threeDTest; + const r: Record = {}; + r.scenario = t.runScenario(idx); // adopt ctx1 as the shim owner (real FFP work) + t.ffpMakeStale(); // poison the global mirror under ctx1 + r.ctx2 = t.createSecondContext(); // the "2D GAL" context + r.use2 = t.useContext(2); + r.draw2 = t.quadDrawFresh(); // GAL-style modern draw → must pass through + t.useContext(1); + (window as any).threeDTest.ffpClearStale(); + return r; + }, MANIFEST.scenarios.indexOf('redraw-mini-board-navigator')); + const thrash = glLines(consoleLines); + console.log(`[TEST] T2 rc=${JSON.stringify(rc)} gl1Lines=${thrash.length}`); + expect(rc.scenario, 'owner-context scenario render').toBe(0); + expect(rc.ctx2, 'second context created').toBe(0); + expect(rc.use2, 'second context current').toBe(0); + expect(rc.draw2, + 'a modern-GL draw under a foreign context must pass through untouched ' + + '(nonzero = it was routed through the FFP pipeline)') + .toBe(0); + expect(thrash, + 'the context guard must not fire for foreign-context draws (thrash)').toEqual([]); + }); + + test('T3: FFP client-array state dies with its context', async ({ page }) => { + await page.goto('/3d-webgl/3d_webgl_test.html'); + await page.waitForFunction(() => (window as any).threeDTest?.isReady(), undefined, { timeout: 60000 }); + + const rc = await page.evaluate(() => { + const t = (window as any).threeDTest; + const r: Record = {}; + t.ffpMakeStale(); // poison under the original context + r.recreate = t.recreateContext(); // context (and its VBO) destroyed + r.draw = t.quadDrawFresh(); // fresh modern draw in the new context + return r; + }); + await page.evaluate(() => new Promise(requestAnimationFrame)); + const green = await page.evaluate(() => { + const el = document.getElementById('canvas') as HTMLCanvasElement; + const c = document.createElement('canvas'); + c.width = el.width; c.height = el.height; + const x = c.getContext('2d', { willReadFrequently: true })!; + x.drawImage(el, 0, 0); + const d = x.getImageData(0, 0, el.width, el.height).data; + let g = 0, n = 0; + for (let i = 0; i < 16; i++) + for (let j = 0; j < 16; j++) { + const p = (Math.floor((el.height * j) / 16) * el.width + Math.floor((el.width * i) / 16)) * 4; + n++; + if (d[p] < 40 && d[p + 1] > 200 && d[p + 2] < 40) g++; + } + return g / n; + }); + console.log(`[TEST] T3 rc=${JSON.stringify(rc)} greenFraction=${green}`); + expect(rc.recreate, 'context recreation').toBe(0); + expect(rc.draw, + 'client-array state from a dead context must not route draws in the new one ' + + '(nonzero = the stale enabled flag survived the context change)') + .toBe(0); + expect(green, + 'the fresh-context quad must render green (blank/garbage = draw was routed ' + + 'through the FFP pipeline with dead-context state)') + .toBeGreaterThan(0.9); + }); + }); + test('render all scenarios', async ({ page }) => { test.setTimeout(300000); diff --git a/tests/kicad/3d-viewer-engine-toggle.spec.ts b/tests/kicad/3d-viewer-engine-toggle.spec.ts new file mode 100644 index 0000000..09e58ee --- /dev/null +++ b/tests/kicad/3d-viewer-engine-toggle.spec.ts @@ -0,0 +1,126 @@ +import { test, expect } from './fixtures'; +import { clickByTooltip, clickToolbarTool, shotPath } from '../e2e/utils/element-tracker'; +import { waitForPcbnew } from './utils/pcbnew-ready'; +import { DEMO, countGlCanvases, loadBoard, logThreeDDiag, openThreeDViewer, waitForThreeDRender } + from './utils/threed-viewer'; + +/** + * Engine round-trip smoke: OpenGL → raytracing → OpenGL must render and stay + * console-clean. + * + * The user-visible regression: after the round-trip the viewer showed only the + * background gradient, with `[gl1] WebGL context changed` firing mid-session and + * INVALID_OPERATION storms on BOTH WebGL contexts. Root cause: the wasm/gl1 shim's + * FFP draw routing keyed on a single process-global client-array flag — an + * interrupted fixed-function window (MODEL_3D::BeginDrawMulti) left it set, after + * which the raytracer blit's and the 2D GAL's glDrawArrays were routed through the + * FFP pipeline, scribbling their own VAOs (the persistent blank). Fixed by + * owner-context routing gates + VAO isolation in the shim, client-state teardown in + * contextSync, and client-array disables in the blit preamble; the deterministic + * mechanism-level reproductions live in tests/e2e/3d-webgl.spec.ts (T1-T3). + * + * This spec drives the REAL viewer round-trip. The exact race needs interaction + * timing no harness forces reliably, so this is the happy-path gate: with the fixes + * the round-trip must ALWAYS render and never emit a context-change or + * INVALID_OPERATION line. + * + * CI-skip mirrors 3d-viewer-deadlock.spec.ts: the raytrace pass must converge in + * seconds, which needs a real GPU-adjacent machine, not CI's contended software GL. + * ISOLATED in its own spec file (own worker) like the other heavy 3D specs. + */ +test.describe('3D viewer engine toggle', () => { + test.skip(!!process.env.CI, 'raytrace-convergence timing needs a real GPU; the ' + + 'mechanism-level coverage runs everywhere in e2e/3d-webgl.spec.ts'); + + test.describe.configure({ mode: 'serial' }); + test.setTimeout(480000); + + test('raytracing round-trip re-renders and stays console-clean', async ({ page, testLogger }) => { + await page.goto('/kicad/pcbnew.html'); + await waitForPcbnew(page); + await loadBoard(page, testLogger); + + const glBefore = await countGlCanvases(page); + await openThreeDViewer(page, glBefore); + await waitForThreeDRender(page); + await logThreeDDiag(page, 'engine-toggle: first OpenGL render'); + + // Content snapshot of the newest glcanvas: sampled colour count + hash. + const snap = () => page.evaluate(() => { + const list = document.querySelectorAll('canvas[id^="glcanvas-"]'); + const el = list[list.length - 1] as HTMLCanvasElement; + const tmp = document.createElement('canvas'); + tmp.width = el.width; tmp.height = el.height; + const ctx = tmp.getContext('2d', { willReadFrequently: true })!; + ctx.drawImage(el, 0, 0); + const img = ctx.getImageData(0, 0, el.width, el.height).data; + const colors = new Set(); + let hash = 0; + for (let i = 0; i < 24; i++) { + for (let j = 0; j < 24; j++) { + const p = (Math.floor(el.height * j / 24) * el.width + + Math.floor(el.width * i / 24)) * 4; + colors.add(`${img[p]},${img[p + 1]},${img[p + 2]}`); + hash = (hash * 31 + img[p] + img[p + 1] * 7 + img[p + 2] * 13) | 0; + } + } + return { colors: colors.size, hash }; + }); + + const before = await snap(); + console.log(`[TEST] OpenGL frame: ${JSON.stringify(before)}`); + + // ── Toggle to raytracing. KNOWN ISSUE (3d-viewer-deadlock.spec.ts, 2026-07-04, + // re-verified 2026-08-13): the raytraced image never displays on the wasm + // build — but the ENGINE does switch and the reload machinery does run, and + // that switch-back reload (display-list re-record with the GL context lock + // released) is exactly the poisoning path this spec guards. So no + // blitted-frame expectation here; the round-trip itself is the test. ── + const toggled = (await clickToolbarTool(page, 'Use raytracing')) + || (await clickByTooltip(page, 'Render current view using Raytracing')); + expect(toggled, 'the raytracer toolbar toggle should be clickable').toBe(true); + + // Give the raytracer engine a bounded window to run repaints: poll the canvas + // until two consecutive samples agree (content is allowed to stay identical — + // the known-issue inert blit — or to change and settle). + let rtSettle = await snap(); + await expect.poll(async () => { + const next = await snap(); + const stable = next.hash === rtSettle.hash; + rtSettle = next; + return stable; + }, { timeout: 60000, intervals: [2000] }).toBe(true); + await logThreeDDiag(page, 'engine-toggle: raytracer engine window elapsed'); + await page.screenshot({ path: shotPath(page, `3d-engine-toggle-rt-${DEMO.name}.png`), + scale: 'css' }); + + // ── Toggle back to the OpenGL engine. ── + const toggledBack = (await clickToolbarTool(page, 'Use raytracing')) + || (await clickByTooltip(page, 'Render current view using Raytracing')); + expect(toggledBack, 'the raytracer toggle should toggle back').toBe(true); + + // THE regression assertion: the OpenGL engine must render the board again. + await expect.poll(async () => (await snap()).colors, { + message: 'the OpenGL engine must re-render the board after the round-trip ' + + '(a near-uniform canvas means FFP draws were misrouted/corrupted)', + timeout: 90000, + intervals: [1000], + }).toBeGreaterThan(8); + await page.screenshot({ path: shotPath(page, `3d-engine-toggle-back-${DEMO.name}.png`), + scale: 'css' }); + + // ── Console gates. ── + const allLines = [...testLogger.consoleLogs, ...testLogger.errors]; + const ctxChanges = allLines.filter((l) => l.includes('[gl1] WebGL context changed')); + expect(ctxChanges, + 'the gl1 context guard must never fire during an engine toggle ' + + '(the context does not change) — firing means draws ran under a foreign context') + .toEqual([]); + const glErrors = allLines.filter((l) => l.includes('INVALID_OPERATION')); + expect(glErrors, + `no INVALID_OPERATION storms during the round-trip:\n${glErrors.slice(0, 5).join('\n')}`) + .toEqual([]); + const aborts = allLines.filter((l) => l.includes('Aborted(')); + expect(aborts, `WASM aborted during the engine toggle:\n${aborts.join('\n\n')}`).toEqual([]); + }); +}); diff --git a/wasm/gl1/README.md b/wasm/gl1/README.md index af0a211..ef5f974 100644 --- a/wasm/gl1/README.md +++ b/wasm/gl1/README.md @@ -34,11 +34,28 @@ Link sites (both read `sources.txt` + `wrapped_symbols.txt`): ## Draw routing -A `glDrawArrays`/`glDrawElements` call is FFP traffic iff `GL_VERTEX_ARRAY` -client state is enabled: only GL1 code calls `glEnableClientState`, while the -raytracer blit (`eda_3d_canvas_wasm.cpp`) and the 2D WebGL GAL drive their own -GLSL programs and never touch client state — their draws pass through -untouched. +A `glDrawArrays`/`glDrawElements` call is FFP traffic iff BOTH hold: + +- the CURRENT WebGL context is the shim's **owner context** (`contextIsOwner()`, + adopted at the first FFP client-state mutation or `programSync()` in a + context — a draw under any other context is a modern-GL consumer by + definition and always passes through, even mid-display-list-recording); +- `GL_VERTEX_ARRAY` client state is enabled (only GL1 code calls + `glEnableClientState`; the raytracer blit and the 2D WebGL GAL drive their + own GLSL programs). + +The owner gate exists because the client-state flag is process-global and an +interrupted FFP window (e.g. `MODEL_3D::BeginDrawMulti` loops crossing a JSPI +suspension) can leave it set — without the gate, the 2D GAL's draws got routed +through the FFP pipeline (the engine-toggle blank-viewer bug). Two further +hardenings from the same bug: + +- **VAO isolation** (`ScopedDefaultVAO`, gl1_draw.cpp): draw executors bind + VAO 0 for their attribute setup and restore the caller's binding — a routed + draw arriving with the caller's VAO bound (the blit's attribute 0 collides + with `ATTR_POSITION`) must never scribble it. +- `contextSync()` resets the client-array mirror entirely on a context change: + enables, pointers and captured VBO names all described the dead context. ## Invariants that are easy to break (learned from the native goldens) diff --git a/wasm/gl1/include/gl1_shim.h b/wasm/gl1/include/gl1_shim.h index d1f2220..74370e7 100644 --- a/wasm/gl1/include/gl1_shim.h +++ b/wasm/gl1/include/gl1_shim.h @@ -230,6 +230,12 @@ void stateAlphaFunc( GLenum func, GLclampf ref ); // immediate-mode state: the change can be detected mid-scene-rebuild, and // those modules are context-agnostic CPU state. void contextSync(); +// True when the CURRENT WebGL context is the shim's owner context (or no owner +// exists yet). The __wrap_* draw interceptors gate FFP routing on this: only +// the owner context's draws can be fixed-function traffic — a draw under any +// other context (the 2D GAL, a recreated canvas) always passes through, no +// matter what the global client-array mirror says. (gl1_state.cpp) +bool contextIsOwner(); // Per-TU cache drops invoked by contextSync() on a context change. void shadersDropContextObjects(); // FFP program + uniform locations + fail latch void drawDropContextObjects(); // stream/scratch VBOs diff --git a/wasm/gl1/src/gl1_draw.cpp b/wasm/gl1/src/gl1_draw.cpp index 7c43774..3849bed 100644 --- a/wasm/gl1/src/gl1_draw.cpp +++ b/wasm/gl1/src/gl1_draw.cpp @@ -40,11 +40,40 @@ enum }; +// The shim's attribute setup assumes the DEFAULT VAO (see the VAO-policy note +// above) — but a routed draw can arrive with the CALLER's VAO bound (the +// raytracer blit and the 2D GAL both bind their own VAO right before drawing, +// and their attribute 0 collides with ATTR_POSITION). Scribbling attribute +// state into that VAO corrupts the caller PERMANENTLY — the engine-toggle +// blank-viewer amplifier. Every draw executor therefore isolates itself on +// VAO 0 and restores the caller's binding afterwards. +class ScopedDefaultVAO +{ +public: + ScopedDefaultVAO() + { + glGetIntegerv( GL_VERTEX_ARRAY_BINDING, &m_prev ); + if( m_prev != 0 ) + glBindVertexArray( 0 ); + } + ~ScopedDefaultVAO() + { + if( m_prev != 0 ) + glBindVertexArray( (GLuint) m_prev ); + } + +private: + GLint m_prev = 0; +}; + + void drawImmVertices( GLenum mode, const ImmVertex* verts, GLsizei count ) { if( !programSync() ) return; + ScopedDefaultVAO vaoGuard; + if( !s_streamVBO ) glGenBuffers( 1, &s_streamVBO ); @@ -203,6 +232,8 @@ void drawArraysWithSources( GLenum mode, GLsizei count, const AttribSource aSrc[ return; } + ScopedDefaultVAO vaoGuard; + GLint prevArrayBuffer = 0; glGetIntegerv( GL_ARRAY_BUFFER_BINDING, &prevArrayBuffer ); diff --git a/wasm/gl1/src/gl1_entry_ffp.cpp b/wasm/gl1/src/gl1_entry_ffp.cpp index ec6b874..99178a4 100644 --- a/wasm/gl1/src/gl1_entry_ffp.cpp +++ b/wasm/gl1/src/gl1_entry_ffp.cpp @@ -407,6 +407,15 @@ void glColorMaterial( GLenum face, GLenum mode ) static ClientArray* clientArraySlot( GLenum cap ) { + // FFP client-state mutation is the earliest signal that fixed-function + // code is working in the CURRENT context — adopt/refresh ownership here, + // BEFORE the mutation lands in the mirror. This is what moves ownership + // to a recreated 3D context (its scene rebuild calls gl*Pointer / + // glEnableClientState long before its first draw), and what guarantees a + // context change wipes stale client-array state before new state is + // recorded. Only FFP callers reach these names; the 2D GAL never does. + contextSync(); + State& s = S(); switch( cap ) @@ -456,6 +465,7 @@ static GLuint currentArrayBufferBinding() void glVertexPointer( GLint size, GLenum type, GLsizei stride, const GLvoid* ptr ) { + contextSync(); // see clientArraySlot() ClientArray& a = S().clientArrays[CA_VERTEX]; a.size = size; a.type = type; diff --git a/wasm/gl1/src/gl1_entry_wrapped.cpp b/wasm/gl1/src/gl1_entry_wrapped.cpp index 64c8109..4910980 100644 --- a/wasm/gl1/src/gl1_entry_wrapped.cpp +++ b/wasm/gl1/src/gl1_entry_wrapped.cpp @@ -78,6 +78,18 @@ void __wrap_glGetFloatv( GLenum pname, GLfloat* params ) void __wrap_glDrawArrays( GLenum mode, GLint first, GLsizei count ) { + // Only the shim's owner context can carry FFP traffic. A draw under any + // other context (the 2D GAL, the raytracer blit after a canvas swap) is a + // modern-GL consumer by definition — pass it through even if the global + // client-array mirror was left enabled by an interrupted FFP window, and + // even while a display list is recording (a foreign draw must never be + // swallowed into the owner's open list). + if( !contextIsOwner() ) + { + __real_glDrawArrays( mode, first, count ); + return; + } + if( dlistRecording() ) { dlistRecordDrawArrays( mode, first, count ); @@ -98,6 +110,13 @@ void __wrap_glDrawArrays( GLenum mode, GLint first, GLsizei count ) void __wrap_glDrawElements( GLenum mode, GLsizei count, GLenum type, const GLvoid* indices ) { + // Same owner-context gate as __wrap_glDrawArrays. + if( !contextIsOwner() ) + { + __real_glDrawElements( mode, count, type, indices ); + return; + } + if( dlistRecording() ) { GL1_WARN_ONCE( "glDrawElements inside glNewList is not supported — dropped" ); diff --git a/wasm/gl1/src/gl1_state.cpp b/wasm/gl1/src/gl1_state.cpp index 026ab0f..5a612dc 100644 --- a/wasm/gl1/src/gl1_state.cpp +++ b/wasm/gl1/src/gl1_state.cpp @@ -41,6 +41,16 @@ static int currentContextId() // first contextSync() under a live context. static int s_ownerContext = 0; +bool contextIsOwner() +{ + int cur = currentContextId(); + + // No live context: nothing can be drawn anyway. No owner yet: the first + // FFP consumer under any context becomes the owner (adopted in + // contextSync() on its first programSync()). + return cur != 0 && ( s_ownerContext == 0 || cur == s_ownerContext ); +} + void contextSync() { int cur = currentContextId(); @@ -61,8 +71,13 @@ void contextSync() State& s = S(); s.boundTexture2D = 0; + // Client-array state dies with its context: the enables, the captured + // VBO names AND the CPU pointers all described the dead context's + // world. Keeping `.enabled` was the engine-toggle bug's tail — a flag + // left set by the old context kept routing modern-GL draws through the + // FFP pipeline in the new one. for( int i = 0; i < CA_COUNT; ++i ) - s.clientArrays[i].boundBuffer = 0; + s.clientArrays[i] = ClientArray(); // The new program starts with default-initialized uniforms; force a // full re-upload on its first sync.