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.

Created attachment 78457 [details]
for file "main/sc/source/filter/xml/xmlfonte.cxx"

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

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.

Created attachment 78464 [details]
Extended patch to also make safe other places where SfxStyleSheetIterator is used

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

(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"?

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

(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!

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

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!

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

ALG: Argh! Thanks, Ariel, I just overlooked this one, sorry. How good that someone is watching it. I'll take care of it ASAP...

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

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.

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

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

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!

Suggest to put it into AOO 3.5.0 release

Since last SVT(r1400866) shows there is no memory leak, so close this defect as resolved.