Download it now!
Bug 112388 - Fraction::Fraction(Fraction const&) leaks memory
Summary: Fraction::Fraction(Fraction const&) leaks memory
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
6.0.0.0.alpha0+
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:6.0.0 target:5.4.3
Keywords:
Depends on:
Blocks:
 
Reported: 2017-09-14 14:04 UTC by Telesto
Modified: 2017-09-25 13:19 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments
BT and screenshot (262.15 KB, application/x-zip-compressed)
2017-09-14 14:05 UTC, Telesto
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Telesto 2017-09-14 14:04:44 UTC
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
Comment 1 Telesto 2017-09-14 14:05:17 UTC
Created attachment 136243 [details]
BT and screenshot
Comment 2 Julien Nabet 2017-09-14 19:11:51 UTC
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.
Comment 3 Julien Nabet 2017-09-16 15:35:39 UTC
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.
Comment 4 Commit Notification 2017-09-24 20:15:48 UTC
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.
Comment 5 Commit Notification 2017-09-25 07:54:47 UTC
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.
Comment 6 Caolán McNamara 2017-09-25 07:55:42 UTC
that seems to do the trick
Comment 7 Julien Nabet 2017-09-25 08:08:27 UTC
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! :-)
Comment 8 Caolán McNamara 2017-09-25 13:05:05 UTC
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.
Comment 9 Julien Nabet 2017-09-25 13:15:33 UTC
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.
Comment 10 Commit Notification 2017-09-25 13:19:45 UTC
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.