Bug 111432 - AddTempDevFont(const OUString& rFontFileURL) leaks memory on invocation
Summary: AddTempDevFont(const OUString& rFontFileURL) leaks memory on invocation
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: graphics stack (show other bugs)
Version:
(earliest affected)
6.0.0.0.alpha0+ Master
Hardware: x86-64 (AMD64) Mac OS X (All)
: medium normal
Assignee: Julien Nabet
QA Contact:
URL:
Whiteboard: target:6.0.0 target:5.4.1
Keywords:
: 99929 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-08-07 10:13 UTC by Alex Thurgood
Modified: 2017-08-29 20:07 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments
Screenshot of Instruments memory leak profile analysis (59.76 KB, image/png)
2017-08-07 10:28 UTC, Alex Thurgood
Details
Ref count screenshot in NSURL (80.51 KB, image/png)
2017-08-07 10:32 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-07 10:13:44 UTC
Description:
Tracing memory leaks in LibreOfficeDev on Mac OSX

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.
8. Stop recording.
9. Analyse the profile trace.

Actual Results:  
The app leaks memory each time AddTempDevFont(const OUString& rFontFileURL) is invoked

Expected Results:
There should be no memory leaks.


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-07 10:18:41 UTC
In this particular instance the code invoked is in salgdi.cxx, with the leaks occuring here :

    CFStringRef rFontPath = CFStringCreateWithCString(nullptr, aCFileName.getStr(), kCFStringEncodingUTF8); - 101 byte leak


    CFURLRef rFontURL = CFURLCreateWithFileSystemPath(nullptr, rFontPath, kCFURLPOSIXPathStyle, true); - 197 byte leak
Comment 2 Alex Thurgood 2017-08-07 10:19:04 UTC
@khaled : thought you might be interested
Comment 3 Alex Thurgood 2017-08-07 10:22:22 UTC
Similarly, at line 321 in salgdi.cxx :

AddTempDevFont(aFileStatus.getFileURL());

a leak of 288 bytes
Comment 4 Alex Thurgood 2017-08-07 10:24:08 UTC
On startup, there are 101 occurrences of this on Mac OS. I assume that this corresponds to the number of fonts on the system ?
Comment 5 Alex Thurgood 2017-08-07 10:28:50 UTC
Created attachment 135205 [details]
Screenshot of Instruments memory leak profile analysis
Comment 6 Alex Thurgood 2017-08-07 10:31:30 UTC
In NSURL, similar leaks are occurring, cf. enclosed screenshot of refcounts.
Comment 7 Alex Thurgood 2017-08-07 10:32:02 UTC
Created attachment 135206 [details]
Ref count screenshot in NSURL
Comment 8 Alex Thurgood 2017-08-07 10:40:13 UTC
Similarly, the call to 

            CTFontDescriptorRef pDesc = CTFontCopyFontDescriptor(pFallback);

at line 80 of salgdi.cxx in the CFDictionary object.
Comment 9 Telesto 2017-08-07 12:43:58 UTC
Confirming. See the attachment link posted here: bug 99784 comment 86
Comment 10 Julien Nabet 2017-08-07 13:18:45 UTC
https://opengrok.libreoffice.org/xref/core/vcl/quartz/salgdi.cxx#288

288  static bool AddTempDevFont(const OUString& rFontFileURL)
289  {
290      OUString aUSytemPath;
291      OSL_VERIFY( !osl::FileBase::getSystemPathFromFileURL( rFontFileURL, aUSytemPath ) );
292      OString aCFileName = OUStringToOString( aUSytemPath, RTL_TEXTENCODING_UTF8 );
293  
294      CFStringRef rFontPath = CFStringCreateWithCString(nullptr, aCFileName.getStr(), kCFStringEncodingUTF8);
295      CFURLRef rFontURL = CFURLCreateWithFileSystemPath(nullptr, rFontPath, kCFURLPOSIXPathStyle, true);
296  
297      CFErrorRef error;
298      bool success = CTFontManagerRegisterFontsForURL(rFontURL, kCTFontManagerScopeProcess, &error);
299      if (!success)
300      {
301          CFRelease(error);
302      }
303  
304      return success;
305  }

rFontPath and rFontURL  should be released with this just before the return:
CFRelease( rFontPath );
CFRelease( rFontURL  )
I'll take this one.

Alex: if you build from sources, could you give it a try? (1: does it build, 2: does it help for memory consumption)
Indeed, I won't be able to test before getting back to home after my daytime job.

BTW, great tool! :-)
Comment 11 Julien Nabet 2017-08-07 17:32:39 UTC
I give it a try with https://gerrit.libreoffice.org/#/c/40845/
Comment 12 Commit Notification 2017-08-07 17:47:10 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

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

tdf#111432: fix leaks in AddTempDevFont 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 13 Julien Nabet 2017-08-07 18:07:55 UTC
Alex: https://tinderbox.libreoffice.org/cgi-bin/gunzip.cgi?tree=MASTER&full-log=1502128359.24387 shows it builds ok.
(my Mac is building right now from scratch so couldn't tell)
I cherry-pick the commit in 5.4 for review: see https://gerrit.libreoffice.org/#/c/40847/

I don't think a backport in 5.3 branch worths it since 5.3.5 has just been released and there's only 1 version left which will be for major bugs.

When you'll build or get a daily build including this commit, could you provide some feedback so we're if it fixes it or not?
Comment 14 Alex Thurgood 2017-08-08 07:03:17 UTC
(In reply to Julien Nabet from comment #13)

> 
> I don't think a backport in 5.3 branch worths it since 5.3.5 has just been
> released and there's only 1 version left which will be for major bugs.
> 
> When you'll build or get a daily build including this commit, could you
> provide some feedback so we're if it fixes it or not?

Hi Julien, sorry, I was busy with other stuff yesterday. If the change is in master already, I can pull and rebuild on my machine and run the tests again.
Comment 15 Julien Nabet 2017-08-08 07:57:22 UTC
(In reply to Alex Thurgood from comment #14)
>...
> Hi Julien, sorry, I was busy with other stuff yesterday. If the change is in
> master already, I can pull and rebuild on my machine and run the tests again.

No problem! :-)
Hopefully you can pull and rebuild because there's no daily builds (see http://nabble.documentfoundation.org/MacOs-Daily-master-builds-lack-of-MacOs-Jenkins-td4220058.html)
Comment 16 Alex Thurgood 2017-08-08 13:05:21 UTC
(In reply to Julien Nabet from comment #15)
> (In reply to Alex Thurgood from comment #14)
> >...
> > Hi Julien, sorry, I was busy with other stuff yesterday. If the change is in
> > master already, I can pull and rebuild on my machine and run the tests again.
> 
> No problem! :-)
> Hopefully you can pull and rebuild because there's no daily builds (see
> http://nabble.documentfoundation.org/MacOs-Daily-master-builds-lack-of-MacOs-
> Jenkins-td4220058.html)

Hi Julien,

Yes, your fix seems to have done the trick. Those leaks are no longer apparent.



However, I notice that at line 80, salgdi.xx :

            CTFontDescriptorRef pDesc = CTFontCopyFontDescriptor(pFallback);

is still leaking big time - perhaps this suffers from the same problem, or should I open a separate bug report ?

This happens when you open and scroll down the list of fonts in the dropdown font selector Writer:
 


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 17 Alex Thurgood 2017-08-08 13:10:02 UTC
(In reply to Alex Thurgood from comment #16)

This happens when CoreTextGlyphFallbackSubstitution::FindFontSubstitute() is invoked
Comment 18 Alex Thurgood 2017-08-08 13:12:56 UTC
I'll close this one as fixed and open a separate report for the other other font management leaks.
Comment 19 Julien Nabet 2017-08-08 13:14:24 UTC
Thank you Alex for your feedback.
I'll take a look but I suppose it's the exact same cause.
I'll put it in See also since it's the same method.
Comment 20 Commit Notification 2017-08-10 13:04:59 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=ec60beaf9e73beccf348726d0be4e28f35abe38a&h=libreoffice-5-4

tdf#111432: fix leaks in AddTempDevFont 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.
Comment 21 Julien Nabet 2017-08-29 20:07:06 UTC
*** Bug 99929 has been marked as a duplicate of this bug. ***