Fix resource leaks in Application teardown (WebGL context + renderer event listeners) - #1582
Merged
Conversation
Teardown deleted every GL object the renderer owned and removed the canvas from the DOM, but never handed back the context itself. A canvas keeps its context until the canvas is garbage-collected — non-deterministic and routinely delayed — so each destroyed application left a live context behind. Browsers cap how many they keep (~16 on Chromium) and force-lose the oldest past that. A long-lived page that builds and tears down several applications therefore accumulates dead-but-unfreed contexts until an unrelated later getContext() stalls or returns one already lost. That hits any SPA that unmounts a game view — the examples gallery does exactly this on every navigation — and it is also what makes unrelated specs time out in CI, where the shared browser session spans every spec file. WebGLRenderer.destroy() now releases the context through WEBGL_lose_context. The hint had been sitting commented out in this same file since forever (webgl_renderer.js:295-296). destroy() stays idempotent — GL calls on a lost context are no-ops by spec — and it is already terminal (Application.init() refuses to run again afterwards), so losing the context forecloses nothing. Drivers without the extension are unaffected. webgl_vao_teardown.spec.js records the new contract: the old test asserted getError() === NO_ERROR after destroy, which a deliberately-lost context cannot satisfy; it now asserts isContextLost() and that a second teardown does not throw. Verified to fail with the fix reverted. A "create/destroy N applications past the context cap" test was written and deliberately REMOVED — it passed identically with and without the fix, so it discriminated nothing. The reasoning is left as a comment so the dead end is not re-derived. Also drops the unnecessary Application from octree_adversarial.spec.js: it only ever needed a `world` for the isFloating branch and nothing there floats, so it now stands up no canvas at all (import 280ms -> 12ms). Full suite 234 files / 5851 pass, eslint 0 errors, biome clean, tsc clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi
Follow-up to the destroy() context-release fix. An audit parsing the `renderer:` argument at each `new Application(` call site (not grepping filenames — camera3d_integration has 19 Applications but deliberately uses CANVAS) found 24 GL-capable Applications created in specs that never call destroy(). Against a Chromium cap of ~16 live contexts, that is what pushed unrelated suites' beforeAll hooks past their 90s timeout on CI. Two kinds of offender, two fixes: - Suites that never needed GL at all — bezier, linedash, timer — pinned to video.CANVAS. An unspecified renderer resolves to AUTO, so these were silently holding a WebGL context for the whole session. - "Reset-only" Applications: a fresh app built inside afterAll purely to restore global defaults (Camera2d, a clean world) for later spec files. Eight of these across depth, glcore-audit, webgl_save_restore, mesh, camera3d_integration, lighting3d, gltf_model and canvas-cliprect-transform took a context under AUTO and were never destroyed. They do not render, so they are now CANVAS. gltf_model was already using CANVAS for its real app and AUTO for the throwaway. Also adds afterAll teardown to bezier, linedash and timer: a Canvas Application still leaves a canvas, listeners and timers live in the shared browser session, so it should be destroyed whichever backend it uses. Full suite 234 files / 5851 pass, eslint 0 errors, biome clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi
WebGLRenderer subscribed to GAME_RESET, ONCONTEXT_RESTORED and
CANVAS_ONRESIZE; CanvasRenderer to GAME_RESET. All four were inline
anonymous arrows, which cannot be passed to off() — so nothing could ever
unregister them. CanvasRenderer had no destroy() at all, inheriting the
base class no-op.
Two consequences, both silent. A destroyed renderer kept reacting to those
events. And each handler closes over the renderer, so the subscription
pinned the renderer, its batchers and its GPU objects against garbage
collection — which is why releasing the GL context in the previous commit
was not sufficient on its own: the JS graph stayed reachable from the
event bus.
The handlers are now per-instance fields and destroy() calls off() on each,
matching what WebGPURenderer already did (it stores this.onGameReset /
this.onCanvasResize and unregisters both).
Adds tests/application_lifecycle.spec.js. It asserts the structural
property that made the bug possible — handlers must be retrievable
per-instance references, and CanvasRenderer must define its own destroy().
Two stronger tests were attempted and abandoned, and the spec records why
so the dead ends are not re-walked:
- "destroy, then emit(GAME_RESET), assert no reaction" — emit reaches
every listener in the shared browser session, including ones left by
other spec files, and throws partway through. A try/catch around it
would pass without reaching the handler under test.
- "spy on event.off" — vitest browser mode cannot spy on ESM exports.
The structural assertion is necessary but not sufficient: it would not
catch a destroy() that simply forgot to call off(). A listener-count
assertion would be strictly better and needs a test-visible way to inspect
the bus. Said so in the spec rather than implying more coverage than there
is.
Note World (GAME_RESET) and Container (CANVAS_ONRESIZE) have the same
unpaired-subscription shape and are NOT fixed here — Container matters most
since every container in the scene graph takes one.
Full suite 235 files / 5854 pass, eslint 0 errors, biome clean, tsc clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi
Same class as the renderer fix in the previous commit, in the scene graph. - Container (root only — the subscription is guarded by `this.root === true`, so it is one per world, not one per node) subscribed to CANVAS_ONRESIZE with an inline arrow. Unremovable, and the closure kept the container and its entire child tree reachable from the event bus. - World subscribed to GAME_RESET (handler + context) and LEVEL_LOADED (inline arrow), and had no destroy() of its own — so a destroyed world kept resetting itself on GAME_RESET and clearing a broadphase nobody read on LEVEL_LOADED, and could never be collected. Both now hold their handlers as fields, and destroy() calls off() before delegating to the container teardown. Container clears the field so a second destroy is a no-op rather than a double off(). Extends tests/application_lifecycle.spec.js with the two cases. Same caveat as the renderer tests, already documented in that file: these assert the structural property (handlers are retrievable, and cleared on teardown), which is necessary but does not prove `off()` was called. Full suite 235 files / 5856 pass, eslint 0 errors, biome clean, tsc clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three related leaks in
Applicationteardown, found while chasing intermittent CItestfailures. The symptom was always the same and always misleading:renderTargetPool.spec.jsandtexturecache-batcher-reset.spec.jsfailing withHook timed out in 90000msin abeforeAllcallinggetWebGLRenderer— never an assertion failure, 5840 other tests passing, and a re-run of the identical commit often going green.Those two specs were only ever the victims.
vitest.config.ts:28-31already named the mechanism:hookTimeout: 90000was the mitigation. These are the causes.1.
destroy()never released the GL contextWebGLRenderer.destroy()deleted every GL object it owned andApplication.destroy()removed the canvas from the DOM — but neither handed back the context. A canvas keeps its context until the canvas is garbage-collected, which is non-deterministic and routinely delayed. Chromium keeps ~16 live and force-loses the oldest past that.Now released via
WEBGL_lose_context. The hint had been sitting commented out in that same file (webgl_renderer.js:295-296).destroy()is already terminal —init()refuses to run again — so losing the context forecloses nothing.This affects shipped games, not just CI: any SPA that unmounts a game view leaks a context per teardown. The examples gallery does exactly that on every navigation.
2. Renderers never unregistered their global event listeners
WebGLRenderersubscribed toGAME_RESET,ONCONTEXT_RESTORED,CANVAS_ONRESIZE;CanvasRenderertoGAME_RESET. All four were inline anonymous arrows — which cannot be passed tooff(), so nothing could ever unregister them.CanvasRendererhad nodestroy()at all, inheriting the base no-op.This is why fix 1 alone was not enough: each handler closes over the renderer, so the subscription pinned the renderer, its batchers and its GPU objects against garbage collection. The context was freed; the JS graph stayed reachable from the event bus.
Handlers are now per-instance fields and
destroy()unregisters each — matching what WebGPU already did correctly (webgpu_renderer.js:3268-3269). That asymmetry showed up twice in this work: the newer backend was the careful one both times.3. Specs leaked 24 GL-capable Applications
An audit parsing the
renderer:argument at eachnew Application(call site — not grepping filenames, sincecamera3d_integrationhas 19 Applications but deliberately usesCANVAS— found 24 GL-capable Applications created in specs that never calldestroy(). Against a ~16 cap, that is the pile-up.Two kinds, two fixes:
bezier,linedash,timer— pinned tovideo.CANVAS. An unspecified renderer resolves toAUTO, so these silently held a context all session.afterAllpurely to restore global defaults for later spec files. Eight of these took a context underAUTOand were never destroyed. They do not render, so they are nowCANVAS.Also adds
afterAllteardown to the three converted suites — a CanvasApplicationstill leaves a canvas, listeners and timers behind, so it should be destroyed whichever backend it uses.Tests
tests/application_lifecycle.spec.js(new) and an updated contract inwebgl_vao_teardown.spec.js, whose old assertion (getError() === NO_ERRORafter destroy) a deliberately-lost context cannot satisfy. It now assertsisContextLost()— verified to fail with the fix reverted.Two things deliberately not shipped, both recorded in-file so they are not re-derived:
emit(GAME_RESET), assert no reaction" test —emitreaches every listener in the shared browser session including ones left by other spec files, and throws partway through; atry/catchwould pass without reaching the handler under test. Spying onevent.offinstead is impossible in vitest browser mode (ESM namespaces are not configurable).So the lifecycle spec asserts the structural property that made the bug possible: handlers must be retrievable per-instance references. That is necessary but not sufficient — it would not catch a
destroy()that simply forgot to calloff(). Stated plainly in the spec rather than implying more coverage than exists.Known remaining, not fixed here
World(GAME_RESET) andContainer(CANVAS_ONRESIZE) have the same unpaired-subscription shape.Containeris the significant one — every container in the scene graph takes a listener, so a level reload accumulates them. Left out to keep this reviewable; worth its own change.Whether this fully stops the CI timeouts is not proven — it removes the accumulation mechanism, and the leak counts are measured, but I could not reproduce the timeout locally on a machine with a real GPU.
Gates
tsc --noEmit🤖 Generated with Claude Code
https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi