ref counting issue with typeface used in MinikinFontSkia

[psharma]

Hi, 

It looks like there is some issue with ref counting with MinikinFontSkia code.

/frameworks/base/libs/hwui/hwui/MinikinSkia.cpp. Constructor doesn't increase the ref count but reduce the ref count for typeface. I believe mTypeface(typeface) should be mTypeface(SkRef(typeface)) 

MinikinFontSkia::MinikinFontSkia(SkTypeface* typeface, const void* fontData, size_t fontSize,
        int ttcIndex) :
    MinikinFont(typeface->uniqueID()), mTypeface(typeface), mFontData(fontData),

    mFontSize(fontSize), mTtcIndex(ttcIndex) {

}

MinikinFontSkia::~MinikinFontSkia() {
   SkSafeUnref(mTypeface);
}

Regards,

psharma

[Prashant Sharma]

looks like no one has faced any issue with this. The question is if constructor is not increasing the ref count than why destructor is reducing the ref count of  typeface ? any idea if we can discuss this with any of google guys ?

--
You received this message because you are subscribed to the Google Groups "android-platform" group.
To unsubscribe from this group and stop receiving emails from it, send an email to android-platform+unsubscribe@googlegroups.com.
To post to this group, send email to android-platform@googlegroups.com.
Visit this group at https://groups.google.com/group/android-platform.
For more options, visit https://groups.google.com/d/optout.

[Colin Cross]

From the internal owners of MinikinFontSkia:
I agree with about reporter about that we should increment ref count
in ctor and decrement dtor, and caller should decrement the ref count
once MinikinFontSkia acquires the ownership. However, as far as I read
the implementation in Lollipop, the passed SkTypeface is only owned by
the MinikinFontSkia, and caller don't decrements ref count. Thus, the
ref count of passed SkTypeface should be always 1 until
MinikinFontSkia destruction happens. So no crash or leak should
happen.

>> email to android-platfo...@googlegroups.com.
>> To post to this group, send email to android-...@googlegroups.com.

>> Visit this group at https://groups.google.com/group/android-platform.
>> For more options, visit https://groups.google.com/d/optout.
>
>

> --
> You received this message because you are subscribed to the Google Groups
> "android-platform" group.
> To unsubscribe from this group and stop receiving emails from it, send an

> email to android-platfo...@googlegroups.com.
> To post to this group, send email to android-...@googlegroups.com.

[psharma]

Thanks Colin for the reply. I thought similar as well but it looks like is there is a another connection as well. below stack trace shows how Typeface destructor could be called which was created in 

FontFamily_addFont .

-------------------------------------------------

~SkTypeface_AndroidStream
SkRefCntBase::unref() const
~SkScalerContext_FreeType
~SkGlyphCache
SkGlyphCache_Globals::internalPurge(unsigned long)
SkGlyphCache_Globals::attachCacheToHead(SkGlyphCache*)
SkFunctionWrapper<void, SkGlyphCache, &SkGlyphCache::AttachCache>::operator()(SkGlyphCache*)
SkCanvas::onDrawPosText(void const*, unsigned long, SkPoint const*, SkPaint const&)
android::SkiaCanvas::drawGlyphs(unsigned short const*, float const*, int, SkPaint const&, float, float, float, float, float, float, float)
void android::MinikinUtils::forFontRun<android::DrawTextFunctor>(android::Layout const&, android::Paint*, android::DrawTextFunctor&)
android::Canvas::drawText(unsigned short const*, int, int, int, float, float, int, android::Paint const&, android::Typeface*)
drawTextString - android_graphics_Canvas.cpp

--------------------------------------------------------

clearly SkScalerContext also contains the same typeface and typeface destructor could be invoke from destructor of SkScalerContext as we have not increased it's ref count when assigning to Minikinfontskia

------------------------------

callstack for SkScalerContext creation which contains the same typeface 

SkScalerContext
SkScalerContext_FreeType_Base
SkTypeface_FreeType::onCreateScalerContext(SkDescriptor const*) const
SkTypeface::createScalerContext(SkDescriptor const*, bool) const
SkGlyphCache::VisitCache(SkTypeface*, SkDescriptor const*, bool (*)(SkGlyphCache const*, void*), void*)
SkGlyphCache::DetachCache(SkTypeface*, SkDescriptor const*)
SkPaint::descriptorProc(SkSurfaceProps const*, SkPaint::FakeGamma, SkMatrix const*, void (*)(SkTypeface*, SkDescriptor const*, void*), void*) const
SkPaint::detachCache(SkSurfaceProps const*, SkPaint::FakeGamma, SkMatrix const*) const
android::MinikinFontSkia::GetHorizontalAdvance(unsigned int, android::MinikinPaint const&) const

---------------------------

[psharma]

If it helps I found the exact place where typeface from MinikinFontSkia is assigned to another class !! so MinikinFontSkia is not the only class which owns this typeface 

check /frameworks/base/libs/hwui/hwui/MinikinSkia.cpp

void MinikinFontSkia::populateSkPaint(SkPaint* paint, const MinikinFont* font, FontFakery fakery) {

    paint->setTypeface(reinterpret_cast<const MinikinFontSkia*>(font)->GetSkTypeface()); // !!!!!!!!!! 

    paint->setFakeBoldText(paint->isFakeBoldText() || fakery.isFakeBold());

    if (fakery.isFakeItalic()) {

        paint->setTextSkewX(paint->getTextSkewX() - 0.25f);

    }

}

Same typeface is assigned to SkPaint than to SkScalarContext, and in some random scenario it may cause problem. So I think the fix which I shared before is needed( replace mTypeface(typeface) with  mTypeface(SkRef(typeface))  !!!!!