Take ownership of cairo_pattern_t in canvas_state_t with refcounts#2582
Open
iurisilvio wants to merge 1 commit into
Open
Take ownership of cairo_pattern_t in canvas_state_t with refcounts#2582iurisilvio wants to merge 1 commit into
iurisilvio wants to merge 1 commit into
Conversation
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 correctness issues that become visible once finalizers run promptly (NAPI_EXPERIMENTAL path): - _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 a valid context. The destructor intentionally does NOT call _resetPersistentHandles() itself — that invokes napi_delete_reference which is unsafe inside GC. The handles are reset from Finalize(env), which runs deferred via node_api_post_finalizer. - resetState clears all states (not just pop()) and re-points 'state' at the freshly emplaced top; the original pop()+emplace() left 'state' dangling when states was empty. Fixes Automattic#2578
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.
Fixes #2578.
canvas_state_tholds fourcairo_pattern_t*members (fillPattern,strokePattern,fillGradient,strokeGradient) but the destructor only freedfontDescription. Every Context2d that ended life with any non-null pattern leaked it. The copy constructor copied the raw pointers —save()/restore()chains shared the samecairo_pattern_t*across states with no shared ownership.This PR takes proper refcounted ownership:
canvas_state_tcopy ctor + newoperator=cairo_pattern_referenceeach pattern.~canvas_state_tcairo_pattern_destroys all four.setFillPattern/setStrokePattern/setFillGradient/setStrokeGradient/clearFillPattern/clearStrokePatternhandle replacement at the call sites (SetFillStyle/SetStrokeStyle/_setFillColor/_setStrokeColor).Related correctness fixes that become observable when finalizers run promptly (the
NAPI_EXPERIMENTALfinalizer path):_fillStyleand_strokeStyleuseReset(obj, 1)(strong ref) so the JSCanvasGradient/CanvasPatternobject stays alive while the Context2d state still references its nativecairo_pattern_t.~Context2dpops all states before destroying_layout/_contextso~canvas_state_truns with a valid context. The destructor intentionally does not call_resetPersistentHandles()itself —napi_delete_referenceis unsafe inside GC. Handles are reset only fromFinalize(env), which runs deferred vianode_api_post_finalizer.resetStateuses awhile (!states.empty()) states.pop()loop and re-pointsstate = &states.top()afteremplace(); the originalpop()+emplace()leftstatedangling whenstateswas empty.Standalone repro for the leak portion is in #2578 — ~5 KiB/iter sustained on a gradient loop, slope drops to allocator noise after this PR.