Description: Fraction::Fraction(Fraction const& leaks memory with a single invocations Steps to Reproduce: Steps to Reproduce: 1. Start the 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 Calc document icon to open a blank Clac document. 7. Close the document 8. Stop recording (after the closing is fully processed) 9. Analyse the profile trace. Actual Results: Memory leak in OFraction::Fraction(Fraction const&) Expected Results: Shouldn't leak memory Reproducible: Always User Profile Reset: No Additional Info: Version: 6.0.0.0.alpha0+ Build ID: e970395c692a5c315914ddf5b43cf01e590345ff CPU threads: 4; OS: Mac OS X 10.12.4; UI render: default; Locale: en-US (en_US.UTF-8); Calc: group User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
Created attachment 136243 [details] BT and screenshot
Caolán/Eike: I took a look at the BT included here and found toolkit, vcl and sc parts. 3 libtllo.dylib Fraction::Fraction(Fraction const&) ./tools/source/generic/fract.cxx:60^M 4 libtllo.dylib Fraction::Fraction(Fraction const&) ./tools/source/generic/fract.cxx:61^M 5 libtllo.dylib Fraction::operator=(Fraction const&) ./tools/source/generic/fract.cxx:296^M 6 libvcllo.dylib MapMode::SetScaleY(Fraction const&) ./vcl/source/gdi/mapmod.cxx:127^M 7 libsclo.dylib ScGridWindow::GetDrawMapMode(bool) ./sc/source/ui/view/gridwin3.cxx:268^M 8 libsclo.dylib ScTabView::MakeDrawView(TriState) ./sc/source/ui/view/tabview5.cxx:244^M After adding some debug logs, I compared the number of calls of Fraction ctr and dtr and indeed, there's less call to dtr. I tried to find why and noticed o3tl::cow_wrapper< ImplMapMode > ImplType at frame 6 If there were just a dozen of them, I would take a bt of each to compare but there are thousands of calls. I also tried to distinguished the 5 ctrs, hoping to find a specific one called n times ("n" would have been the investigated diff) Any thoughts to keep on here? Besides, frame 8 corresponds to: 244 pGridWin[i]->SetMapMode(pGridWin[i]->GetDrawMapMode()); and searching in SetMapMode, thought this patch for review https://gerrit.libreoffice.org/#/c/42306/ may also be useful even if not for the Fraction part indeed.
Looking at destructor of ScTabView from sc/source/ui/view/tabview5.cxx line 142 + ctr of ScTabView and its Init method, I wonder if the order of dispose/delete may be wrong. Indeed, I would have put aViewData.KillEditView() at the end since there are a lot of objects which have been created from aViewData. Idem for pGridWin, perhaps the block: for (i=0; i<4; i++) pGridWin[i].disposeAndClear(); should be just at the end before aViewData.KillEditView() I tested this and had no crash but didn't succeed in obtaining nb calls of Fraction destructor with calls of Fraction ctr.
Caolán McNamara committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=eaf7d58ef57daa7580057b38625cd9a96ffe2a43 tdf#112388 make aComboButton release its ScGridWindow outputdevice 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.
Caolán McNamara committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=4d5e9db574bdb1a7517ffda01efe0746cc058d47 Related: tdf#112388 last Fraction leak 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.
that seems to do the trick
Caolán: just for curiosity and if you have time of course, I'd be interested in knowing how you found the root causes. Indeed, just opening and closing Calc gave more than 5000 calls to ctr and dtr so retrieving bt of each one and study them is impossible. Also, the bt of Frac retrieved by Instruments (from MacOs) seemed to show nothing wrong (eg: use of unique_ptr/pointer seemed ok) Anyway, thank you a lot for these 2 patches! :-)
Well on the first one comment #2 showed that ScGridWindow was the owner of the Fraction, so I just added fprintf(stderr, "ctor %p\n", this); to the ScGridWindow ctor and ...dtor... to the dtor and quickly verified that the ctor was called, but not its dtor. I checked that dispose was called and it was, so something else had to be holding a reference to it that blocked the dtor from getting called. I just visually inspected the ctor and checked aComboButton and that seemed a good candidate, tweaked it as above and the ScGridWindow dtor got called, so that was that. On the other, a little harder, so I added static int foo to tools/source/generic/fract.cxx added ctor/dtor fprintfs to the Fraction::Impl ctor/dtor like above except ctor is fprintf(stderr, "ctor %p instance %d\n", this, foo++); and started/closed calc, took the output, sorted and uniq -u it by the printed address so (ctor with matching dtors are filtered out) and there's a line left of the mismatched ctor without a dtor, took that leaked address and its instance number from the output, then simply added if (foo == <thatnumber>) { something; } to the ctor and a breakpoint on that line, run calc, and the backtrace then shows that the Fraction was created from the code of commit number 2 of above. Its rough and ready and unsophisticated but (as long as there's no timing or threading involved) fairly simple to set up.
Caolán: thank you a lot for your very detailed feedback! I was already glad this one was fixed (a step further for more robustness) but it's even better to know how to find fixes. I should have found first one, but I wouldn't have thought about static int for the second one. I'll copy this on my notes because I'm quite sure it may help for other leaks.
Caolán McNamara committed a patch related to this issue. It has been pushed to "libreoffice-5-4": http://cgit.freedesktop.org/libreoffice/core/commit/?id=9602a07fe0cd3f1b9d78279629393ec1d173ebca&h=libreoffice-5-4 tdf#112388 make aComboButton release its ScGridWindow outputdevice 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.