fix(canvas): release a canvas' surface reference on its own thread - #4055
Open
mlecoq wants to merge 1 commit into
Open
fix(canvas): release a canvas' surface reference on its own thread#4055mlecoq wants to merge 1 commit into
mlecoq wants to merge 1 commit into
Conversation
mlecoq
force-pushed
the
fix/canvas-surface-release-thread
branch
from
September 9, 2026 13:50
5393754 to
11b6a57
Compare
`JsiSkSurface::getCanvas()` gives the `JsiSkCanvas` a strong reference to the
surface (`setSurface`, needed for Graphite readback), a new wrapper is created
on every call, and `JsiSkCanvas` exposes no `dispose()`. So the only way that
reference goes away is garbage collection — and with Hermes' concurrent GC the
finalizer can run on the GC thread.
When it holds the last reference (the JS `SkSurface` wrapper was disposed, or
collected first), the `SkSurface` is then destroyed off-thread. GrGpuResource
refcounting and the GrResourceCache arrays are not thread safe, so the owning
`GrDirectContext` is left inconsistent and a later flush aborts, e.g.
GrResourceCache::removeResource
GrGpuResource::release
GrResourceCache::notifyARefCntReachedZero
GrTextureProxy::~GrTextureProxy
...
GrDirectContext::flushAndSubmit
RNSkia::JsiSkSurface::flush
which is what `JsiSkImage`, `JsiSkSurface` and `JsiSkPicture` already guard
against by handing their object to the dispatcher of the thread they were
created on. Do the same for the surface a canvas keeps alive.
It also makes `surface.dispose()` effective: today it drops the wrapper's
reference while a canvas wrapper still holds one, so the render target is only
freed at the next GC.
mlecoq
force-pushed
the
fix/canvas-surface-release-thread
branch
from
September 9, 2026 14:00
11b6a57 to
0649161
Compare
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
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.
Description
JsiSkSurface::getCanvas()hands the newJsiSkCanvasa strong reference tothe surface (
setSurface, needed for the GraphitereadPixelsfallback). Afresh wrapper is created on every call, and
JsiSkCanvas— unlikeJsiSkImage/JsiSkSurface/JsiSkPicture— is a plainJsiSkNativeObject: it exposes nodispose()and has no destructor. The onlything that ever drops that reference is garbage collection, and with Hermes'
concurrent GC the finalizer can run on the GC thread.
So when the canvas wrapper happens to hold the last reference — the JS
SkSurfacewrapper was disposed, or was collected first — theSkSurfaceisdestroyed on the GC thread.
GrGpuResourcerefcounting and theGrResourceCachearrays are not thread safe, so the owningGrDirectContextis left inconsistent, and it is a later flush on the owning thread that
aborts:
This is exactly the hazard the other wrappers already guard against, in their
own words:
It shows up in apps that draw into an offscreen surface from a worklet
(
Skia.Surface.MakeOffscreen→getCanvas()→flush()on every frame): thesurface is created on one thread, the wrappers pile up, and their release ends
up on the GC thread. It also means
surface.dispose()is not effective today:it drops the wrapper's reference while a canvas wrapper still holds one, so
the render target is only freed at the next GC.
Changes
JsiSkCanvasnow keeps the dispatcher of the thread it was given a surface onand, in its destructor, hands the surface reference back to it — the same
mechanism
JsiSkImage,JsiSkSurfaceandJsiSkPictureuse.setSurfacealso drains that thread's queue, like the
JsiSkImageconstructor does, sodeferred releases don't pile up.