Issue 120106 - [From Symphony] There is a memory leak in function SwHTMLWriter::CollectFlyFrms
Summary: [From Symphony] There is a memory leak in function SwHTMLWriter::CollectFlyFrms
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 15:51 UTC by ChaoHuang
Modified: 2013-02-16 09:10 UTC (History)
4 users (show)

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


Attachments
for file "main/sw/source/filter/html/htmlfly.cxx" (515 bytes, patch)
2012-06-27 16:07 UTC, ChaoHuang
no flags Details | Diff
Extended patch (10.24 KB, patch)
2012-06-28 12:54 UTC, Armin Le Grand
no flags Details | Diff
for file "main/sw/source/filter/ww8/writerhelper.cxx" (1000 bytes, patch)
2012-06-29 17:30 UTC, ChaoHuang
awf.aoo: review+
Details | Diff

Note You need to log in before you can comment on or make changes to this issue.
Description ChaoHuang 2012-06-27 15:51:35 UTC
Steps to reproduce the defect:
1) Launch Aoo3.4
2) New a odt file
3) Insert a customer shape, like a rectangle shape
4) Save the odt file as "HTML Document (OpenOffice.org Writer)"
5) Close it

Defect : There is a memory leak in function SwHTMLWriter::CollectFlyFrms
Comment 1 ChaoHuang 2012-06-27 15:59:53 UTC
There is a local container aFlyPos with type SwPosFlyFrms declared in the beginning of function "SwHTMLWriter::CollectFlyFrms". It will call function "SwDoc::GetAllFlyFmts" to fill the container. All the elements in the container are created on heap. But they are not deleted in any place. So it is a memory leak. The solution is to release all the element in the container.
Comment 2 ChaoHuang 2012-06-27 16:07:56 UTC
Created attachment 78503 [details]
for file "main/sw/source/filter/html/htmlfly.cxx"
Comment 3 Armin Le Grand 2012-06-28 11:34:54 UTC
ALG SwPosFlyFrms and GetAllFlyFmts is not only used in htmlfly.cxx, but also in unoobj2.cxx and writerhelper.cxx. Only in writerhelper.cxx is already a freeing mechanism, thus the mem leak is also for unoobj2.cxx.
In all cases the list is filled by calling GetAllFlyFmts, so ideal would be a helper class to hold the results which does automatic cleanups on destruction. Investigating further...
Comment 4 Armin Le Grand 2012-06-28 12:54:38 UTC
Created attachment 78515 [details]
Extended patch

ALG->ChaoHuang: I replaced the old SV_PTR_* stuff with stl usage and secured all usages, please have a look. Thanks!
Comment 5 ChaoHuang 2012-06-29 04:08:01 UTC
(In reply to comment #4)
> Created attachment 78515 [details]
> Extended patch
> 
> ALG->ChaoHuang: I replaced the old SV_PTR_* stuff with stl usage and secured
> all usages, please have a look. Thanks!

There are three places to call function "SwDoc::GetAllFlyFmts"
  1)SwXParaFrameEnumeration::SwXParaFrameEnumeration    in unoobj2.cxx  
  2)sw.util.GetFrames    in writerhelper.cxx
  3)SwHTMLWriter::CollectFlyFrms()    in htmlfly.cxx

There is always a definition for a local SwPosFlyFrms object before calling "SwDoc::GetAllFlyFmts". Then the reference of the local SwPosFlyFrms object will be passed into "SwDoc::GetAllFlyFmts" to fill with the result. So it is OK to return a local SwPosFlyFrms obj in function "SwDoc::GetAllFlyFmts" and to use it to create a SwPosFlyFrms  obj. So I agree to change the header for function "SwDoc::GetAllFlyFmts" 
from
      void SwDoc::GetAllFlyFmts( SwPosFlyFrms& rPosFlyFmts,
                                 const SwPaM* pCmpRange, 
                                 sal_Bool bDrawAlso ) const
to
      SwPosFlyFrms SwDoc::GetAllFlyFmts( const SwPaM* pCmpRange, 
                                         sal_Bool bDrawAlso ) const

I have one question about function "SwPosFlyFrmCmp::operator()" in new file
According to the code in previous function "SwPosFlyFrm::operator<"

sal_Bool SwPosFlyFrm::operator<( const SwPosFlyFrm& rPosFly )
{
	if( pNdIdx->GetIndex() == rPosFly.pNdIdx->GetIndex() )
	{
		// dann entscheidet die Ordnungsnummer!
		return nOrdNum < rPosFly.nOrdNum;
	}
	return pNdIdx->GetIndex() < rPosFly.pNdIdx->GetIndex();
}

Is it enough to only compare "index" in new function "SwPosFlyFrmCmp::operator()"

bool SwPosFlyFrmCmp::operator()(const SwPosFlyFrmPtr& rA, const SwPosFlyFrmPtr& rB) const 
{ 
    return rA->GetNdIndex() < rB->GetNdIndex(); 
}

Should it be
bool SwPosFlyFrmCmp::operator()(const SwPosFlyFrmPtr& rA, const SwPosFlyFrmPtr& rB) const 
{
    if (rA->GetNdIndex() == rB->GetNdIndex())
    {
         return rA->GetOrdNum() < rB->GetOrdNum();
    }

    return rA->GetNdIndex() < rB->GetNdIndex(); 
}

I think that we should keep the existing logic as many as we can in only replacing the STL container.

Thanks!
Comment 6 Armin Le Grand 2012-06-29 08:58:08 UTC
ALG->ChaoHuang: Thank you very much for your review, of course you are right, I oversaw it. I'm so happy that we do reviews, it can be seen why :-) Changing that.
Besides that, have you tested and/or other comments?
Comment 7 Armin Le Grand 2012-06-29 12:41:09 UTC
ALG: Comitted extended patch with corrected operator as r.1355342. Thanks for the patch and reviewing!
Keeping opened, please have a look and set it to closed when okay, I'll be on vacation the next two weeks.
Comment 8 ChaoHuang 2012-06-29 14:52:11 UTC
(In reply to comment #7)
> ALG: Comitted extended patch with corrected operator as r.1355342. Thanks
> for the patch and reviewing!
> Keeping opened, please have a look and set it to closed when okay, I'll be
> on vacation the next two weeks.

hi, Armin
I'm doing testing for the extended patch. It will be finished in half an hour. I will update the result. Thanks!
Comment 9 ChaoHuang 2012-06-29 16:23:10 UTC
(In reply to comment #7)
> ALG: Comitted extended patch with corrected operator as r.1355342. Thanks
> for the patch and reviewing!
> Keeping opened, please have a look and set it to closed when okay, I'll be
> on vacation the next two weeks.

hi, Armin
I merged the code manually to my local dev env. It works well. The SwPosFlyFrm obj is created, then being released. There is no such kind of memory leak now. Thanks for your great work !

I have one question about the extended path in R1355342.
In function "sw::Frames SwPosFlyFrmsToFrames(const SwPosFlyFrms &rFlys)", the name of iterator in the loop is "aPos", which is the same as the one in "else" branch. It will make developer confused. Please refer to the code snippet.
  SwPosition aPos((*aPos)->GetNdIndex());

So I suggest to change the iterator name from "aPos" to "aIter". The code may be like this :

    sw::Frames SwPosFlyFrmsToFrames(const SwPosFlyFrms &rFlys)
    {
        sw::Frames aRet;
        
        for(SwPosFlyFrms::const_iterator aIter(rFlys.begin()); aPos != rFlys.end(); aPos++)
        {
            const SwFrmFmt &rEntry = (*aIter)->GetFmt();

            if (const SwPosition* pAnchor = rEntry.GetAnchor().GetCntntAnchor())
			{
                aRet.push_back(sw::Frame(rEntry, *pAnchor));
			}
            else
            {
                SwPosition aPos((*aIter)->GetNdIndex());

                if (SwTxtNode* pTxtNd = aPos.nNode.GetNode().GetTxtNode())
                {
                    aPos.nContent.Assign(pTxtNd, 0);
                }
                
                aRet.push_back(sw::Frame(rEntry, aPos));
            }
        }
        return aRet;
    }
Comment 10 ChaoHuang 2012-06-29 16:25:18 UTC
(In reply to comment #7)
> ALG: Comitted extended patch with corrected operator as r.1355342. Thanks
> for the patch and reviewing!
> Keeping opened, please have a look and set it to closed when okay, I'll be
> on vacation the next two weeks.

hi, Armin
I merged the code manually to my local dev env. It works well. The SwPosFlyFrm obj is created, then being released. There is no such kind of memory leak now. Thanks for your great work !

I have one question about the extended path in R1355342.
In function "sw::Frames SwPosFlyFrmsToFrames(const SwPosFlyFrms &rFlys)", the name of iterator in the loop is "aPos", which is the same as the one in "else" branch. It will make developer confused. Please refer to the code snippet.
  SwPosition aPos((*aPos)->GetNdIndex());

So I suggest to change the iterator name from "aPos" to "aIter". The code may be like this :

    sw::Frames SwPosFlyFrmsToFrames(const SwPosFlyFrms &rFlys)
    {
        sw::Frames aRet;
        
        for(SwPosFlyFrms::const_iterator aIter(rFlys.begin()); aIter!= rFlys.end(); aIter++)
        {
            const SwFrmFmt &rEntry = (*aIter)->GetFmt();

            if (const SwPosition* pAnchor = rEntry.GetAnchor().GetCntntAnchor())
			{
                aRet.push_back(sw::Frame(rEntry, *pAnchor));
			}
            else
            {
                SwPosition aPos((*aIter)->GetNdIndex());

                if (SwTxtNode* pTxtNd = aPos.nNode.GetNode().GetTxtNode())
                {
                    aPos.nContent.Assign(pTxtNd, 0);
                }
                
                aRet.push_back(sw::Frame(rEntry, aPos));
            }
        }
        return aRet;
    }
Comment 11 Armin Le Grand 2012-06-29 17:14:03 UTC
Hi Chao Huang,
sound reasonable, go ahead with it. I will be off for two weeks vacation, so plese take over. You know what you are doing!
Comment 12 ChaoHuang 2012-06-29 17:30:26 UTC
Created attachment 78522 [details]
for file "main/sw/source/filter/ww8/writerhelper.cxx"

To rename the iterator, which can be distinguished from a local var.
Comment 13 Ariel Constenla-Haile 2012-06-30 09:46:56 UTC
(In reply to comment #10)
> I have one question about the extended path in R1355342.
> In function "sw::Frames SwPosFlyFrmsToFrames(const SwPosFlyFrms &rFlys)",
> the name of iterator in the loop is "aPos", which is the same as the one in
> "else" branch. It will make developer confused. 

This issues a warning on Linux:

/home/buildslave20/slave20/openoffice-linux32-nightly/build/main/sw/source/filter/ww8/writerhelper.cxx: In function 'sw::Frames {anonymous}::SwPosFlyFrmsToFrames(const SwPosFlyFrms&)':
/home/buildslave20/slave20/openoffice-linux32-nightly/build/main/sw/source/filter/ww8/writerhelper.cxx:154:32: warning: declaration of 'aPos' shadows a previous local [-Wshadow]
/home/buildslave20/slave20/openoffice-linux32-nightly/build/main/sw/source/filter/ww8/writerhelper.cxx:144:42: warning: shadowed declaration is here [-Wshadow]

And the commit breaks the build:

/home/buildslave20/slave20/openoffice-linux32-nightly/build/main/sw/source/filter/ww8/writerhelper.cxx:154:35: error: no match for 'operator*' in '*aPos'
/home/buildslave20/slave20/openoffice-linux32-nightly/build/main/sw/source/filter/ww8/writerhelper.cxx:154:35: note: candidates are:
/home/buildslave20/slave20/openoffice-linux32-nightly/build/main/solver/350/unxlngi6.pro/inc/tools/gen.hxx:228:14: note: Point operator*(const Point&, long int)
/home/buildslave20/slave20/openoffice-linux32-nightly/build/main/solver/350/unxlngi6.pro/inc/tools/gen.hxx:228:14: note:   candidate expects 2 arguments, 1 provided
/home/buildslave20/slave20/openoffice-linux32-nightly/build/main/solver/350/unxlngi6.pro/inc/tools/fract.hxx:132:17: note: Fraction operator*(const Fraction&, const Fraction&)
/home/buildslave20/slave20/openoffice-linux32-nightly/build/main/solver/350/unxlngi6.pro/inc/tools/fract.hxx:132:17: note:   candidate expects 2 arguments, 1 provided
make: *** [/home/buildslave20/slave20/openoffice-linux32-nightly/build/main/solver/350/unxlngi6.pro/workdir/CxxObject/sw/source/filter/ww8/writerhelper.o] Error 1
make: *** Waiting for unfinished jobs....
dmake:  Error code 2, while making 'all'
Comment 14 Armin Le Grand 2012-06-30 13:35:39 UTC
@Ariel: Could you please add and commit the patch of Chao Huang which changes the variable naming leading to conflicts, I'm out of reach of a development machine. Thanks in advance!
Comment 15 Ariel Constenla-Haile 2012-06-30 23:02:57 UTC
(In reply to comment #14)
> @Ariel: Could you please add and commit the patch of Chao Huang which
> changes the variable naming leading to conflicts, I'm out of reach of a
> development machine. Thanks in advance!

Done. Committed revision 1355833
Comment 16 ChaoHuang 2012-07-01 05:35:33 UTC
(In reply to comment #15)
> (In reply to comment #14)
> > @Ariel: Could you please add and commit the patch of Chao Huang which
> > changes the variable naming leading to conflicts, I'm out of reach of a
> > development machine. Thanks in advance!
> 
> Done. Committed revision 1355833

hi,Ariel
Thanks for your finding and help to commit the patch.
Comment 17 Andre 2012-07-09 15:21:54 UTC
Comment on attachment 78503 [details]
for file "main/sw/source/filter/html/htmlfly.cxx"

Superseded by other patch.
Comment 18 Andre 2012-07-09 15:22:27 UTC
Comment on attachment 78522 [details]
for file "main/sw/source/filter/ww8/writerhelper.cxx"

Reviewed: OK
Comment 19 Andre 2012-07-09 15:23:37 UTC
Setting status to fixed.
Comment 20 ChaoHuang 2012-10-17 08:17:23 UTC
Suggest to put it into AOO 3.5.0 release
Comment 21 Yan Ji 2012-11-30 04:47:38 UTC
Since last SVT(r1400866) shows there is no memory leak, so close this defect as resolved.