Skip to content

fix(canvas): avoid double-free when ImageData pixel buffers are collected - #149

Merged
triniwiz merged 1 commit into
NativeScript:masterfrom
LorenzGit:contrib/imagedata-ownership
Sep 9, 2026
Merged

triniwiz merged 1 commit into
NativeScript:masterfrom
LorenzGit:contrib/imagedata-ownership

Conversation

@LorenzGit

Copy link
Copy Markdown
Contributor

Accessing ImageData.data creates an ArrayBuffer whose deleter released both its owned U8Buffer and the borrowed ImageData pointer. ImageDataImpl also releases that pointer. On a physical Apple TV with NativeScript 9.1 / V8 14.9, Canvas pixel readback followed by garbage collection crashed in canvas_native_image_data_release on a V8 worker thread.

Release only the U8Buffer in the ArrayBuffer deleter. The Rust buffer already owns a clone of the pixel storage, so a retained Uint8ClampedArray remains valid after its ImageData wrapper is collected. This is an existing Apple bridge ownership bug, independent of tvOS platform support.

Validation: the signed physical-device Release game now passes seven pixel/state checks and the included tools/tests/imagedata-ownership.js regression. The regression creates 4,096 images, forces GC, and checks 64 retained pixel views for stable contents and writable storage. Call await checkImageDataOwnership(ImageData) inside a NativeScript Apple runtime with global.gc available. The public tvOS reviewer fixture also runs it. The original device crash and fresh passing results are retained in the private recovery evidence. iOS hardware has not been rerun for this fix.

@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: e1364cdc-5170-4f75-8ba7-d2eeb4b3c4e1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@triniwiz
triniwiz merged commit db60569 into NativeScript:master Sep 9, 2026
1 check passed
triniwiz added a commit that referenced this pull request Sep 14, 2026
Brings in #144-#149. Conflict resolution notes:

* #147 (V8 14 native bridge) overlapped almost entirely with the V8 14
  migration this branch already carries. Both sides fix the same API break;
  master does it with dual-version shims (canvas::GetAlignedPointer,
  canvas::Receiver, CANVAS_FAST_FUNCTION, #if V8_MAJOR_VERSION >= 14) so one
  tree can build against V8 10.3 and 14, while this branch targets a single
  vendored V8 14.9 (tools/scripts/download-v8.sh) and calls the native APIs
  directly. 118 of the 127 conflicting sources were that difference alone --
  after rewriting master's shims into this branch's idiom, 112 compared
  byte-identical -- so those keep this branch's spelling and the now-unused
  shim block is dropped from Common.h.

* Master's shims also compile fast API calls out on V8 >= 14
  (CANVAS_FAST_FUNCTION -> v8::CFunction{}, c_function -> nullptr). V8 14.9
  still declares both FunctionTemplate::New(..., const CFunction*) and
  NewWithCFunctionOverloads, and this branch's fast paths are built against
  it, so taking that would have silently disabled every fast call in the
  binding layer. Helpers.h keeps this branch's version; the four
  CANVAS_FAST_FUNCTION call sites that auto-merged into
  OES_vertex_array_objectImpl and WEBGL_draw_buffersImpl -- no conflict was
  raised for those -- are restored to v8::CFunction::Make.

* Master's *Array fast overloads are all guarded #if V8_MAJOR_VERSION < 14,
  so they are dead on 14 and equivalent to this branch having removed them.

* #149 (ImageData double free) applies unchanged. canvas_native_image_data_get_data
  borrows its argument and returns a U8Buffer holding a second refcounted
  handle to the same pixels, so ImageDataBuffer must release only the buffer --
  ~ImageDataImpl already releases the ImageData. The comment is reworded from
  master's, which described the buffer as owning a clone of the pixel storage;
  it is a shared handle, and that is what makes the JS data view live.

* Package versions stay on the 3.0.0-alpha line.
* canvas-release.aar keeps this branch's binary; it predates the Android
  render fixes in 638a265 and needs rebuilding from the merged sources.
triniwiz added a commit that referenced this pull request Sep 14, 2026
… them in CI

Nothing in CI ran tests, and the four crates that already had #[test]
modules could not be built on the host at all, so none of them had ever
executed. With the canvas-core build fixed they do; `make test` and two new
CI jobs run them.

ImageData ownership (crates/canvas-c/src/c2d/image_data.rs)
  ImageData is a manually refcounted handle -- Clone copies the raw pixel
  pointer and bumps an Arc, and the storage is freed only when the last
  handle drops. canvas_native_image_data_get_data borrows and hands back a
  second handle to the same pixels, which is what makes the JS `data` view
  live and what #149 turned on. The tests pin all three parts: the buffer
  aliases the ImageData's pixels, releasing the buffer leaves the ImageData
  usable (the double free #149 fixed), and the buffer keeps the pixels alive
  when the ImageData handle is dropped first.

V8 bridge invariants (tools/tests/check-v8-bridge-invariants.py)
  Static checks over the bridge sources for things the compiler accepts but
  that break at runtime or silently cost performance: internal-field and
  external-pointer accessors that are missing their V8 14 tag (an untagged
  read returns null rather than failing to build), V8 14-removed APIs coming
  back, dual-version shims re-entering a branch that ships one vendored V8,
  and the overload helper regressing to a bare pointer -- which is exactly
  how 23 of the 41 fast-call overloads went unregistered. Both of the
  regressions this merge introduced by auto-merge would have been caught
  here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants