fix: dispose WebGL renderers and revoke blob URLs to prevent resource leaks#846
Merged
EdwardMoyse merged 2 commits intoMar 25, 2026
Merged
Conversation
… leaks - Add renderer.dispose() and domElement.remove() calls in RendererManager.cleanup() to free WebGL contexts on re-initialization - Replace saveBlob IIFE with a method that revokes object URLs after download and removes the temporary anchor element from the DOM - Reuse saveBlob in makeScreenShot to eliminate duplicate blob URL leak - Add 3 unit tests for cleanup() covering renderer disposal, resize listener removal, and safe no-op when no handler is set Closes HSF#845
There was a problem hiding this comment.
Pull request overview
Fixes long-running session resource leaks in phoenix-event-display by disposing Three.js renderers during lifecycle cleanup and revoking Blob object URLs used for downloads/screenshot export.
Changes:
- Dispose/remove renderer DOM elements during
RendererManager.cleanup()to avoid leaking WebGL contexts. - Refactor download logic into
ThreeManager.saveBlob()and revoke Blob URLs after triggering download; reuse for screenshots. - Add unit tests covering renderer disposal, DOM removal, and resize listener cleanup.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
packages/phoenix-event-display/src/managers/three-manager/renderer-manager.ts |
Adds renderer disposal + canvas removal during cleanup. |
packages/phoenix-event-display/src/managers/three-manager/index.ts |
Reworks Blob download helper to revoke object URLs; reuses for screenshot export. |
packages/phoenix-event-display/src/tests/managers/three-manager/renderer-manager.test.ts |
Adds tests for new cleanup behavior (dispose/remove/listener removal). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…er reuse Keep the main WebGLRenderer alive in cleanup() since init() reuses it via getMainRenderer(). Only secondary renderers (e.g. overlay) are disposed. The overlay reference is cleared so stale overlay state does not persist across re-initialization. Without this, a cleanup() -> init() cycle would leave mainRenderer pointing to a disposed WebGLRenderer, breaking setLocalClippingEnabled, getLocalClipping, and any subsequent rendering.
Member
|
Thank you for this! |
Collaborator
Author
|
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.
Resolves #845
Three resource leaks compound over time in long-running sessions:
WebGL contexts never freed -
RendererManager.cleanup()removed the resize listener but never calledrenderer.dispose(). On re-initialization, old WebGL contexts leaked. Browsers cap contexts at ~8–16; after that, the oldest context is silently lost.Blob URLs never revoked in
saveBlob-URL.createObjectURL()was called on each download butURL.revokeObjectURL()was never called, keeping blob references in memory until page unload. The` element was also permanently appended to the DOM via an IIFE.Duplicate leak in
makeScreenShot- The screenshot export created its own<a>element and blob URL with the same missing cleanup pattern.Changes
renderer-manager.ts-cleanup()now callsrenderer.dispose()andrenderer.domElement.remove()for every renderer in the list, then clears the array.three-manager/index.ts- Replaced thesaveBlobIIFE with aprivate saveBlob()method that creates a temporary<a>element, triggers the download, then revokes the object URL and removes the element viasetTimeout.makeScreenShotnow reuses this method instead of duplicating the pattern.renderer-manager.test.ts- Added 3 test cases:init()+cleanup()cleanup()does not throw when no resize handler is setTesting
All 29 test suites / 178 tests pass (including 3 new).