Bug 111634 - Multiple memory leaks in libmacabdrv1
Summary: Multiple memory leaks in libmacabdrv1
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Base (show other bugs)
Version:
(earliest affected)
6.0.0.0.alpha0+ Master
Hardware: x86-64 (AMD64) Mac OS X (All)
: medium normal
Assignee: Not Assigned
QA Contact:
URL:
Whiteboard: target:6.0.0 target:5.4.2
Keywords:
Depends on:
Blocks:
 
Reported: 2017-08-10 09:53 UTC by Alex Thurgood
Modified: 2017-08-22 06:05 UTC (History)
7 users (show)

See Also:
Crash report or crash signature:


Attachments
Summary of memory leak profile (24.76 KB, text/plain)
2017-08-10 09:56 UTC, Alex Thurgood
Details
Screenshot of refcounts in NSDate associated with MacAb (92.24 KB, image/png)
2017-08-10 09:59 UTC, Alex Thurgood
Details
Memory leak profile after recent commits (14.36 KB, text/plain)
2017-08-16 14:06 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-10 09:53:14 UTC
Description:
Analysing memory leaks with Instruments.app on MacOS and my LibreOffice master build indicates that there are multiple (literally 1000's) invocations of code for mallocs which appear to be leaked. This might be the reason why accessing the MacAb is so slow...

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.
6. Load a previously created MacAb.odb file
7. Click on Tables.
8. Double-click on the MacAb table to open it.
9. Close the table.
10. Close the ODB file.
9. Stop recording.
10. Analyse the profile trace.

Actual Results:  
Multiple instances of memory leaks.

Expected Results:
It shouldn't leak memory.


Reproducible: Always

User Profile Reset: No

Additional Info:
The major leaks occur in :

macabrecord.cxx lines 68-80 (at least 10,072 iterations) :

void MacabRecord::insertAtColumn (CFTypeRef _value, ABPropertyType _type, const sal_Int32 _column)
{
    if(_column < size)
    {
        if(fields[_column] == nullptr)
            fields[_column] = new macabfield;

        fields[_column]->value = _value;
        if (fields[_column]->value)
            CFRetain(fields[_column]->value);
        fields[_column]->type = _type;
    }
}

with :

fields[_column] = new macabfield;

being singled out by the tool as the problematic instruction


Other "leaky" points :

lines 546-563 of macabrecords.cxx (3175 iterations)

and in particular line 560 :             headerNames[0] = new macabfield;


lines 116-133 of macabrecords.cxx (multiple instances of ca. hundreds/thousands of iterations)

and in particular line 124 :         macabfield *_copy = new macabfield;


lines 857-892 of macabrecords.cxx
in particular 

line 860 :   MacabRecord *macabRecord = new MacabRecord(_header->getSize());


line 880 :   propertyValue = ABRecordCopyValue(_abrecord,propertyName);
 

line 885 :  insertPropertyIntoMacabRecord(propertyType, macabRecord, _header, propertyNameString, propertyValue);


line 71 in macabutilities :
CFStringRef ref = CFStringCreateWithCharacters(kCFAllocatorDefault, reinterpret_cast<UniChar const *>(aString.getStr()), aString.getLength());




 



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-10 09:56:37 UTC
Created attachment 135408 [details]
Summary of memory leak profile

This is a summary of the memory leak profile produced
Comment 2 Alex Thurgood 2017-08-10 09:59:51 UTC
Created attachment 135409 [details]
Screenshot of refcounts in NSDate associated with MacAb

A screenshot of refcounts in _NSDate (an Apple core foundation function) linked to  MacAb connectivity
Comment 3 Julien Nabet 2017-08-10 10:08:46 UTC
I'll take a look too, specifically the calls of CFRetain and CFRelease

If I well understood, we don't speak about memory management directly but of the number of owners of an object.
Of course, if there's no more owner, there'll be garbage collecting and the object will be freed.
For your curiosity, you can read from this:
https://developer.apple.com/library/content/documentation/CoreFoundation/Conceptual/CFMemoryMgmt/Concepts/Ownership.html#//apple_ref/doc/uid/20001148-CJBEJBHH

Anyway let's put NEW for the moment.
Comment 4 Alex Thurgood 2017-08-10 11:28:02 UTC
(In reply to Julien Nabet from comment #3)
> I'll take a look too, specifically the calls of CFRetain and CFRelease
> 

> For your curiosity, you can read from this:
> https://developer.apple.com/library/content/documentation/CoreFoundation/
> Conceptual/CFMemoryMgmt/Concepts/Ownership.html#//apple_ref/doc/uid/20001148-
> CJBEJBHH

An interesting read, but one that I only understand in the abstract sense :-/
Comment 5 Commit Notification 2017-08-11 17:03:40 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

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

Related tdf#111634: Multiple memory leaks in libmacabdrv1

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 6 Commit Notification 2017-08-11 17:20:56 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

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

Related tdf#111634: Multiple memory leaks in libmacabdrv1 (part2)

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 7 Julien Nabet 2017-08-11 22:11:39 UTC
Alex: could you give a try when you'll got a build with:
1)
Related tdf#111634: Multiple memory leaks in libmacabdrv1
CFRelease multiLabel since has been created from OUStringToCFString
https://cgit.freedesktop.org/libreoffice/core/commit/?id=5d065a77adf82948909fa76ea0f0ea98f89fd3e6

2)
Related tdf#111634: Multiple memory leaks in libmacabdrv1 (part2)
Delete "sub arrays" of headerNames which is macabfield ** before calling delete headerNames []
https://cgit.freedesktop.org/libreoffice/core/commit/?id=84fc4dec5f419cbe30fa6ac4e228f083837f7887
?

I'd like to know the leaks which are still there in this part.
Comment 8 Julien Nabet 2017-08-12 16:44:14 UTC
The code comes from https://developer.apple.com/library/content/documentation/DeviceDrivers/Conceptual/HID/workingwith/workingwith.html
which is legacy code.
Here's the beginning of this page:
"This chapter provides step-by-step instructions, including listings of sample code, for accessing a HID class device on older versions of OS X. If you are developing new code for OS X v10.5 or later, you should use the APIs described in Accessing a HID Device.."

Since prerequisite for LO is 10.9 minimum so I suppose it should be converted with the help of this page:
https://developer.apple.com/library/content/documentation/DeviceDrivers/Conceptual/HID/new_api_10_5/tn2187.html#//apple_ref/doc/uid/TP40000970-CH214-SW2

I can't help here but I'm interested in being notified about what's new for this bugtracker so I won't uncc myself of this one :-)
Comment 9 Commit Notification 2017-08-14 06:35:02 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=c68a7c94f02989ac7122be0a319251dbaf344952&h=libreoffice-5-4

Related tdf#111634: Multiple memory leaks in libmacabdrv1

It will be available in 5.4.2.

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 10 Commit Notification 2017-08-14 06:35:10 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=39a58df9e9f990faf231c2b1c2dc7fcdc1c4a6cf&h=libreoffice-5-4

Related tdf#111634: Multiple memory leaks in libmacabdrv1 (part2)

It will be available in 5.4.2.

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 11 Alex Thurgood 2017-08-16 14:05:16 UTC
Hi Julien,

I rebuilt master after your changes were made, and am enclosing the memory leak output profile. Some of the previous leaky points seem to have been resolved, but others remain.
Comment 12 Alex Thurgood 2017-08-16 14:06:23 UTC
Created attachment 135588 [details]
Memory leak profile after recent commits
Comment 13 Alex Thurgood 2017-08-16 14:13:39 UTC
The biggest seems to be :

Malloc 16 Bytes	328067	< multiple >	5,01 MiB	libmacabdrv1.dylib	connectivity::macab::MacabRecord::copy(int) const


macabfield *MacabRecord::copy(const sal_Int32 i) const
{
    /* Note: copy(i) creates a new macabfield identical to that at
     * location i, whereas get(i) returns a pointer to the macabfield
     * at location i.
     */
    if(i < size)
    {
        macabfield *_copy = new macabfield;
        _copy->type = fields[i]->type;
        _copy->value = fields[i]->value;
        if (_copy->value)
            CFRetain(_copy->value);
        return _copy;
    }

    return nullptr;
}


This gets invoked 360,000 times in all in just the short sequence of usage described originally.
Comment 14 Telesto 2017-08-16 15:07:58 UTC
@Alex
Off-topic (not sure how to contact you), but would you mind to make a few instrument leak trace (I don't have a own build around with source code linked)
1. related to bug 105500. It's starts leaking when creating a shape. And increases when switching back and forward between two shapes.
2. Open Impress -> Cancel or accept the template -> select master Pages tab in the sidebar (again a leak)
3. I also noticed a possible leak related to the Document recovery (when discarding the recovery)
Comment 15 Alex Thurgood 2017-08-17 07:14:32 UTC
(In reply to Telesto from comment #14)
> @Alex
> Off-topic (not sure how to contact you), but would you mind to make a few
> instrument leak trace (I don't have a own build around with source code
> linked)
> 1. related to bug 105500. It's starts leaking when creating a shape. And
> increases when switching back and forward between two shapes.
> 2. Open Impress -> Cancel or accept the template -> select master Pages tab
> in the sidebar (again a leak)
> 3. I also noticed a possible leak related to the Document recovery (when
> discarding the recovery)

@Telesto : I'll take a look. You can see my email address if you hover the mouse over my name at the top of this bug.
Comment 16 Commit Notification 2017-08-17 21:30:02 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

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

Related tdf#111634: Multiple memory leaks in libmacabdrv1 (part3)

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 17 Commit Notification 2017-08-18 07:47:56 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=0d9db27cb03e393397f292b9e09b4ac515a4b883&h=libreoffice-5-4

Related tdf#111634: Multiple memory leaks in libmacabdrv1 (part3)

It will be available in 5.4.2.

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 18 Alex Thurgood 2017-08-18 14:21:31 UTC
(In reply to Commit Notification from comment #16)
> Julien Nabet committed a patch related to this issue.
> It has been pushed to "master":
> 
> http://cgit.freedesktop.org/libreoffice/core/commit/
> ?id=27b1e21913d8119ea27be05954156d15ca069e66
> 
> Related tdf#111634: Multiple memory leaks in libmacabdrv1 (part3)
> 
> 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.


Hmm, testing with 

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

doesn't seem to show any difference, I still see 300K+ invocations of 

connectivity::macab::MacabRecord::copy(int) const
Comment 19 Noel Grandin 2017-08-18 14:31:48 UTC
(In reply to Alex Thurgood from comment #18)
> doesn't seem to show any difference, I still see 300K+ invocations of 
> 
> connectivity::macab::MacabRecord::copy(int) const


Note that allocations by themselves are not a problem. The problem is when the allocation is not matched by a free.

I see that MacabRecords::createHeaderForProperty is calling copy(), and then attempting to release the records at the bottom, but without calling 
   CFRelease(_copy->value);
That may be a problem.

I actually think that the macabfield struct should have a destructor, and that destructor should call CFRelease, instead of replicating the logic all over the place.

Also  MacabRecords::createHeaderForProperty should probably be using 
    std::vector<std::unique_ptr<macabfield >>
instead of manually managing stuff.


Also MacabHeader contains a field called "fields" that should really be a
   std::vector<std::unique_ptr<macabfield>>
and MacabHeader does not seem to be releasing anything when it destructs, which is probably a bug.


In general, this whole chunk of code is pretty bad.
Comment 20 Julien Nabet 2017-08-19 14:16:18 UTC
(In reply to Noel Grandin from comment #19)
> ...
> I actually think that the macabfield struct should have a destructor, and
> that destructor should call CFRelease, instead of replicating the logic all
> over the place.
> ...
I tried this but had a crash:
    frame #1: 0x000000015e824b17 libmacabdrv1.dylib`connectivity::macab::MacabRecord::copy(this=0x00007f8ef81a14d0, i=0) const at MacabRecord.cxx:129
    frame #2: 0x000000015e828d18 libmacabdrv1.dylib`connectivity::macab::MacabRecords::createHeaderForProperty(this=0x00007f8ef82c4590, _propertyType=6, _propertyValue=0x00007f8ef81c5240, _propertyName=0x00007f8ef83395d0) const at MacabRecords.cxx:763
    frame #3: 0x000000015e82775a libmacabdrv1.dylib`connectivity::macab::MacabRecords::createHeaderForProperty(this=0x00007f8ef82c4590, _propertyType=262, _propertyValue=0x00007f8ef8196c30, _propertyName=0x000000015be3cd80) const at MacabRecords.cxx:657
    frame #4: 0x000000015e826b96 libmacabdrv1.dylib`connectivity::macab::MacabRecords::createHeaderForProperty(this=0x00007f8ef82c4590, _record=0x00007f8ef823e380, _propertyName=0x00007fff9b9f4280, _recordType=0x00007fff9b9ed4a0, _isPropertyRequired=true) const at MacabRecords.cxx:535
    frame #5: 0x000000015e826047 libmacabdrv1.dylib`connectivity::macab::MacabRecords::createHeaderForRecordType(this=0x00007f8ef82c4590, _records=0x00007f8ef8187a00, _recordType=0x00007fff9b9ed4a0) const at MacabRecords.cxx:470
    frame #6: 0x000000015e825bde libmacabdrv1.dylib`connectivity::macab::MacabRecords::initialize(this=0x00007f8ef82c4590) at MacabRecords.cxx:175
    frame #7: 0x000000015e8301ff libmacabdrv1.dylib`connectivity::macab::MacabAddressBook::getMacabRecords(this=0x00007f8ef814f0b0) at MacabAddressBook.cxx:129
    frame #8: 0x000000015e813fb0 libmacabdrv1.dylib`connectivity::macab::MacabDatabaseMetaData::getTables(this=0x000000015d8e8928, (null)=0x00007fff59436c48, (null)=0x00007fff59436c40, (null)=0x00007fff59436c38, types=0x00007fff59436c70) at MacabDatabaseMetaData.cxx:958
    frame #9: 0x000000015e814a5b libmacabdrv1.dylib`non-virtual thunk to connectivity::macab::MacabDatabaseMetaData::getTables(this=0x000000015d8e8928, (null)=0x00007fff59436c48, (null)=0x00007fff59436c40, (null)=0x00007fff59436c38, types=0x00007fff59436c70) at MacabDatabaseMetaData.cxx:0
    frame #10: 0x00000001580279da libdbalo.dylib`dbaccess::OFilteredContainer::construct(this=0x00007f8ef5bcc3e0, _rTableFilter=0x0000000156d17248, _rTableTypeFilter=0x0000000156d17250) at FilteredContainer.cxx:347

I put the value to nullptr, I even tested value != nullptr in copy function, it still crashes at the same location.

I'm stuck here.

Moreover, I noticed that a bunch of files in macab driver but not only in this part (apple remote part too), used carbon.h.
Carbon is deprecated from MacOs 10.8 (the min MacOs version required for LO) see https://developer.apple.com/library/content/releasenotes/General/CarbonCoreDeprecations/index.html

It seems MacOs/ios specific parts need some rewriting. Perhaps an idea for next year GSOC? But first perhaps notify this situation in ESC?
Comment 21 Noel Grandin 2017-08-19 17:23:28 UTC
(In reply to Julien Nabet from comment #20)
> (In reply to Noel Grandin from comment #19)
> > ...
> > I actually think that the macabfield struct should have a destructor, and
> > that destructor should call CFRelease, instead of replicating the logic all
> > over the place.
> > ...
> I tried this but had a crash:

I pushed a WIP patch here:

   https://gerrit.libreoffice.org/#/c/41332/

completely untested, but it illustrates what I think is the right approach here.
Comment 22 Julien Nabet 2017-08-20 18:48:54 UTC
Alex: do you have some link to explain more precisely how to run LO with XCode?

Indeed I runned make xcode-ide-integration
but in osx, I got libreoffice.xcodeproj	and soffice.xcodeproj
I opened first one and XCode started to index all the files.
But then what to do precisely?
Where's Instruments.app?
How to define target?

(I built with LO on console with enable-dbgutil not from XCode).
Comment 23 Telesto 2017-08-20 18:55:01 UTC
Instruments is a separate application: Spotlight (CMD + Space), type: instruments
Comment 24 Alex Thurgood 2017-08-21 08:37:49 UTC
(In reply to Julien Nabet from comment #22)
> Alex: do you have some link to explain more precisely how to run LO with
> XCode?
>

Hi Julien,
 
You don't need to build LO with XCode, indeed, I'm not even sure that you can (I managed once a very long time ago, but before the xcode-ide-integration script even existed - however, at the time, and it was completely by chance, I couldn't get any executable to run, so forget building with XCode).

You don't need Xcode to use Instruments.app, but the reason I mention it in my reports is that Xcode includes a menu entry for developer tools that includes Instruments.app. However, as I found out recently, and as Telesto has indicated, you can find and start it directly by just typing Instruments into the Spotlight search function. Once Instruments has started, you can point it to your instdir tree as the target for analysis. Starting the recording (having previously selected the analytical tool of your choice (memory allocation, leaks, time profiling, stack/call tree, etc) will then automatically launch LODev.app in your /instdir, and close LO once you stop recording.

I have to say that it is still a pretty slow process with a debug build, even with 8G RAM and 4 cores. Doing even the simplest of recordings can take 5 minutes or more each time.
Comment 25 Julien Nabet 2017-08-21 19:57:49 UTC
(In reply to Telesto from comment #23)
> Instruments is a separate application: Spotlight (CMD + Space), type:
> instruments

Found it, Ctrl + Space gives Spotlight then instruments
Comment 26 Julien Nabet 2017-08-21 20:23:22 UTC
(In reply to Alex Thurgood from comment #24)
> (In reply to Julien Nabet from comment #22)
> > Alex: do you have some link to explain more precisely how to run LO with
> > XCode?
> >
> 
> Hi Julien,
>  
> You don't need to build LO with XCode, indeed, I'm not even sure that you
> can (I managed once a very long time ago, but before the
> xcode-ide-integration script even existed - however, at the time, and it was
> completely by chance, I couldn't get any executable to run, so forget
> building with XCode).
> 
> You don't need Xcode to use Instruments.app, but the reason I mention it in
> my reports is that Xcode includes a menu entry for developer tools that
> includes Instruments.app. However, as I found out recently, and as Telesto
> has indicated, you can find and start it directly by just typing Instruments
> into the Spotlight search function. Once Instruments has started, you can
> point it to your instdir tree as the target for analysis. Starting the
> recording (having previously selected the analytical tool of your choice
> (memory allocation, leaks, time profiling, stack/call tree, etc) will then
> automatically launch LODev.app in your /instdir, and close LO once you stop
> recording.
> 
> I have to say that it is still a pretty slow process with a debug build,
> even with 8G RAM and 4 cores. Doing even the simplest of recordings can take
> 5 minutes or more each time.

I got only 4GB on my Mac from 2011 (i5, 2,5Ghz), hope that just launching Instruments instead of use Instruments from xCode will help :-)
Comment 27 Telesto 2017-08-22 06:05:33 UTC
(In reply to Julien Nabet from comment #26)
> I got only 4GB on my Mac from 2011 (i5, 2,5Ghz), hope that just launching
> Instruments instead of use Instruments from xCode will help :-)

Another option to speed things up a bit is to launch LibreOffice first, attach it as a running proces, and start recording from the start center or something like that. It feels snappier to me. It will give some faulty/broke results at te beginnen of the recording, however.

When searching issues for issues, a normal - optimised - build will do perfectly fine (in my experience). A symbol build is (only) needed for a source code view.