1672899 - NativeFontResourceMac font data is leaked

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Markus Stange [:mstange]]

Attachment: testcase

It looks like the CFData we create in NativeFontResourceMac does not get freed when we release the CGFontRef, even after bug 1672088.

Comment 1

[Markus Stange [:mstange]]

Attachment: debugging patch

With this patch applied, loading the testcase and then closing the tab should print ~FontData but it does not.

Comment 2

[Markus Stange [:mstange]]

Adding a return; at the beginning of DrawTargetSkia::DrawGlyphs fixes the leak. DrawGlyphs calls CTFontCreateWithGraphicsFont at the following stack:

    frame #7: 0x00007fff21a32939 CoreText`CTFontCreateWithGraphicsFont + 70
    frame #8: 0x0000000105da8113 XUL`ctfont_create_exact_copy(baseFont=<unavailable>, textSize=2048, transform=<unavailable>) at SkFontHost_mac.cpp:1087:13 [opt]
    frame #9: 0x0000000105da7f67 XUL`SkScalerContext_Mac::SkScalerContext_Mac(this=0x00000001223d4000, typeface=<unavailable>, effects=<unavailable>, desc=<unavailable>) at SkFontHost_mac.cpp:1120:15 [opt]
    frame #10: 0x0000000105dabae2 XUL`SkTypeface_Mac::onCreateScalerContext(SkScalerContextEffects const&, SkDescriptor const*) const [inlined] SkScalerContext_Mac::SkScalerContext_Mac(this=0x00000001223d4000, typeface=sk_sp<SkTypeface_Mac> @ 0x00007000054dd960, effects=0x00007000054dda40, desc=0x00007000054dda80) at SkFontHost_mac.cpp:1096:1 [opt]
    frame #11: 0x0000000105dabad4 XUL`SkTypeface_Mac::onCreateScalerContext(this=0x000000011f87c100, effects=0x00007000054dda40, desc=0x00007000054dda80) const at SkFontHost_mac.cpp:2378 [opt]
    frame #12: 0x0000000105e8412c XUL`SkTypeface::createScalerContext(this=0x000000011f87c100, effects=0x00007000054dda40, desc=0x00007000054dda80, allowFailure=<unavailable>) const at SkScalerContext.cpp:904:46 [opt]
    frame #13: 0x0000000105eaaa29 XUL`SkTypeface::onComputeBounds(this=0x000000011f87c100, bounds=0x000000011f87c118) const at SkTypeface.cpp:405:50 [opt]
    frame #14: 0x0000000105eaa8af XUL`SkTypeface::getBounds() const [inlined] SkTypeface::getBounds() const::$_3::operator()() const at SkTypeface.cpp:378:20 [opt]
    frame #15: 0x0000000105eaa89c XUL`SkTypeface::getBounds() const [inlined] void SkOnce::operator(this=<unavailable>, fn=<unavailable>)<SkTypeface::getBounds() const::$_3>(SkTypeface::getBounds() const::$_3&&) at SkOnce.h:36 [opt]
    frame #16: 0x0000000105eaa88a XUL`SkTypeface::getBounds(this=0x000000011f87c100) const at SkTypeface.cpp:377 [opt]
    frame #17: 0x0000000105e4ae9b XUL`SkFontPriv::GetFontBounds(font=0x000000011f87cb40) at SkFont.cpp:400:34 [opt]
    frame #18: 0x0000000105ea9157 XUL`SkTextBlobBuilder::ConservativeRunBounds(run=0x000000011f87cb40) at SkTextBlob.cpp:307:31 [opt]
    frame #19: 0x0000000105ea8cf2 XUL`SkTextBlobBuilder::make() [inlined] SkTextBlobBuilder::updateDeferredBounds(this=0x00007000054ddd08) at SkTextBlob.cpp:374:47 [opt]
    frame #20: 0x0000000105ea8cd5 XUL`SkTextBlobBuilder::make(this=0x00007000054ddd08) at SkTextBlob.cpp:605 [opt]
    frame #21: 0x0000000103187201 XUL`mozilla::gfx::DrawTargetSkia::DrawGlyphs(this=<unavailable>, aFont=<unavailable>, aBuffer=0x00007000054dde20, aPattern=0x00007000054dde30, aStrokeOptions=<unavailable>, aOptions=<unavailable>) at DrawTargetSkia.cpp:1359:38 [opt]

jrmuizel looked at the upstream Skia code and found that it no longer calls that function.

Comment 3

[Markus Stange [:mstange]]

Skia has encountered these same leaks: https://bugs.chromium.org/p/skia/issues/detail?id=9627

It's probably worth picking up these fixes from Skia. If we want to cherry-pick the relevant patches, here's a list:
https://github.com/google/skia/commits/b5d977fae3b75ad3200c9492acdd55f710ce9450/src/ports/SkFontHost_mac.cpp

I think we want everything from October 2019 to February 2020.

Comment 4

[Markus Stange [:mstange]]

Lee, let me know how you would like to handle this. Not sure if I'm the right person to do the work here, but I could be convinced. :)

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Attachment: Bug 1672899. Avoid CGFonts in ctfont_create_exact_copy when possible.

This avoids the leaks caused by going through a CGFont. I'm
not sure if it breaks something else.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=966554ed3f1ded200a4520722df73d9eb67d719f

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14e261584639
Avoid CGFonts in ctfont_create_exact_copy when possible. r=jfkthame

Comment 8

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/14e261584639

Comment 9

[Jeff Muizelaar [:jrmuizel]]

So it looks like the change commited wasn't sufficient to make sure the data gets freed. The CGFont is referenced by something else that ends up in a cache (TSingleAttrDescriptorCache perhaps?) when CTFontCopyFontDescriptor is called. It's possible that this is a fixed size cache and so the problem won't get as bad. To confirm that we should add a memory reporter for this font data.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

I tried eliminating calls to CTFontCopyFontDescriptor. The explicit calls were easy to remove however CTFontCreateCopyWithAttributes calls CTFontCopyFontDescriptor and that's a harder to deal with.