Issue 120105 - [From Symphony] There is a memory leak in function ScTableSheetObj::PrintAreaUndo_Impl
[From Symphony] There is a memory leak in function ScTableSheetObj::PrintArea...
Status: CLOSED FIXED
Product: performance
Classification: Code
Component: code
AOO 3.4.0
All All
: P3 normal (vote)
: AOO 4.0
Assigned To: AOO issues mailing list
:
Depends on:
Blocks: 120975 121366
  Show dependency treegraph
 
Reported: 2012-06-27 13:57 UTC by ChaoHuang
Modified: 2013-02-16 09:11 UTC (History)
2 users (show)

See Also:
Issue Type: DEFECT
Latest Confirmation on: ---
Developer Difficulty: ---


Attachments
for file "main/sc/source/ui/unoobj/cellsuno.cxx" (530 bytes, patch)
2012-06-27 14:55 UTC, ChaoHuang
chao.dev.h: review?
Details | Diff
Extended patch (1.86 KB, patch)
2012-06-27 16:19 UTC, Armin Le Grand
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this issue.
Description ChaoHuang 2012-06-27 13:57:18 UTC
Steps to reproduce the defect:
1) Launch Aoo3.4
2) New a ods file
3) Set "Print Ranges" through menu "Format / Print Ranges / Edit", choose "- user defined -", then give a range
4) Save the ods file, close it
5) Reopen the ods file, close it again

Defect : There is a memory leak in function ScTableSheetObj::PrintAreaUndo_Impl
Comment 1 ChaoHuang 2012-06-27 14:19:06 UTC
The function "ScTableSheetObj::PrintAreaUndo_Impl" is called by these fuctions:
  1)ScTableSheetObj::setPrintAreas
  2)ScTableSheetObj::setPrintTitleColumns
  3)ScTableSheetObj::setTitleColumns
  4)ScTableSheetObj::setPrintTitleRows
  5)ScTableSheetObj::setTitleRows

The argument passed to function "ScTableSheetObj::PrintAreaUndo_Impl" is the same in these five functions. Here is the code snippet.
    ScPrintRangeSaver* pOldRanges = pDoc->CreatePrintRangeSaver();

A new obj will be created on heap in function "ScDocument::CreatePrintRangeSaver". But there is no code to release pOldRanges in these five functions.

In the body of function "ScTableSheetObj::PrintAreaUndo_Impl", there are two places to free obj "pOldRanges".
  1) the "else" branch of "if ( pDocSh )"
     >>> to call "delete pOldRanges" directly
  2) the branch of "if ( pDocSh ) && if (bUndo)"
     >>> to save it in the member data in class ScUndoPrintRange, to free it in the destructor of class ScUndoPrintRange.

So there will be a memory leak about obj "pOldRanges" in the "else" branch of "if (bUndo)". The situation is the same for obj "pNewRanges" in function "ScTableSheetObj::PrintAreaUndo_Impl". The sulution is to free them explicitly.
Comment 2 ChaoHuang 2012-06-27 14:55:50 UTC
Created attachment 78501 [details]
for file "main/sc/source/ui/unoobj/cellsuno.cxx"
Comment 3 Armin Le Grand 2012-06-27 16:19:54 UTC
Created attachment 78504 [details]
Extended patch

ALG: Good catch! No need to create pNewRanges when not in undo, possible to use pOldRanges as flag if used or not and cleanup on central place. Added patch to show this.
It would be even better to use boost::shared_ptr for ScPrintRangeSaver and to derive it from boost::noncopyable (it contains a pointer, when implicitely copied nothing good will happen). Old code, *sigh*.
Comment 4 ChaoHuang 2012-06-27 16:26:00 UTC
(In reply to comment #3)
> Created attachment 78504 [details]
> Extended patch
> 
> ALG: Good catch! No need to create pNewRanges when not in undo, possible to
> use pOldRanges as flag if used or not and cleanup on central place. Added
> patch to show this.
> It would be even better to use boost::shared_ptr for ScPrintRangeSaver and
> to derive it from boost::noncopyable (it contains a pointer, when
> implicitely copied nothing good will happen). Old code, *sigh*.

Thanks for the extended patch ! 
Armin, I learn a lot from you. Thanks!
Comment 5 Armin Le Grand 2012-06-28 09:00:06 UTC
ALG->ChaoHuang: Any more quesitons, else I will commit. Thanks!
Comment 6 ChaoHuang 2012-06-28 22:00:30 UTC
(In reply to comment #5)
> ALG->ChaoHuang: Any more quesitons, else I will commit. Thanks!

hi, Armin
Please go ahead to commit the extended. I tested the code. It's OK. Thanks!
Comment 7 Armin Le Grand 2012-06-29 08:55:47 UTC
ALG->ChaoHuang: Thanks for testing and taking a look. Preparing commit.
Comment 8 Armin Le Grand 2012-06-29 09:20:25 UTC
ALG: Comitted as 0. Thanks for the patch!
Comment 9 ChaoHuang 2012-10-17 08:16:30 UTC
Suggest to put it into AOO 3.5.0 release
Comment 10 Yan Ji 2012-11-30 04:46:43 UTC
Since last SVT(r1400866) shows there is no memory leak, so close this defect as resolved.