Skip to content

Context2d: keep fillStyle/strokeStyle strong, fix resetState and ~Context2d teardown#2598

Open
iurisilvio wants to merge 1 commit into
Automattic:masterfrom
iurisilvio:fix/context2d-state-correctness
Open

Context2d: keep fillStyle/strokeStyle strong, fix resetState and ~Context2d teardown#2598
iurisilvio wants to merge 1 commit into
Automattic:masterfrom
iurisilvio:fix/context2d-state-correctness

Conversation

@iurisilvio

Copy link
Copy Markdown
Contributor

Context2d state-handling correctness (split out of #2582)

Three small, independent fixes to Context2d state handling. Split from #2582 — which now contains only the cairo_pattern_t refcounting — this PR is the correctness half and does not depend on it.

1. Keep fillStyle / strokeStyle as strong references

_fillStyle / _strokeStyle are weak references (.Reset(obj), refcount 0). For an image-backed CanvasPattern, the cairo pattern's surface points at the Image's pixel buffer (cairo_image_surface_create_for_data), which Image::clearData() frees unconditionally when the Image is GC'd. If the wrapper is collected while the Context2d state still references that pattern, drawing reads freed memory.

.Reset(obj, 1) keeps the wrapper alive while it is the active style. (Pairs with the source retention in #2583, which keeps the Image itself alive for the pattern's lifetime.)

2. resetState() leaves state dangling

void Context2d::resetState() {
  states.pop();      // pops one level only
  states.emplace();  // `state` still points at the popped/old top -> dangling
  ...
}

Fix: pop all states, emplace one, and re-point state = &states.top().

3. ~Context2d() teardown order

Pop states before destroying the cairo context, so each canvas_state_t is torn down while the context is still valid.


Note: this overlaps with #2582 in SetFillStyle / SetStrokeStyle (both touch those lines). Whichever merges first, the other rebases trivially.

…text2d teardown

Three Context2d state-handling correctness fixes, independent of the
canvas_state_t cairo_pattern_t ownership change in Automattic#2582:

- Hold a strong reference to the fillStyle/strokeStyle JS object
  (_fillStyle/_strokeStyle.Reset(obj, 1)). For an image-backed CanvasPattern the
  cairo pattern's surface points at the Image's pixel buffer, which
  Image::clearData() frees when the Image is GC'd; keeping the wrapper alive
  while the state still references its pattern prevents a use-after-free
  (matches the CanvasPattern source retention in Automattic#2583).
- resetState(): pop all states and re-point state = &states.top() after
  emplace(); the original pop()+emplace() left `state` dangling and only
  cleared one level.
- ~Context2d(): pop states before destroying the cairo context.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.

1 participant