Bug 112254 - CuiAboutConfigTabPage::InsertEntry leaks memory
Summary: CuiAboutConfigTabPage::InsertEntry leaks memory
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
6.0.0.0.alpha0+ Master
Hardware: All 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-09-06 06:54 UTC by Telesto
Modified: 2017-09-15 14:55 UTC (History)
1 user (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 Telesto 2017-09-06 06:54:58 UTC
Description:
CuiAboutConfigTabPage::InsertEntry leaks memory with multiple invocations

Steps to Reproduce:
1. Start the Instruments
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. Go to LibreOffceDev -> Preferences 
7. Open LibreOffceDev -> Advanced
8. Open Expert Configuration
10. Close the Writer file without saving.
11. Stop recording.
12. Analyse the profile trace.

Actual Results:  
Memory leak in CuiAboutConfigTabPage::InsertEntry at pEntry->SetUserData( new UserData(rPropertyPath) );


Expected Results:
Shouldn't leak memory


Reproducible: Always

User Profile Reset: No

Additional Info:
The code pointed to by the profile tool is in optaboutconfig.cxx, Line 213 ( 4233 Samples) -> pEntry->SetUserData( new UserData(rPropertyPath)


void CuiAboutConfigTabPage::InsertEntry(const OUString& rPropertyPath, const OUString& rProp, const OUString& rStatus,
                                        const OUString& rType, const OUString& rValue, SvTreeListEntry *pParentEntry,
                                        bool bInsertToPrefBox)
{
    SvTreeListEntry* pEntry = new SvTreeListEntry;
    pEntry->AddItem(o3tl::make_unique<SvLBoxContextBmp>(
        Image(), Image(), false)); //It is needed, otherwise causes crash
    pEntry->AddItem(o3tl::make_unique<SvLBoxString>(rProp));
    pEntry->AddItem(o3tl::make_unique<SvLBoxString>(rStatus));
    pEntry->AddItem(o3tl::make_unique<SvLBoxString>(rType));
    pEntry->AddItem(o3tl::make_unique<SvLBoxString>(rValue));
    pEntry->SetUserData( new UserData(rPropertyPath) );

    if(bInsertToPrefBox)
        m_pPrefBox->Insert( pEntry, pParentEntry );
    else
        m_prefBoxEntries.push_back(std::unique_ptr<SvTreeListEntry>(pEntry));


User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/603.1.29 (KHTML, like Gecko) Version/10.1 Safari/603.1.29
Comment 1 Alex Thurgood 2017-09-07 07:36:26 UTC
Confirming with :

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
Comment 2 Alex Thurgood 2017-09-07 07:40:27 UTC
Line 213 in optaboutconfig.cxx is pointed to as the culprit :

    pEntry->SetUserData( new UserData(rPropertyPath) );
Comment 3 Julien Nabet 2017-09-07 08:37:38 UTC
I noticed SvHeaderTabListBoxImpl hadn't destructor to call pHeaderBar.disposeAndClear
483  namespace svt
484  {
485      struct SvHeaderTabListBoxImpl
486      {
487          VclPtr<HeaderBar>       m_pHeaderBar;
488          AccessibleFactoryAccess m_aFactoryAccess;
489  
490          SvHeaderTabListBoxImpl() : m_pHeaderBar( nullptr ) { }
491      };
492  }
(see https://opengrok.libreoffice.org/xref/core/svtools/source/contnr/svtabbx.cxx#494)
I'll submit a patch for this if nobody does it before.

I'm not sure it's sufficient. I tried to follow the relationships of the different classes involved, it's quite a maze to follow.
Comment 4 Noel Grandin 2017-09-07 19:47:31 UTC
probably the easiest fix is to define a custom subclass of SvTreeListEntry that delete's the UserData in it's destructor.
something like
  class MyListEntry : public SvTreeListEntry{
  public:
    ~MyListEntry() { delete static_cast<UserData>(GetUserData()); }
  }
unless the ownership of UserData is more complicated.
Comment 5 Julien Nabet 2017-09-07 20:02:44 UTC
(In reply to Noel Grandin from comment #4)
> probably the easiest fix is to define a custom subclass of SvTreeListEntry
> that delete's the UserData in it's destructor.
> something like
>   class MyListEntry : public SvTreeListEntry{
>   public:
>     ~MyListEntry() { delete static_cast<UserData>(GetUserData()); }
>   }
> unless the ownership of UserData is more complicated.

Sorry Noel, I don't know C++ enough to understand this mechanism.
It seems I can't do anything here, so let's uncc.
Comment 6 Commit Notification 2017-09-10 12:32:15 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

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

tdf#112254: fix memory leak in optaboutconfig (cui)

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-09-10 12:33:56 UTC
Telesto/Alex: here too, I compared the number of calls of constructor and destructor of UserData object before and after the patch and it seems ok now.
Waiting for your feedback to backport in 5.4 branch.
Comment 8 Telesto 2017-09-14 14:52:34 UTC
No leaking on Windows and Mac! Thanks Julien!
Comment 9 Telesto 2017-09-15 11:56:45 UTC
Working fine; backport, please =)
Comment 10 Julien Nabet 2017-09-15 12:13:32 UTC
backport for 5.4 waiting for review: https://gerrit.libreoffice.org/#/c/42324/

Let's put this one FIXED.
Comment 11 Commit Notification 2017-09-15 14:55:26 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=14f22c32e1edc2c5c4a72f3e3621e1de57b26b57&h=libreoffice-5-4

tdf#112254: fix memory leak in optaboutconfig (cui)

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.