Bug 111567 - Template Manager memory leak in TemplateLocalView::insertItems()
Summary: Template Manager memory leak in TemplateLocalView::insertItems()
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: framework (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:
Depends on:
Blocks:
 
Reported: 2017-08-09 16:17 UTC by Alex Thurgood
Modified: 2017-08-17 13:53 UTC (History)
2 users (show)

See Also:
Crash report or crash signature:


Attachments
Screenshot of Instruments.app showing memory leaks (93.83 KB, image/png)
2017-08-09 16:19 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-09 16:17:18 UTC
Description:
When initiating the TemplateManager, and then moving through the dropdown filter to filter templates according to application module, multiple instances of memory leaks occur in 

TemplateLocalView::insertItems()

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 the template manager from the left dropdown menu.
7. When the Template Manager has loaded, cycle between the various application module filters using the dropdown menu.
8. Close the template manager window.
9. Stop recording.
10. Analyse the profile trace.

Actual Results:  
Multiple instances of memory leaks occur in 

TemplateLocalView::insertItems()

when instantiating and navigating through the template filter.

Expected Results:
It shouldn't leak memory


Reproducible: Always

User Profile Reset: No

Additional Info:
The problem appears to lie in templatelocalview.cxx in this block of code :

void TemplateLocalView::insertItems(const std::vector<TemplateItemProperties> &rTemplates, bool isRegionSelected, bool bShowCategoryInTooltip)
{
    mItemList.clear();

    std::vector<ThumbnailViewItem*> aItems(rTemplates.size());
    for (size_t i = 0, n = rTemplates.size(); i < n; ++i )
    {
        const TemplateItemProperties *pCur = &rTemplates[i];

        TemplateViewItem *pChild;
        if(isRegionSelected)
            pChild = new TemplateViewItem(*this, pCur->nId);
        else
            pChild = new TemplateViewItem(*this, i+1);

        pChild->mnDocId = pCur->nDocId;
        pChild->mnRegionId = pCur->nRegionId;
        pChild->maTitle = pCur->aName;
        pChild->setPath(pCur->aPath);




and in particularly with this line :

            pChild = new TemplateViewItem(*this, i+1);



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-09 16:19:41 UTC
Created attachment 135355 [details]
Screenshot of Instruments.app showing memory leaks
Comment 2 Telesto 2017-08-09 17:20:55 UTC
Memory usage is increasing on Windows too while browsing through the template manager. It only drops after closing the template manager. So I tend to confirm.

Not sure if it's related, but there is probably also leaking when switching templates in Impress. Steps to reproduce
1. Open Impress Document -> Choose a template
2. Add slides (around 25)
3. Select all slides. Apply different templates from Master slides panel
4. Memory usage will increase (after each template)

I tested it on Windows with Undo count set to zero (Tools - > Options - LibO - Advanced - Expert config: org.openoffice.Office.Common/Undo).
Comment 3 Julien Nabet 2017-08-09 17:23:22 UTC
(In reply to Telesto from comment #2)
> Memory usage is increasing on Windows too while browsing through the
> template manager. It only drops after closing the template manager. So I
> tend to confirm.
>...
When you close the template manager, is the memory consumption the same as before opening it, a bit more, a lot more?
If it's the same, I'm not sure there's a leak.
Comment 4 Telesto 2017-08-09 18:39:50 UTC
(In reply to Julien Nabet from comment #3)
Never mind. I shouldn't compare MacOS with Windows. Repro with:
Version: 6.0.0.0.alpha0+
Build ID: f7c59aaa078576a413846b7c8024e728818ca2be
CPU threads: 4; OS: Mac OS X 10.12.6; UI render: default; 
Locale: nl-NL (nl_NL.UTF-8); Calc: group
Comment 5 Noel Grandin 2017-08-10 09:03:36 UTC
To me, looks like the bug is that 

   TemplateLocalView::insertItems()

calls 

   mItemList.clear();

at the top of it's code. It should not, it should leave the clearing and deleting to 

  ThumbnailView::updateItems

which already handles it correctly.

Probably ThumbnailView should not be exposing it's mItemList member anyhow.
Comment 6 Julien Nabet 2017-08-10 09:16:42 UTC
(In reply to Noel Grandin from comment #5)
> To me, looks like the bug is that 
> 
>    TemplateLocalView::insertItems()
> 
> calls 
> 
>    mItemList.clear();
> 
> at the top of it's code. It should not, it should leave the clearing and
> deleting to 
> 
>   ThumbnailView::updateItems
> 
> which already handles it correctly.
> 
> ...

You're right Noel.
ThumbnailView::updateItems (from https://opengrok.libreoffice.org/xref/core/sfx2/source/control/thumbnailview.cxx#1005) calls ThumbnailView::ImplDeleteItems() (from https://opengrok.libreoffice.org/xref/core/sfx2/source/control/thumbnailview.cxx#ImplDeleteItems) which deletes each item before clearing the vector.
So the simple patch would be to delete this extra "mItemList.clear();" in TemplateLocalView::insertItems
Comment 7 Julien Nabet 2017-08-10 19:14:10 UTC
I submitted a patch on gerrit: https://gerrit.libreoffice.org/#/c/40995/
Comment 8 Commit Notification 2017-08-11 05:37:53 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

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

tdf#111567: fix memleak in TemplateLocalView::insertItems

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 9 Commit Notification 2017-08-11 07:13: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=08ae774fe92f0576691c9b0177fef45d18fd77b3&h=libreoffice-5-4

tdf#111567: fix memleak in TemplateLocalView::insertItems

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 10 Julien Nabet 2017-08-11 07:17:02 UTC
Let's put this one to FIXED, since there's only 1 version left for 5.3
Comment 11 Alex Thurgood 2017-08-17 13:53:51 UTC
Verified fixed with my master build of today (17/08/2017)