Skip to content

webcanvas: memory safety++#243

Open
tinyjin wants to merge 2 commits intomainfrom
jinny/wcanvas-memory-safety++
Open

webcanvas: memory safety++#243
tinyjin wants to merge 2 commits intomainfrom
jinny/wcanvas-memory-safety++

Conversation

@tinyjin
Copy link
Member

@tinyjin tinyjin commented Mar 10, 2026

When dispose() was called explicitly and GC fired afterward, the FinalizationRegistry callback would call _tvg_paint_unref / _tvg_gradient_del again on the already-freed pointer, corrupting the WASM heap.

const shape = new TVG.Shape();
shape.appendRect(0, 0, 100, 100).fill(255, 0, 0, 255);
canvas.add(shape);

// User explicitly cleans up
shape.dispose();

// Later, GC collects the Shape object and FinalizationRegistry fires
// calls _tvg_paint_unref on the already-freed pointer -> double-free

This PR adds:

  • Fixes issue by invalidating registry token (ptr = 0) on dispose() so FinalizationRegistry callback becomes a no-op
  • Adds test cases to ensure WASM object memory safety

@tinyjin tinyjin self-assigned this Mar 10, 2026
Copilot AI review requested due to automatic review settings March 10, 2026 13:17
@tinyjin tinyjin requested a review from hermet as a code owner March 10, 2026 13:17
@tinyjin tinyjin added bug Something isn't working test webcanvas WebCanvas labels Mar 10, 2026
@tinyjin tinyjin changed the title WebCanvas: memory safety++ webcanvas: memory safety++ Mar 10, 2026
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves WASM-backed object memory safety by preventing double-free scenarios between manual dispose() and FinalizationRegistry-driven cleanup, and adds tests to validate GC-based cleanup behavior.

Changes:

  • Refactors FinalizationRegistry setup to share logic and avoid capturing this via bound cleanup functions.
  • Updates WasmObject.dispose() to invalidate the finalizer token pointer after manual cleanup.
  • Adds GC/double-free regression tests plus a shared test helper, and enables --expose-gc for the Node test runner.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
packages/webcanvas/vitest.config.ts Enables exposed GC for Node-based Vitest runs.
packages/webcanvas/src/interop/registry.ts Deduplicates registry construction via createRegistry().
packages/webcanvas/src/interop/WasmObject.ts Stores a mutable registry token and zeros ptr on dispose to avoid double-free.
packages/webcanvas/test/helpers.ts Adds utilities to force finalization and assert GC cleanup / no double-free.
packages/webcanvas/test/text.test.ts Adds dispose+GC and GC-cleanup tests for Text.
packages/webcanvas/test/shape.test.ts Adds dispose+GC and GC-cleanup tests for Shape.
packages/webcanvas/test/scene.test.ts Adds dispose+GC and GC-cleanup tests for Scene.
packages/webcanvas/test/picture.test.ts Adds dispose+GC and GC-cleanup tests for Picture.
packages/webcanvas/test/gradient.test.ts Adds dispose+GC and GC-cleanup tests for gradients.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@tinyjin tinyjin force-pushed the jinny/wcanvas-memory-safety++ branch from 881d488 to e117590 Compare March 10, 2026 13:25
Copilot AI review requested due to automatic review settings March 10, 2026 13:28
@tinyjin tinyjin force-pushed the jinny/wcanvas-memory-safety++ branch from e117590 to cdd075a Compare March 10, 2026 13:28
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link

🚀 Playground preview deployment ready!

🎮 Playground: https://thorvg-playground-rdod1sn0e-thorvg-web.vercel.app

@tinyjin
Copy link
Member Author

tinyjin commented Mar 12, 2026

Hold, I've found minor issue. Under reviewing.

@tinyjin tinyjin marked this pull request as draft March 12, 2026 09:57
@tinyjin tinyjin force-pushed the jinny/wcanvas-memory-safety++ branch from cdd075a to b1b1cbf Compare March 18, 2026 05:44
@github-actions
Copy link

🚀 Playground preview deployment ready!

🎮 Playground: https://thorvg-playground-4zqz89iov-thorvg-web.vercel.app

@github-actions
Copy link

🚀 Playground preview deployment ready!

🎮 Playground: https://thorvg-playground-p89jvez8u-thorvg-web.vercel.app

@tinyjin tinyjin force-pushed the jinny/wcanvas-memory-safety++ branch from d47e2e6 to f3fe918 Compare March 18, 2026 12:02
@github-actions
Copy link

🚀 Playground preview deployment ready!

🎮 Playground: https://thorvg-playground-haiknsopy-thorvg-web.vercel.app

@tinyjin tinyjin force-pushed the jinny/wcanvas-memory-safety++ branch from f3fe918 to bcbbca7 Compare March 18, 2026 12:50
@tinyjin tinyjin marked this pull request as ready for review March 18, 2026 12:58
Copilot AI review requested due to automatic review settings March 18, 2026 12:58
@github-actions
Copy link

🚀 Playground preview deployment ready!

🎮 Playground: https://thorvg-playground-1qg9f1fnm-thorvg-web.vercel.app

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@hermet hermet added the showstopper Regression bugs / Critical errors label Mar 20, 2026
tinyjin added 2 commits March 20, 2026 15:28
When dispose() was called explicitly and GC fired afterward, the
FinalizationRegistry callback would call `tvg_paint_unref` / `tvg_gradient_del`
again on the already-freed pointer, corrupting the WASM heap.
@tinyjin tinyjin force-pushed the jinny/wcanvas-memory-safety++ branch from bcbbca7 to 2b41698 Compare March 20, 2026 06:29
@github-actions
Copy link

🚀 Playground preview deployment ready!

🎮 Playground: https://thorvg-playground-rigjmp6oi-thorvg-web.vercel.app

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working showstopper Regression bugs / Critical errors webcanvas WebCanvas

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants