Bug 111894 - PaletteManager::LoadPalettes() leaks memory
Summary: PaletteManager::LoadPalettes() leaks memory
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: framework (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 target:5.4.4
Keywords:
: 108605 (view as bug list)
Depends on:
Blocks: Area-Fill-Tab Color-Palettes
  Show dependency treegraph
 
Reported: 2017-08-18 07:47 UTC by Alex Thurgood
Modified: 2017-11-23 07:45 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:47:58 UTC
Description:
PaletteManager::LoadPalettes() leaks memory with multiple invocations

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 new Writer document icon to open a blank Writer document.
7. Type some random text.
8. Right mouse button click on the page
9. Select Area, then Pattern.
10. Close the Writer file without saving.
11. Stop recording.
12. Analyse the profile trace.


Actual Results:  
Memory leak in PaletteManager::LoadPalettes()

Expected Results:
Shouldn't leak memory


Reproducible: Always

User Profile Reset: No

Additional Info:
The code pointed to by the profile tool is in PaletteManager.cxx

void PaletteManager::LoadPalettes()
{
    m_Palettes.clear();
    OUString aPalPaths = SvtPathOptions().GetPalettePath();

    std::stack<OUString> aDirs;
    sal_Int32 nIndex = 0;
    do
    {
        aDirs.push(aPalPaths.getToken(0, ';', nIndex));
    }
    while (nIndex >= 0);

    std::set<OUString> aNames;
    //try all entries palette path list user first, then
    //system, ignoring duplicate file names
    while (!aDirs.empty())
    {
        OUString aPalPath = aDirs.top();
        aDirs.pop();

        osl::Directory aDir(aPalPath);
        osl::DirectoryItem aDirItem;
        osl::FileStatus aFileStat( osl_FileStatus_Mask_FileName |
                                   osl_FileStatus_Mask_FileURL  |
                                   osl_FileStatus_Mask_Type     );
        if( aDir.open() == osl::FileBase::E_None )
        {
            while( aDir.getNextItem(aDirItem) == osl::FileBase::E_None )
            {
                aDirItem.getFileStatus(aFileStat);
                if(aFileStat.isRegular() || aFileStat.isLink())
                {
                    OUString aFName = aFileStat.getFileName();
                    INetURLObject aURLObj( aFileStat.getFileURL() );
                    OUString aFNameWithoutExt = aURLObj.GetBase();
                    if (aNames.find(aFName) == aNames.end())
                    {
                        std::unique_ptr<Palette> pPalette;
                        if( aFName.endsWithIgnoreAsciiCase(".gpl") )
                            pPalette.reset(new PaletteGPL(aFileStat.getFileURL(), aFNameWithoutExt));
                        else if( aFName.endsWithIgnoreAsciiCase(".soc") )
                            pPalette.reset(new PaletteSOC(aFileStat.getFileURL(), aFNameWithoutExt));
                        else if ( aFName.endsWithIgnoreAsciiCase(".ase") )
                            pPalette.reset(new PaletteASE(aFileStat.getFileURL(), aFNameWithoutExt));

                        if( pPalette && pPalette->IsValid() )
                            m_Palettes.push_back( std::move(pPalette) );
                        aNames.insert(aFNameWithoutExt);
                    }
                }
            }
        }
    }
}


and in particular, line 109:

 pPalette.reset(new PaletteSOC(aFileStat.getFileURL(), aFNameWithoutExt));





User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:54.0) Gecko/20100101 Firefox/54.0
Comment 1 Telesto 2017-08-18 08:57:12 UTC
Thanks for filling the bug. However, it seems a was focusing to much on the leak detection. Instruments is indicating a leak, but the memory usage in general isn't structurally increasing as far I can tell. So there is a change that this is the same as bug 105500 comment 14
Comment 2 Alex Thurgood 2017-08-25 14:56:07 UTC
(In reply to Telesto from comment #1)
> Thanks for filling the bug. However, it seems a was focusing to much on the
> leak detection. Instruments is indicating a leak, but the memory usage in
> general isn't structurally increasing as far I can tell. So there is a
> change that this is the same as bug 105500 comment 14

Even though some of the memory is released from PaletteManager::LoadPalettes, overal memory consumption continues to increase
Comment 3 Telesto 2017-08-25 15:04:54 UTC
(In reply to Alex Thurgood from comment #2)
> (In reply to Telesto from comment #1)
> > Thanks for filling the bug. However, it seems a was focusing to much on the
> > leak detection. Instruments is indicating a leak, but the memory usage in
> > general isn't structurally increasing as far I can tell. So there is a
> > change that this is the same as bug 105500 comment 14
> 
> Even though some of the memory is released from
> PaletteManager::LoadPalettes, overal memory consumption continues to increase

Confirming -> setting to new.
Comment 4 Telesto 2017-08-25 17:36:58 UTC
Another example where PaletteManager::LoadPalettes() is leaking:
1. Open Writer
2. Add a table with a few cells
3. Open the context menu (for me right click) -> Table properties -> Background Tab --> Select a cell color
4. Select a different cell repeat step 3.
Comment 5 Alex Thurgood 2017-08-28 11:23:55 UTC
(In reply to Telesto from comment #4)
> Another example where PaletteManager::LoadPalettes() is leaking:
> 1. Open Writer
> 2. Add a table with a few cells
> 3. Open the context menu (for me right click) -> Table properties ->
> Background Tab --> Select a cell color
> 4. Select a different cell repeat step 3.



Confirming too.

Note that line 131 of vclptr.hxx is also implicated here with :

        return VclPtr< reference_type >( new reference_type(std::forward<Arg>(arg)...), SAL_NO_ACQUIRE );
Comment 6 Julien Nabet 2017-09-02 10:58:33 UTC
I'm still trying to understand how to run Instruments leak and I'm a bit stuck.
First it's indeed very slow but I had been warned.
Then when clicking first time on a leak, it displays a bt. But when clicking or double clicking on another leak, it doesn't work, it stays on the same bt.
There's only a vertical dashline which appears on leak but that's all.

Also, reading https://developer.apple.com/library/content/documentation/DeveloperTools/Conceptual/InstrumentsUserGuide/FindingLeakedMemory.html, there's no "Command 2" to invert calltrack
I searched on Youtube too but there are a lot of old videos with different interface.

It's a pity because there are quite a bunch of bugtrackers about leak and it would have been useful to have analysis from Instruments.
Comment 7 Julien Nabet 2017-09-03 19:58:11 UTC
I finally found how to keep on.

I select leaks on bottom pane, then click on a specific line to see the associated bt at bottom right pane.
It allowed me to fix an enable-debug only leak (see https://cgit.freedesktop.org/libreoffice/core/commit/?id=09ae0a74bb946bc64fbc76082a52064a36ee9b01)
Comment 8 Alex Thurgood 2017-09-05 07:50:37 UTC
(In reply to Julien Nabet from comment #7)
> I finally found how to keep on.


Sorry, I have been away, but it seems that you have got the hang of it :-)



I'm still seeing (with my latest master build from yesterday) unreleased memory allocation in :

   pPalette.reset(new PaletteSOC(aFileStat.getFileURL(), aFNameWithoutExt));
Comment 9 Julien Nabet 2017-09-05 19:10:00 UTC
Noel: trying to find where's the leak, I noticed 2 things:
In svx/source/tbxctrls/tbcontrl.cxx, the order of destruction in
1368  void SvxColorWindow::dispose()
1369  {
1370      mpColorSet.clear();
1371      mpRecentColorSet.clear();
1372      mpPaletteListBox.clear();
1373      mpButtonAutoColor.clear();
1374      mpButtonNoneColor.clear();
1375      mpButtonPicker.clear();
1376      mpAutomaticSeparator.clear();
1377      SfxPopupWindow::dispose();
1378  }

isn't the reverse ordering of construction:
1246  SvxColorWindow::SvxColorWindow(const OUString&            rCommand,
...
1263      get(mpPaletteListBox,     "palette_listbox");
1264      get(mpButtonAutoColor,    "auto_color_button");
1265      get(mpButtonNoneColor,    "none_color_button");
1266      get(mpButtonPicker,       "color_picker_button");
1267      get(mpColorSet,           "colorset");
1268      get(mpRecentColorSet,     "recent_colorset");
1269      get(mpAutomaticSeparator, "separator4");

or is the order doesn't be taken into account because it seems there's  no parent-children relation for these vars?

Also, I wonder if using disposeAndClear may help here since we have this:
58      VclPtr<ListBox>     mpPaletteListBox;
59      VclPtr<PushButton>  mpButtonAutoColor;
60      VclPtr<PushButton>  mpButtonNoneColor;
61      VclPtr<PushButton>  mpButtonPicker;
62      VclPtr<FixedLine>   mpAutomaticSeparator;
in include/svx/colorwindow.hxx ?
Comment 10 Noel Grandin 2017-09-05 19:22:05 UTC
(In reply to Julien Nabet from comment #9)
> or is the order doesn't be taken into account because it seems there's  no
> parent-children relation for these vars?
> 

correct.

> Also, I wonder if using disposeAndClear may help here since we have this:


Worth a try. disposeAndClear() is technically more correct, since this class is the primary owner.
Comment 11 Maxim Monastirsky 2017-09-05 19:49:36 UTC
(In reply to Noel Grandin from comment #10)
> Worth a try. disposeAndClear() is technically more correct, since this class
> is the primary owner.
The primary owner is actually VclBuilder, and it does disposeAndClear() in VclBuilder::disposeBuilder().
Comment 12 Commit Notification 2017-09-09 17:38:32 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

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

tdf#111894: fix leak memory with PaletteManager with SvxColorListBox

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-09-09 17:44:35 UTC
I hope this last patch may help.
At least, I compared the number of calls of PaletteManager constructor and destructor and it's the same value with the patch (there was more calls to constructor than calls to destructor before the patch).

I'm eager to hear some news about this bugtracker and if it helps here, it may help for the 2 other bugtrackers put in see also.

Just a con, even if it helps, it's more a bandaid than a real fix. Indeed, the real root cause isn't clear.
Comment 14 Telesto 2017-09-10 15:38:04 UTC
The bandaid seems to work nicely. No leaking in PaletteManager::LoadPalettes()
Thanks for fixing Julien!
Comment 15 Julien Nabet 2017-09-11 20:15:02 UTC
Here's the patch for 5.4 branch waiting for approval: https://gerrit.libreoffice.org/#/c/42138/

Let's put this one to FIXED now.

Telesto: would you have some time to give a try to tdf#108605 and tdf#105500 put in see also?
Comment 16 Telesto 2017-09-12 18:17:31 UTC
*** Bug 108605 has been marked as a duplicate of this bug. ***
Comment 17 Telesto 2017-09-12 18:21:19 UTC
(In reply to Julien Nabet from comment #15)
> Here's the patch for 5.4 branch waiting for approval:
> https://gerrit.libreoffice.org/#/c/42138/
> 
> Let's put this one to FIXED now.
> 
> Telesto: would you have some time to give a try to tdf#108605 and tdf#105500
> put in see also?

No change for bug 105500
Comment 18 Alex Thurgood 2017-09-13 09:55:37 UTC
Verified with 

Version: 6.0.0.0.alpha0+
Build ID: 9c5c905680f7cb58eb3d0fbf25725a50c17896da
CPU threads: 4; OS: Mac OS X 10.12.6; UI render: default; 
Locale: fr-FR (fr_FR.UTF-8); Calc: group
Comment 19 Commit Notification 2017-09-15 10:47:27 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=61d85c4e7c30ea0f5242d927b7456190020b4fbe&h=libreoffice-5-4

tdf#111894: fix leak memory with PaletteManager with SvxColorListBox

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 20 Commit Notification 2017-09-19 07:10:49 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

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

Revert "tdf#111894: fix leak memory with PaletteManager with SvxColorListBox"

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 21 Julien Nabet 2017-09-19 07:21:15 UTC
I had to revert the fix because of regression (see https://gerrit.libreoffice.org/#/c/42138/2/svx/source/tbxctrls/tbcontrl.cxx)
Comment 22 Commit Notification 2017-09-19 07:34:32 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=324d7f28beaf36c91361debf478e74b9fced7bfb&h=libreoffice-5-4

Revert "tdf#111894: fix leak memory with PaletteManager with SvxColorListBox"

It will be available in 5.4.3.

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 23 Commit Notification 2017-09-19 08:57:51 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "libreoffice-5-4-2":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=7509927b9e54c9b249a6a01adc02e8ebcbaf62f4&h=libreoffice-5-4-2

Revert "tdf#111894: fix leak memory with PaletteManager with SvxColorListBox"

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 24 Julien Nabet 2017-09-20 06:53:17 UTC
A new patch is discussed here:
https://gerrit.libreoffice.org/#/c/42494/
Comment 25 Commit Notification 2017-09-21 21:56:07 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=46fa042b94a0364c09482e8a09f8874119db231c

tdf#111894: fix leak memory with PaletteManager (take 2)

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 26 Julien Nabet 2017-09-25 22:09:12 UTC
I don't know if the patch for 5.4 will be accepted but let's put this one to FIXED.
Comment 27 Commit Notification 2017-11-23 07:45:40 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=439a6e7c7e32089810b878dca77fa64ee454a596&h=libreoffice-5-4

tdf#111894: fix leak memory with PaletteManager (take 2)

It will be available in 5.4.4.

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.