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
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
(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
(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.
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.
(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 );
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.
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)
(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));
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 ?
(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.
(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().
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.
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.
The bandaid seems to work nicely. No leaking in PaletteManager::LoadPalettes() Thanks for fixing Julien!
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?
*** Bug 108605 has been marked as a duplicate of this bug. ***
(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
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
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.
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.
I had to revert the fix because of regression (see https://gerrit.libreoffice.org/#/c/42138/2/svx/source/tbxctrls/tbcontrl.cxx)
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.
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.
A new patch is discussed here: https://gerrit.libreoffice.org/#/c/42494/
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.
I don't know if the patch for 5.4 will be accepted but let's put this one to FIXED.
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.