Skip to content

fix(🐛): ref the typeface borrowed from SkFont - #4025

Open
dennytosp wants to merge 1 commit into
Shopify:mainfrom
dennytosp:fix/ref-typeface-borrowed-from-font
Open

fix(🐛): ref the typeface borrowed from SkFont#4025
dennytosp wants to merge 1 commit into
Shopify:mainfrom
dennytosp:fix/ref-typeface-borrowed-from-font

Conversation

@dennytosp

@dennytosp dennytosp commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Fixes #3983.

SkFont exposes two accessors, and the one used here is the borrowing one — quoting include/core/SkFont.h on the pinned chrome/m152:

/** Does not alter SkTypeface SkRefCnt.

    @return  non-null SkTypeface
*/
SkTypeface* getTypeface() const;

/** Increases SkTypeface SkRefCnt by one.

    @return  A non-null SkTypeface.
*/
sk_sp<SkTypeface> refTypeface() const;

sk_sp's raw-pointer constructor adopts without ref'ing ("No call to ref() or unref() will be made"), so sk_sp<SkTypeface>(getObject()->getTypeface()) handed the JsiSkTypeface wrapper a reference it never owned. Every font.getTypeface() call from JS therefore cost the typeface one reference the moment its wrapper was garbage collected. Code that derives a paint variant from a loaded font does this per render:

const bold = useMemo(() => {
  const f = Skia.Font(baseFont.getTypeface(), size);
  f.setEmbolden(true);
  return f;
}, [baseFont]);

Once the stolen unrefs cross the typeface's real refcount it is destroyed while live SkFonts still point at it, which is where the crashes in the issue come from — SkTypeface::getBounds under GlyphsCmd::draw, and SkTypeface::textToGlyphs under JsiSkFont::getGlyphIDs.

refTypeface() is the accessor that hands over an owned reference, which is what the wrapper needs. It is also the only site in cpp/ that built an sk_sp from a borrowed getter — I grepped the other sk_sp<Sk…>(…) constructions and they all adopt a freshly new-ed object.

The Web binding was never affected: JsiSkFont.getTypeface there wraps the CanvasKit handle, which is not refcounted from JS.

SkFont has two accessors, and getTypeface() is the borrowing one:

  /** Does not alter SkTypeface SkRefCnt. */ SkTypeface* getTypeface() const;
  /** Increases SkTypeface SkRefCnt by one. */ sk_sp<SkTypeface> refTypeface() const;

sk_sp's raw-pointer constructor adopts without ref'ing, so wrapping getTypeface()
handed the JsiSkTypeface wrapper a reference it never owned: every call cost the
typeface one reference the moment that wrapper was collected. Code that derives
paint variants from a loaded font calls this per render, and once the count
crosses the typeface's real refcount it is freed while live SkFonts still point
at it - crashing in SkTypeface::getBounds during a draw or in textToGlyphs
during measurement.

Fixes Shopify#3983
@dennytosp
dennytosp force-pushed the fix/ref-typeface-borrowed-from-font branch from 676d6bf to fb04383 Compare August 24, 2026 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant