Created attachment 135408 [details]
Summary of memory leak profile

This is a summary of the memory leak profile produced

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

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.

(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 :-/

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.

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.

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.

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 :-)

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.

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.

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.

Created attachment 135588 [details]
Memory leak profile after recent commits

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.

@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)

(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.

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.

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.

(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

(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.

(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?

(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.

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).

Instruments is a separate application: Spotlight (CMD + Space), type: instruments

(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.

(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

(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 :-)

(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.

Currently Apple Instruments leak agent crashes on me repeatedly when trying to trace memory leaks with LO master for this bug, so testing impossible.

Testing blocked by bug 126961

Dear Alex Thurgood,

To make sure we're focusing on the bugs that affect our users today, LibreOffice QA is asking bug reporters and confirmers to retest open, confirmed bugs which have not been touched for over a year.

There have been thousands of bug fixes and commits since anyone checked on this bug report. During that time, it's possible that the bug has been fixed, or the details of the problem have changed. We'd really appreciate your help in getting confirmation that the bug is still present.

If you have time, please do the following:

Test to see if the bug is still present with the latest version of LibreOffice from https://www.libreoffice.org/download/

If the bug is present, please leave a comment that includes the information from Help - About LibreOffice.
 
If the bug is NOT present, please set the bug's Status field to RESOLVED-WORKSFORME and leave a comment that includes the information from Help - About LibreOffice.

Please DO NOT

Update the version field
Reply via email (please reply directly on the bug tracker)
Set the bug's Status field to RESOLVED - FIXED (this status has a particular meaning that is not 
appropriate in this case)


If you want to do more to help you can test to see if your issue is a REGRESSION. To do so:
1. Download and install oldest version of LibreOffice (usually 3.3 unless your bug pertains to a feature added after 3.3) from https://downloadarchive.documentfoundation.org/libreoffice/old/

2. Test your bug
3. Leave a comment with your results.
4a. If the bug was present with 3.3 - set version to 'inherited from OOo';
4b. If the bug was not present in 3.3 - add 'regression' to keyword


Feel free to come ask questions or to say hello in our QA chat: https://web.libera.chat/?settings=#libreoffice-qa

Thank you for helping us make LibreOffice even better for everyone!

Warm Regards,
QA Team

MassPing-UntouchedBug