Bug 111497 - CoreTextGlyphFallbackSubstitution::FindFontSubstitute() leaks memory on invocation
Summary: CoreTextGlyphFallbackSubstitution::FindFontSubstitute() leaks memory on invoc...
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: graphics stack (show other bugs)
Version:
(earliest affected)
6.0.0.0.alpha0+
Hardware: x86-64 (AMD64) macOS (All)
: medium normal
Assignee: Julien Nabet
URL:
Whiteboard: target:6.0.0 target:5.4.1
Keywords:
Depends on:
Blocks:
 
Reported: 2017-08-08 13:20 UTC by Alex Thurgood
Modified: 2017-08-11 12:29 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments
Screenshot of memory leak profiling with Instruments.app (329.73 KB, image/png)
2017-08-08 13:21 UTC, Alex Thurgood
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Thurgood 2017-08-08 13:20:12 UTC
Description:
CoreTextGlyphFallbackSubstitution::FindFontSubstitute() leaks memory on invocation.

Steps to Reproduce:
Steps to Reproduce:
1. Start XCode, then Instruments.app
2. Choose Memory Leak profile tool
3. Select LibreOffice.app in instdir as target process
4. Click on the record button, LODev is started by the profiling tool
5. Wait for the StartCenter to load - note the occurrences of memory leaks as they occur.
6. Open a new Writer document.
7. When the document has loaded, click on the dropdown font list. Scroll up and down the list with the Up/Down cursor arrows
8. Stop recording.
9. Analyse the profile trace.


Actual Results:  
CoreTextGlyphFallbackSubstitution::FindFontSubstitute() leaks memory on invocation.

Invoked multiple times in CFData, CFDictionary, and multiple MALLOCS of 336 bytes each

Expected Results:
It shouldn't leak memory.


Reproducible: Always

User Profile Reset: No

Additional Info:


User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:54.0) Gecko/20100101 Firefox/54.0
Comment 1 Alex Thurgood 2017-08-08 13:21:26 UTC
Created attachment 135286 [details]
Screenshot of memory leak profiling with Instruments.app
Comment 2 Alex Thurgood 2017-08-08 13:22:33 UTC
The leak appears to occur here :

line 80, salgdi.xx :

            CTFontDescriptorRef pDesc = CTFontCopyFontDescriptor(pFallback);

from this block :
 


bool bFound = false;
    CoreTextStyle rStyle(rPattern);
    CTFontRef pFont = static_cast<CTFontRef>(CFDictionaryGetValue(rStyle.GetStyleDict(), kCTFontAttributeName));
    CFStringRef pStr = CreateCFString(rMissingChars);
    if (pStr)
    {
        CTFontRef pFallback = CTFontCreateForString(pFont, pStr, CFRangeMake(0, CFStringGetLength(pStr)));
        if (pFallback)
        {
            bFound = true;

            CTFontDescriptorRef pDesc = CTFontCopyFontDescriptor(pFallback);
            FontAttributes rAttr = DevFontFromCTFontDescriptor(pDesc, nullptr);

            rPattern.maSearchName = rAttr.GetFamilyName();

            rPattern.SetWeight(rAttr.GetWeight());
            rPattern.SetItalic(rAttr.GetItalic());
            rPattern.SetPitch(rAttr.GetPitch());
            rPattern.SetWidthType(rAttr.GetWidthType());

            SalData* pSalData = GetSalData();
            if (pSalData->mpFontList)
                rPattern.mpFontData = pSalData->mpFontList->GetFontDataFromId(reinterpret_cast<sal_IntPtr>(pDesc));

            CFRelease(pFallback);
        }
        CFRelease(pStr);
    }
Comment 3 Julien Nabet 2017-08-08 13:23:04 UTC
I'll take it a look but if you've got time, you can give it a try by adding this line in vcl/quartz/salgdi.cxx:
CFRelease(pDesc);
after this line:
CFRelease(pFallback);
Comment 4 Commit Notification 2017-08-08 16:59:12 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=986cd454b2a39ac380b137148f944c0d5ead2631

tdf#111497: fix leak in FindFontSubstitute with CFRelease

It will be available in 6.0.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 5 Julien Nabet 2017-08-08 17:01:32 UTC
For 5.4, there's https://gerrit.libreoffice.org/#/c/40896/ waiting for review.
Comment 6 Alex Thurgood 2017-08-09 14:34:12 UTC
Confirmed fixed in

Version: 6.0.0.0.alpha0+
Build ID: 9f2a105aa1467224b662980f6d1c4a42e5d8bdfe
CPU threads: 4; OS: Mac OS X 10.12.6; UI render: default; 
Locale: fr-FR (fr_FR.UTF-8); Calc: group

Nice work Julien !
Comment 7 Julien Nabet 2017-08-09 14:56:58 UTC
(In reply to Alex Thurgood from comment #6)
> ...
> Nice work Julien !

He! You and your analysis tool made 99% of the work! :-)

If you find other things like this, don't hesitate to put me in cc.
Comment 8 Commit Notification 2017-08-11 12:29:57 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "libreoffice-5-4":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=1756d2145e7ab70f5bb32a8343f5b1e4869bd0ee&h=libreoffice-5-4

tdf#111497: fix leak in FindFontSubstitute with CFRelease

It will be available in 5.4.1.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.