fix(🐛): ref the typeface returned by Font.getTypeface() - #4001
Open
giaBaoJS wants to merge 2 commits into
Open
Conversation
SkFont::getTypeface() returns a borrowed pointer ("Does not alter
SkTypeface SkRefCnt"), but sk_sp's raw-pointer constructor adopts it
without calling ref(). JsiSkTypeface then owns that sk_sp and unrefs it
when the JS wrapper is disposed or collected, so every getTypeface()
call from JS costs the typeface one net reference. Once the count
crosses the real refcount the typeface is freed while live SkFonts still
point at it, crashing the next draw or measurement in
SkTypeface::getBounds() / textToGlyphs().
Use sk_ref_sp, Skia's idiom for sharing a borrowed pointer.
Fixes Shopify#3983
Contributor
Author
|
I have signed the CLA! |
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.
Fixes #3983
The bug
JsiSkFont::getTypeface()wraps the pointer returned bySkFont::getTypeface()in an owningsk_sp<SkTypeface>without ref'ing it:Two Skia contracts collide there. Both quoted from the Skia this repo currently pins —
externals/skiaat2a9b593bab4b2fd019fa494c8d401ff1fab0b883(chrome/m152):SkFont::getTypeface()lends.include/core/SkFont.h:208-215— "Does not alter SkTypeface SkRefCnt",return fTypeface.get();.sk_sp's raw-pointer constructor adopts.include/core/SkRefCnt.h:246-250— "Adopt the bare pointer into the newly created sk_sp. No call to ref() or unref() will be made." Its destructor then callsSkSafeUnref(SkRefCnt.h:255-258).JsiSkTypefacemoves thatsk_spintoJsiSkWrappingSkPtrNativeObject<JsiSkTypeface, SkTypeface>, which holds it as an owning member and releases it ondispose()or GC. So eachfont.getTypeface()call from JS ends up costing the typeface one net reference — a reference the wrapper never acquired.Once that drain crosses the typeface's real refcount, the typeface is destroyed while live
SkFonts still point at it, and the next draw or measurement dereferences freed memory. That is theSkTypeface::getBounds()/SkTypeface::textToGlyphs()crash shape reported in #3983.The fix
sk_ref_spis Skia's idiom for taking shared ownership of a borrowed pointer —SkRefCnt.h:383-385,sk_sp<T>(SkSafeRef(obj)).Null behaviour is unchanged:
SkSafeRef(nullptr)returnsnullptr, so a null typeface still produces a wrapper holding null, exactly as today. (On m152SkFont::getTypeface()is documented andSkASSERTed non-null anyway.)This is the only site of its kind
Widening to every explicit
sk_sp<T>(...)construction underpackages/skia/cpp/returns three hits; the other two aresk_sp<SkottieAssetProvider>(new SkottieAssetProvider(...))andsk_sp<SVGAssetProvider>(new SVGAssetProvider(...)), which correctly adopt a freshly allocated object. So this is an isolated slip, not a pattern, and the change stays a one-liner.Verification
1. Runtime experiment against the real prebuilt Skia. I linked a small harness against the m152 binary this repo consumes (
node_modules/react-native-skia-apple-macos/.../libskia.a), reproducing what the JS path does: create a typeface, build anSkFontfrom it, then create and destroy one wrapper that owns thesk_spthe wayJsiSkTypefacedoes. It reads the refcount withSkRefCntBase::unique()only, so it never touches freed memory and the result is deterministic rather than UB:A variant that runs the wrapper create/destroy cycle twice and then keeps using the font aborts (
SIGABRT) on the current code and exits cleanly with the fix.2. Compilation.
packages/skia/cpp/api/JsiSkFont.hcompiles clean on its own (clang++ -std=c++20 -fsyntax-only) against the m152 headers produced byyarn install-skia, and the iOS example app builds end to end with the change:What I did not run: the device E2E suite (
E2E=true yarn test -i e2e). I stopped at the app build rather than driving the Tests screen on a simulator, so please treat CI as the authority there. Nothing in the existing suite exercises this path anyway (Font.spec.tsruns against CanvasKit, not the native binding).On a regression test: I did not add one, because the stolen reference is not observable from JS without provoking the use-after-free itself — any JS-level test for this would have to crash the app (and take the E2E harness down with it) rather than fail. The C++ probe above is the non-UB way to assert it. Happy to add either an E2E test or a native check if you would like one.