Issue 120105 - [From Symphony] There is a memory leak in function ScTableSheetObj::PrintAreaUndo_Impl
Summary: [From Symphony] There is a memory leak in function ScTableSheetObj::PrintArea...
Status: CLOSED FIXED
Alias: None
Product: performance
Classification: Code
Component: code (show other issues)
Version: AOO 3.4.0
Hardware: All All
: P3 Normal (vote)
Target Milestone: 4.0.0
Assignee: AOO issues mailing list
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 120975 121366
  Show dependency tree
 
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 in: ---
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
Add an attachment (proposed patch, testcase, etc.)

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 r1355277. 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.