Release fix memory leak - #1
Open
iurisilvio wants to merge 20 commits into
Open
Conversation
The rotate90 and rotate270 lambdas in rotatePixels() allocated a temporary 'unrotated' buffer via 'new uint8_t[n_bytes]' but never freed it. Every JPEG decoded with EXIF orientation 90, 180, or 270 leaked the full pixel buffer (width * height * 4 bytes) — typically several MiB per call. Also corrects a 'new[]' / 'free()' allocator mismatch in decodeJPEGIntoSurface's OOM error path (delete[] is the correct deallocator for arrays allocated with new[]).
loadImage's onerror handler rejects the Promise but leaves image.src pointing at the input buffer. When libjpeg/libpng allocated a cairo surface before failing mid-decode, that surface stays attached to the Image until V8 GC — under sustained load on malformed inputs this delays cleanup arbitrarily. Assign Buffer.alloc(0) before reject so clearData() runs synchronously.
If libjpeg calls error_exit during jpeg_read_scanlines (corrupt or truncated JPEG that decoded the header but fails in the scan), it longjmps back to the setjmp point in loadJPEGFromBuffer. That bypasses the cleanup at the bottom of decodeJPEGIntoSurface, leaking both 'data' (width * height * 4) and 'src' (width * output_components). Store both pointers on the canvas_jpeg_error_mgr so the error handler can delete[] them before longjmp. Also reorder cleanup to destroy libjpeg state and free src before creating the cairo surface, so the success path is symmetric with the new failure path.
canvas_state_t held four cairo_pattern_t* members (fillPattern, strokePattern, fillGradient, strokeGradient) but the destructor only freed fontDescription. Every Context2d that ended life with a non-null pattern leaked the pattern. The copy constructor copied the raw pointers, so save()/restore() shared the same pattern across states without refcounting — making any per-state destroy unsafe. Switch to proper cairo refcount discipline: - canvas_state_t copy ctor + new operator= reference each pattern. - ~canvas_state_t releases all four patterns. - New setFillPattern / setStrokePattern / setFillGradient / setStrokeGradient helpers replace the previous pattern (refcount--) before storing the new one (refcount++). clearFillPattern / clearStrokePattern release both pattern and gradient when switching to a solid color. Also fix two correctness issues exposed by prompt finalization (NAPI_EXPERIMENTAL path from PR Automattic#2562): - _fillStyle and _strokeStyle now use Reset(obj, 1) (strong ref) so the JS gradient/pattern object stays alive while the Context2d's state still references its native cairo_pattern_t. - ~Context2d pops all states before destroying _layout and _context so canvas_state_t destructors run with valid context. - resetState clears all states (not just pop()) and re-points 'state' at the freshly emplaced top.
Bundles four additional fixes on top of v3.2.3-memfix.1: - index.js loadImage clears image.src on error (defensive cleanup of partial cairo surfaces from malformed inputs). - decodeJPEGIntoSurface frees data/src when libjpeg longjmps from jpeg_read_scanlines mid-decode (corrupt scan data). - canvas_state_t takes refcounted ownership of cairo_pattern_t members (fillPattern, strokePattern, fillGradient, strokeGradient) so they don't leak when state is destroyed. - _fillStyle/_strokeStyle hold strong refs to gradient/pattern JS objects so they can't be GC'd while in use under prompt finalization.
…unts Don't call _resetPersistentHandles() from ~Context2d. napi_delete_reference is unsafe inside GC and Node 22 panics with: FATAL ERROR: Finalizer is calling a function that may affect GC state. The finalizers are run directly from GC and must not affect GC state. Persistent handles are already reset from Finalize(Napi::Env), which runs deferred via node_api_post_finalizer when NAPI_EXPERIMENTAL is enabled.
…emfix.3) The nogc / node_api_nogc_finalize approach (Automattic#2562, Automattic#2436) relied on experimental N-API and did not fix the leak in practice. Revert to node-addon-api ^7.0.0 and drop NAPI_EXPERIMENTAL. - Remove the experimental-only Pattern::Finalize override; _source is released by its Napi::Reference destructor under deferred finalization. - Restore _resetPersistentHandles() in ~Context2d (safe again now that the destructor no longer runs inside GC). - Keep Automattic#2582's cairo_pattern_t refcounting and the CanvasPattern _source / strong fillStyle/strokeStyle refs: these guard a real use-after-free of Image create_for_data surfaces (Image::clearData frees the pixel buffer unconditionally), independent of finalizer timing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
test/memory.test.js asserted synchronous freeing (the nogc behavior) via a synchronous gc() loop, so it fails under standard deferred N-API finalization (RSS ~468 MiB). Drain finalizers via setImmediate before measuring and assert RSS < 256 MiB (settles ~110 MiB when freed vs ~400+ MiB if leaked), so it still catches a real cairo-surface leak. Also fix the `test` script: mocha 5.2.0 ignores `--node-option expose-gc`, so gc was never exposed and this test was silently skipped (pending). Invoke mocha via `node --expose-gc` so it actually runs in CI. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Trim the fork to only the memory leaks reproduced with before/after RSS: - JPEG EXIF rotation (rotate90/rotate270) leak (Automattic#2574, merged upstream) - RsvgHandle / partial cairo surface leak on SVG decode error paths (Automattic#2585) Revert everything else back to v3.2.3 after testing: image.src-on-error (Automattic#2580, no measurable effect), libjpeg longjmp data/src free (Automattic#2581, not reproducible on libjpeg-turbo), cairo_pattern_t refcount (Automattic#2582), CanvasPattern source-retention UAF guard (Automattic#2583), rare decoder error-path frees (Automattic#2587), and the node-addon-api 8 / NAPI_EXPERIMENTAL experiment. Built on node-addon-api 7. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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.
Thanks for contributing!