Bug Hunting Session
Bug 111892 - Memory Leak in sd::TemplateScanner::ScanEntry()
Summary: Memory Leak in sd::TemplateScanner::ScanEntry()
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Impress (show other bugs)
Version:
(earliest affected)
6.0.0.0.alpha0+
Hardware: x86-64 (AMD64) Mac OS X (All)
: medium normal
Assignee: Julien Nabet
URL:
Whiteboard: target:6.0.0
Keywords:
Depends on:
Blocks:
 
Reported: 2017-08-18 07:13 UTC by Alex Thurgood
Modified: 2017-09-05 19:18 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Thurgood 2017-08-18 07:13:37 UTC
Description:
sd::TemplateScanner::ScanEntry() leaks memory (or rather, doesn't release memory correctly)

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.
6. Click on the Impress button to load a new Impress document. 
7. Cancel the template selection to accept a default blank template.
8. In the sidebar, click on the Switch Master Template button.
9. Close the Impress document without saving.
10. Stop recording

Actual Results:  
Notice how sd::TemplateScanner::ScanEntry() allocates multiple memory blocks, but doesn't release them:

Malloc 16 Bytes    12    < multiple >    192 Bytes    libsdlo.dylib    sd::TemplateScanner::ScanEntry()

Expected Results:
It should not 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-18 07:15:07 UTC
sd::TemplateScanner::ScanEntry()
is invoked from TemplateScanner.cxx at line 242 in the following block :

            {
                //  Check whether the entry is an impress template.  If so
                //  add a new entry to the resulting list (which is created
                //  first if necessary).
                //  These strings are used to find impress templates in the tree of
                //  template files.  Should probably be determined dynamically.
                if (    (sContentType == MIMETYPE_OASIS_OPENDOCUMENT_PRESENTATION_TEMPLATE_ASCII)
                    ||  (sContentType == MIMETYPE_OASIS_OPENDOCUMENT_PRESENTATION_ASCII)
                    ||  (sContentType == "application/vnd.stardivision.impress")
                    ||  (sContentType == MIMETYPE_VND_SUN_XML_IMPRESS_ASCII)
                        // The following id comes from the bugdoc in #i2764#.
                    ||  (sContentType == "Impress 2.0"))
                {
                    OUString sLocalisedTitle = SfxDocumentTemplates::ConvertResourceString(sTitle);
                    mpLastAddedEntry = new TemplateEntry(sLocalisedTitle, sTargetURL);
                    mpTemplateDirectory->InsertEntry(mpLastAddedEntry);
                }
            }
Comment 2 Telesto 2017-08-18 13:34:24 UTC
Confirming with 5.4.0.3
Comment 3 Commit Notification 2017-08-20 13:04: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=fe195f2c7b0c22169f265b731981bef47119f166

Related tdf#111892: revamp TemplateScanner

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 4 Julien Nabet 2017-08-20 19:09:02 UTC
Alex: I'm not sure it'll remove the leak but at least, the code will be simpler to check/debug. I let you try with the new code. If it really helps for the leak, I'll try to backport it on 5.4 branch.
Comment 5 Julien Nabet 2017-08-25 14:16:16 UTC
Alex: any better with a build including http://cgit.freedesktop.org/libreoffice/core/commit/?id=fe195f2c7b0c22169f265b731981bef47119f166 ?
Comment 6 Alex Thurgood 2017-08-25 15:11:34 UTC
(In reply to Julien Nabet from comment #5)
> Alex: any better with a build including
> http://cgit.freedesktop.org/libreoffice/core/commit/
> ?id=fe195f2c7b0c22169f265b731981bef47119f166 ?

I haven't rebuilt in a few days, will start a build again and test to report back.
Comment 7 Alex Thurgood 2017-08-28 10:52:42 UTC
(In reply to Julien Nabet from comment #4)
> Alex: I'm not sure it'll remove the leak but at least, the code will be
> simpler to check/debug. I let you try with the new code. If it really helps
> for the leak, I'll try to backport it on 5.4 branch.

Hi Julien,

It still seems to allocate without releasing, but in a different place, now at line 198 TemplateScanner.cxx :

                    mpTemplateEntries.push_back(new TemplateEntry(sLocalisedTitle, sTargetURL));
Comment 8 Julien Nabet 2017-08-28 20:52:08 UTC
Sorry Alex, I gave a new try but it was wrong https://gerrit.libreoffice.org/#/c/41650/

It seems I can't help here.
Comment 9 Julien Nabet 2017-08-29 09:38:59 UTC
Noel: following abandon of https://gerrit.libreoffice.org/#/c/41650/, if I remember what you told about vector members, during destruction of the object, vectors are automatically cleared. When they're cleared, their objects are cleared too.

But here, since we've got a vector of pointers of TemplateEntry objects (see https://opengrok.libreoffice.org/xref/core/sd/source/ui/inc/TemplateScanner.hxx#115), shouldn't we add a loop to call delete on each entry of the vector during destruction? (https://opengrok.libreoffice.org/xref/core/sd/source/ui/dlg/TemplateScanner.cxx#128)
Comment 10 Noel Grandin 2017-08-29 09:45:35 UTC
(In reply to Julien Nabet from comment #9)
> But here, since we've got a vector of pointers of TemplateEntry objects (see
> https://opengrok.libreoffice.org/xref/core/sd/source/ui/inc/TemplateScanner.
> hxx#115), shouldn't we add a loop to call delete on each entry of the vector
> during destruction?
> (https://opengrok.libreoffice.org/xref/core/sd/source/ui/dlg/TemplateScanner.
> cxx#128)

Yes, it looks like the TemplateEntry objects are being leaked. 

As near as I can tell, maFolderList is not doing anything useful currently, and can be removed.

After that, mpTemplateEntries can be converted to std::vector<std::unique_ptr<T>>
Comment 11 Julien Nabet 2017-08-29 18:57:03 UTC
I restored the patch and updated it, so let's wait for review now.
Comment 12 Commit Notification 2017-08-29 19:39:55 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

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

Related tdf#111892: use unique_ptr

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-29 20:04:54 UTC
Alex: waiting for your feedback for backport (I'll have to merge both patches).
Comment 14 Alex Thurgood 2017-09-05 07:58:01 UTC
Indeed fixed when tested against 

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


Thanks Julien !
Comment 15 Julien Nabet 2017-09-05 19:18:35 UTC
Sorry it seems 5.4 branch is too different from master to backport the fix.