Skip to content

fix(canvas): release a canvas' surface reference on its own thread - #4055

Open
mlecoq wants to merge 1 commit into
Shopify:mainfrom
mlecoq:fix/canvas-surface-release-thread
Open

fix(canvas): release a canvas' surface reference on its own thread#4055
mlecoq wants to merge 1 commit into
Shopify:mainfrom
mlecoq:fix/canvas-surface-release-thread

Conversation

@mlecoq

@mlecoq mlecoq commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Description

JsiSkSurface::getCanvas() hands the new JsiSkCanvas a strong reference to
the surface (setSurface, needed for the Graphite readPixels fallback). A
fresh wrapper is created on every call, and JsiSkCanvas — unlike
JsiSkImage / JsiSkSurface / JsiSkPicture — is a plain
JsiSkNativeObject: it exposes no dispose() and has no destructor. The only
thing 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
SkSurface wrapper was disposed, or was collected first — the SkSurface is
destroyed on the GC thread. GrGpuResource refcounting and the
GrResourceCache arrays are not thread safe, so the owning GrDirectContext
is left inconsistent, and it is a later flush on the owning thread that
aborts:

GrResourceCache::removeResource
GrGpuResource::release
GrResourceCache::notifyARefCntReachedZero
GrTextureProxy::~GrTextureProxy
GrTextureEffect::~GrTextureEffect
...
GrPipeline::~GrPipeline
GrOpFlushState::reset
GrDrawingManager::executeRenderTasks
GrDirectContext::flushAndSubmit
RNSkia::JsiSkSurface::flush          (JsiSkSurface.h:101)

This is exactly the hazard the other wrappers already guard against, in their
own words:

// This JSI Object is being deleted from a GC, which might happen
// on a separate Thread. GPU resources (like SkImage) must be deleted
// on the same Thread they were created on, so in this case we schedule
// deletion to run on the Thread this Object was created on.

It shows up in apps that draw into an offscreen surface from a worklet
(Skia.Surface.MakeOffscreengetCanvas()flush() on every frame): the
surface 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

JsiSkCanvas now keeps the dispatcher of the thread it was given a surface on
and, in its destructor, hands the surface reference back to it — the same
mechanism JsiSkImage, JsiSkSurface and JsiSkPicture use. setSurface
also drains that thread's queue, like the JsiSkImage constructor does, so
deferred releases don't pile up.

@mlecoq
mlecoq force-pushed the fix/canvas-surface-release-thread branch from 5393754 to 11b6a57 Compare September 9, 2026 13:50
`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
mlecoq force-pushed the fix/canvas-surface-release-thread branch from 11b6a57 to 0649161 Compare September 9, 2026 14:00
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