Issue 120077 - [From Symphony] There are some memory leaks in function ScXMLFontAutoStylePool_Impl::ScXMLFontAutoStylePool_Impl
[From Symphony] There are some memory leaks in function ScXMLFontAutoStylePoo...
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-25 07:07 UTC by ChaoHuang
Modified: 2013-02-16 09:13 UTC (History)
3 users (show)

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


Attachments
for file "main/sc/source/filter/xml/xmlfonte.cxx" (1.68 KB, patch)
2012-06-25 08:00 UTC, ChaoHuang
chao.dev.h: review?
Details | Diff
Extended patch to also make safe other places where SfxStyleSheetIterator is used (3.72 KB, patch)
2012-06-25 12:58 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-25 07:07:30 UTC
Steps to reproduce the defect:
1) Launch Aoo3.4
2) New a ods file
3) Input some text in a cell
4) Save the ods file, close it

Defect : There is a memory leak in function ScXMLFontAutoStylePool_Impl::ScXMLFontAutoStylePool_Impl
Comment 1 ChaoHuang 2012-06-25 07:17:03 UTC
An unnamed object typed SfxItemPool will be created in heap by calling function "EditEngine::CreatePool()", which will not be released. So it is a memory leak. The solution is to record the unnamed object with member data in class ScXMLFontAutoStylePool_Impl and to free it in destructor.

Another memory leak is from object pItr typed SfxStyleSheetIterator, which is created by calling function "SfxStyleSheetBasePool::CreateIterator". There is no code to free it. So it is also a memory leak. Need to free it explicitly.
Comment 2 ChaoHuang 2012-06-25 08:00:54 UTC
Created attachment 78457 [details]
for file "main/sc/source/filter/xml/xmlfonte.cxx"
Comment 3 Armin Le Grand 2012-06-25 12:45:58 UTC
ALG:_ Checked in sc/source/filter/xml/xmlfonte.cxx that indeed the pool created using EditEngine::CreatePool is not freed and also not given over with owner rights to EditEngine. This is a mem leak and patch is good.

Also checked for SfxStyleSheetIterator* pItr, there are some other cases in sc where this is not freed, too. Checking for whole office...
Comment 4 Armin Le Grand 2012-06-25 12:57:32 UTC
ALG: Checked SfxStyleSheetBasePool::CreateIterator, it indeed returns a newly constructed iterator, giving ownership to the caller. This means all usages need to be secured to delete the used iterator after usage.

Found three other places in the AOO code which do not free a SfxStyleSheetIterator, added code to them to free it. Adding new patch.
Comment 5 Armin Le Grand 2012-06-25 12:58:19 UTC
Created attachment 78464 [details]
Extended patch to also make safe other places where SfxStyleSheetIterator is used
Comment 6 ChaoHuang 2012-06-25 18:23:30 UTC
(In reply to comment #5)
> Created attachment 78464 [details]
> Extended patch to also make safe other places where SfxStyleSheetIterator is
> used

hi, Armin

Thanks for the extend patch in the whole office. The newly object from function "SfxStyleSheetBasePool::CreateIterator" should be freed manually in all the places.
Comment 7 ChaoHuang 2012-06-25 19:25:43 UTC
(In reply to comment #5)
> Created attachment 78464 [details]
> Extended patch to also make safe other places where SfxStyleSheetIterator is
> used

For the changes in file "sc/source/core/data/patattr.cxx" and "sc/source/core/data/stlsheet.cxx", I think that there will be more investigation on the impact on code like :
	pStyle = pIter->First();
        delete pIter;

Is pStyle invalid after calling "delete pIter"?
Comment 8 Armin Le Grand 2012-06-26 08:36:07 UTC
ALG->ChaoHuang: pStyle is valid. SfxStyleSheetIterator is tooling to iterate over existing styles and hands out pointers to existing styles. SfxStyleSheetBase is even refcounted, so could be hold in rtl::Reference< SfxStyleSheetBase > when the danger exists that the SfxStyleSheetBase may be deleted while accessing it. This is not the case here, so using SfxStyleSheetBase* pStyle is sufficient. It would e.g. be deleted when the StyleSheetPool gets deleted and is the last instance holding a reference to the StyleSheet.
More questions (always welcome) or ready to apply this..?
Comment 9 ChaoHuang 2012-06-26 08:56:23 UTC
(In reply to comment #8)
> ALG->ChaoHuang: pStyle is valid. SfxStyleSheetIterator is tooling to iterate
> over existing styles and hands out pointers to existing styles.
> SfxStyleSheetBase is even refcounted, so could be hold in rtl::Reference<
> SfxStyleSheetBase > when the danger exists that the SfxStyleSheetBase may be
> deleted while accessing it. This is not the case here, so using
> SfxStyleSheetBase* pStyle is sufficient. It would e.g. be deleted when the
> StyleSheetPool gets deleted and is the last instance holding a reference to
> the StyleSheet.
> More questions (always welcome) or ready to apply this..?

Thanks for the deep analysis. For my view, the function "SfxStyleSheetBasePool::CreateIterator" is just a tool to reduce the range to find out specific style in SfxStyleSheetBasePool. I agree with your extended patch. Thanks!
Comment 10 Armin Le Grand 2012-06-26 11:19:16 UTC
ALG: in principle, the whole SfxStyleSheetBasePool::CreateIterator is not needed; a SfxStyleSheetIterator can be created by everyone, e.g. as local variable (so it cannot be easily forgotten to be cleaned up). It would only make sense if SfxStyleSheetBasePool::CreateIterator would somehow have exclusive construction rights and somehow wants to control/remember existing instances. Checking removal...
Comment 11 Armin Le Grand 2012-06-26 13:37:30 UTC
ALG: Checked, removal is possible and leads to only one place left where SfxStyleSheetIterator is used as pointered instance, all others local variables. Checked in as r1354011, thanks for the patch!
Comment 12 Ariel Constenla-Haile 2012-06-28 10:52:54 UTC
(In reply to comment #10)
> ALG: in principle, the whole SfxStyleSheetBasePool::CreateIterator is not
> needed; a SfxStyleSheetIterator can be created by everyone

This breaks the styles in Writer:

- Create a new Writer document
- Press F11
- There are no styles for any style family

----------------------------------------------------------------
The UNO layer is affected too.
Run the following macro:

REM  *****  BASIC  *****

Option Explicit

Sub Main
	Dim oDoc as Object
	oDoc = ThisComponent
	
	Dim sStyleFamilies$()
	Dim oStyleFamilies as Object
	oStyleFamilies = oDoc.getStyleFamilies()
	sStyleFamilies = oStyleFamilies.getElementNames()
	
	Dim sStyleFamily$
	Dim oFamily as Object
	For Each sStyleFamily in sStyleFamilies
		oFamily = oStyleFamilies.getByName(sStyleFamily)
		
		Dim sStyleNames$()
		Dim sStyle$, sStyleName$
		Dim oStyle as Object
		sStyleNames = oFamily.getElementNames()
		For Each sStyle In sStyleNames
			oStyle = oFamily.getByName(sStyle)
			sStyleName = oStyle.Name
		Next
	Next
End Sub

Style families are empty

----------------------------------------------------------------

You cannot simply replace SfxStyleSheetBasePool::CreateIterator with creating a SfxStyleSheetIterator.
Writer sub-classes SfxStyleSheetBasePool in SwDocStyleSheetPool and overrides this function to return an SwStyleSheetIterator, derived from SfxStyleSheetIterator; and obviously SfxStyleSheetIterator and SwStyleSheetIterator are not exchangeable (without breaking functionality, like in this case).

This happens in SwXStyleFamily::getElementNames(), where the pBasePool is likely (== I didn't debug this code) an SwDocStyleSheetPool and the iterator should be a SwStyleSheetIterator, not a SfxStyleSheetIterator. This is why the UNO layer is broken.

And it happens in the "Style and Formatting" window, in all the code in sfx2/source/dialog/templdlg.cxx the SfxStyleSheetBasePool is really a SwDocStyleSheetPool when we have a Writer document:

in void SfxCommonTemplateDialog_Impl::Update_Impl() 
- SfxObjectShell is really a SwDocShell
- pDocShell->GetStyleSheetPool() will be returning an SwDocStyleSheetPool (SfxStyleSheetBasePool*	SwDocShell::GetStyleSheetPool())
- this means SfxStyleSheetBasePool::GetIterator_Impl cannot simply create a SfxStyleSheetIterator, and CreateIterator was there with the idea to return a SwDocStyleSheetPool in this case.
Comment 13 Armin Le Grand 2012-06-28 12:31:42 UTC
ALG: Argh! Thanks, Ariel, I just overlooked this one, sorry. How good that someone is watching it. I'll take care of it ASAP...
Comment 14 Armin Le Grand 2012-06-28 14:22:18 UTC
ALG: I have reverted the patch and follow another strategy: Let all CreateIterator usages return a boost::shared_ptr instance. This makes clear that the ownership changes and allows deletion of the SfxStyleSheetIterator in all cases cleanly when the last instance of SfxStyleSheetIteratorPtr (the boost typedef) is released. Building incompatible for testing...
Comment 15 Armin Le Grand 2012-06-28 17:30:43 UTC
ALG: Reverted r1354011, adapted all usages of SfxStyleSheetIterator and CreateIterator to use boost::shared_ptr. Built and checked, works as expected. Committed as r.1355082.
Sorry for the delay, but after updating trunk I stumbled over the 340->350 change and had to make a clean rebuild.
Comment 16 ChaoHuang 2012-06-29 02:50:29 UTC
(In reply to comment #12)
> (In reply to comment #10)
> > ALG: in principle, the whole SfxStyleSheetBasePool::CreateIterator is not
> > needed; a SfxStyleSheetIterator can be created by everyone
> 

confirmed the issue. Thanks for your testing and reporting!

> This breaks the styles in Writer:
> 
> - Create a new Writer document
> - Press F11
> - There are no styles for any style family
> 
> ----------------------------------------------------------------
> The UNO layer is affected too.
> Run the following macro:
> 
> REM  *****  BASIC  *****
> 
> Option Explicit
> 
> Sub Main
> 	Dim oDoc as Object
> 	oDoc = ThisComponent
> 	
> 	Dim sStyleFamilies$()
> 	Dim oStyleFamilies as Object
> 	oStyleFamilies = oDoc.getStyleFamilies()
> 	sStyleFamilies = oStyleFamilies.getElementNames()
> 	
> 	Dim sStyleFamily$
> 	Dim oFamily as Object
> 	For Each sStyleFamily in sStyleFamilies
> 		oFamily = oStyleFamilies.getByName(sStyleFamily)
> 		
> 		Dim sStyleNames$()
> 		Dim sStyle$, sStyleName$
> 		Dim oStyle as Object
> 		sStyleNames = oFamily.getElementNames()
> 		For Each sStyle In sStyleNames
> 			oStyle = oFamily.getByName(sStyle)
> 			sStyleName = oStyle.Name
> 		Next
> 	Next
> End Sub
> 
> Style families are empty
> 
> ----------------------------------------------------------------
> 
> You cannot simply replace SfxStyleSheetBasePool::CreateIterator with
> creating a SfxStyleSheetIterator.
> Writer sub-classes SfxStyleSheetBasePool in SwDocStyleSheetPool and
> overrides this function to return an SwStyleSheetIterator, derived from
> SfxStyleSheetIterator; and obviously SfxStyleSheetIterator and
> SwStyleSheetIterator are not exchangeable (without breaking functionality,
> like in this case).
> 
> This happens in SwXStyleFamily::getElementNames(), where the pBasePool is
> likely (== I didn't debug this code) an SwDocStyleSheetPool and the iterator
> should be a SwStyleSheetIterator, not a SfxStyleSheetIterator. This is why
> the UNO layer is broken.
> 
> And it happens in the "Style and Formatting" window, in all the code in
> sfx2/source/dialog/templdlg.cxx the SfxStyleSheetBasePool is really a
> SwDocStyleSheetPool when we have a Writer document:
> 
> in void SfxCommonTemplateDialog_Impl::Update_Impl() 
> - SfxObjectShell is really a SwDocShell
> - pDocShell->GetStyleSheetPool() will be returning an SwDocStyleSheetPool
> (SfxStyleSheetBasePool*	SwDocShell::GetStyleSheetPool())
> - this means SfxStyleSheetBasePool::GetIterator_Impl cannot simply create a
> SfxStyleSheetIterator, and CreateIterator was there with the idea to return
> a SwDocStyleSheetPool in this case.

Thanks for your analysis ! It's very useful for us to solver the problem.
Comment 17 ChaoHuang 2012-06-29 02:58:53 UTC
(In reply to comment #14)
> ALG: I have reverted the patch and follow another strategy: Let all
> CreateIterator usages return a boost::shared_ptr instance. This makes clear
> that the ownership changes and allows deletion of the SfxStyleSheetIterator
> in all cases cleanly when the last instance of SfxStyleSheetIteratorPtr (the
> boost typedef) is released. Building incompatible for testing...

It seems like that using smart pointer (e.g boost::shared_ptr) is the best way to solve such an "easy-forgot-to-release" issue. I will try to follow this way in future.

Another memory leak is from an unnamed heap object created by "EditEngine::CreatePool()". It is still in the codebase after reverted R1354011 and committed R1355082.
Comment 18 Armin Le Grand 2012-06-29 09:29:40 UTC
ALG->ChaoHuang: Yes, you are right. Thanks for pointing out, the original reason for this task was nearly ignored by incerting the patch. Added and checked the local variable for the EditEngine's SfxItemPool.
Comitted as r1355288, setting task to resolved not. Thanks to all who helped and had a look at it, opensource rules!
Comment 19 ChaoHuang 2012-10-17 08:15:16 UTC
Suggest to put it into AOO 3.5.0 release
Comment 20 Yan Ji 2012-11-30 04:46:27 UTC
Since last SVT(r1400866) shows there is no memory leak, so close this defect as resolved.