Issue 113608 - animations: All animation nodes are leaked
animations: All animation nodes are leaked
Status: RESOLVED FIXED
Product: Draw
Classification: Application
Component: code
OOO310m11
All All
: P2 trivial (vote)
: 4.0.0
Assigned To: zhang jianfang
issues@graphics
:
Depends on:
Blocks: 120975
  Show dependency treegraph
 
Reported: 2010-08-03 09:25 UTC by zhang jianfang
Modified: 2013-07-10 15:04 UTC (History)
3 users (show)

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


Attachments
patch to fix the animation leak problem, but there still is other problem (2.61 KB, patch)
2012-06-13 05:29 UTC, zhang jianfang
no flags Details | Diff
Guard against missing SvxShape::mpImle (565 bytes, patch)
2012-06-14 12:49 UTC, Andre
no flags Details | Diff
Release pointer to object list in SdrObject::Free() (695 bytes, patch)
2012-06-14 16:22 UTC, Andre
no flags Details | Diff
Final extra patch for the remaining crash problem (1.42 KB, patch)
2012-06-17 14:43 UTC, zhang jianfang
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this issue.
Description zhang jianfang 2010-08-03 09:25:19 UTC
If you set animation effects to a sd document, you can find that all
AnimationNode nodes are leaked. Depends on how many Animation effects are
created in sd document, the size of memory leak varies. 

The root causes are,

first, wrong AnimationNode::release() implementation.

	// XInterface
	void SAL_CALL AnimationNode::release(  ) throw ()
	{
		OWeakObject::acquire();
	}


second,

Because AnimationNode maintain a childern node list, while each child node also
has a reference back to the parent node,  there is a cyclic loop reference
between them. So when a SdPage object is destoried,  all AnimationNode objects
for it are in fact not freed.
Comment 1 clippka 2010-08-03 19:41:45 UTC
thanks zhangjfibm, nice catch
Comment 2 Martin Hollmichel 2011-03-16 09:43:59 UTC
set target to 3.x since not release relevant for 3.4.
Comment 3 Andre 2012-06-08 08:16:25 UTC
Checked in the fix for the first part.  SVN revision is 1347931.
Comment 4 zhang jianfang 2012-06-08 08:50:22 UTC
Thanks Andre.
Good to see new update on this old defect. I will try to see if the second part problem still exist and will provide a fix patch late.
Comment 5 zhang jianfang 2012-06-13 05:29:39 UTC
Created attachment 78277 [details]
patch to fix the animation leak problem, but there still is other problem

I have a patch for animation memory leak problem, but unfortunately it exposes another serious crash problem. I think the crash problem exists before too, but it shows up when AnimationNode objects are correctly released.

The crash happens when closing the sd documents and releasing the SdPage objects. The crash call stack is,

 	svxcore.dll!SvxShape::HasSdrObjectOwnership()  Line 299 + 0x6 bytes	C++
 	svxcore.dll!SdrObject::Free(SdrObject * & _rpObject=0x00000000)  Line 489 + 0xe bytes	C++
 	svxcore.dll!SdrObjList::Clear()  Line 271 + 0x9 bytes	C++
 	svxcore.dll!SdrObjList::~SdrObjList()  Line 144	C++
 	svxcore.dll!SdrPage::~SdrPage()  Line 1424 + 0x6c bytes	C++
 	svxcore.dll!FmFormPage::~FmFormPage()  Line 123 + 0x22 bytes	C++
 	sd.dll!SdPage::~SdPage()  Line 161 + 0x149 bytes	C++
 	sd.dll!SdPage::`vector deleting destructor'()  + 0x50 bytes	C++
 	svxcore.dll!SdrModel::DeletePage()  + 0x18 bytes	C++
 	sd.dll!SdDrawDocument::DeletePage()  + 0xf bytes	C++
 	svxcore.dll!SdrModel::ClearModel()  + 0x2a bytes	C++
 	sd.dll!SdDrawDocument::~SdDrawDocument()  + 0xa5 bytes	C++

The exception shows SdrObject::Free() still tries to access SvxShape object after it is deleted. The code snap is,

void SdrObject::Free( SdrObject*& _rpObject )
{
    SdrObject* pObject = _rpObject; _rpObject = NULL;
    if ( pObject == NULL )
        // nothing to do
        return;

	SvxShape* pShape = pObject->getSvxShape();
  if ( pShape && pShape->HasSdrObjectOwnership() )
        // only the shape is allowed to delete me, and will reset the ownership before doing so
        return;

    delete pObject;
}

And as my check, there has already had protect code in SvxShape destructor api, it will try to reset the reference from SdrObject object by calling mpObj->setUnoShape( NULL, SdrObject::GrantXShapeAccess() ).

SvxShape::~SvxShape() throw()
{
   ...
    if ( mpObj.is() )
        mpObj->setUnoShape( NULL, SdrObject::GrantXShapeAccess() );

	if( HasSdrObjectOwnership() && mpObj.is() )
	{
		mpImpl->mbHasSdrObjectOwnership = false;
		SdrObject* pObject = mpObj.get();
		SdrObject::Free( pObject );
	}

  ...
}

Currently the root cause of this problem is still unclear.
Comment 6 Andre 2012-06-14 12:49:57 UTC
Created attachment 78326 [details]
Guard against missing SvxShape::mpImle

I got one step closer.  SvxShape::HasSdrObjectOwnership() did not check that its implementation object mpImpl was still valid before accessing it.

But this reveals yet another crash.
Comment 7 Andre 2012-06-14 16:22:25 UTC
Created attachment 78329 [details]
Release pointer to object list in SdrObject::Free()

Fix for the remaining crash.

Assignment of an animation to a shape leads to construction of the UNO object.  That in turn prevents the SdrObject::Free() function from deleting the SdrObject when the object list is destroyed that contains the object.  The SdrObject still holds its pointer back to the object list.  Dereferencing this pointer causes the crash.

The attached fix resets the pointer to the object list.
Comment 8 zhang jianfang 2012-06-15 02:26:05 UTC
(In reply to comment #6)
> Created attachment 78326 [details]
> Guard against missing SvxShape::mpImle
> 
> I got one step closer.  SvxShape::HasSdrObjectOwnership() did not check that
> its implementation object mpImpl was still valid before accessing it.
> 
> But this reveals yet another crash.

Here the actual problem is the SvxShape object has already been deleted before accessing, not just because SvShape->mpImpl is null.

If I change SdrObject::getSvxShape() in below way, the crash problem also disappears.

SvxShape* SdrObject::getSvxShape() const
{
    DBG_TESTSOLARMUTEX();
        // retrieving the impl pointer and subsequently using it is not thread-safe, of course, so it needs to be
        // guarded by the SolarMutex

#if OSL_DEBUG_LEVE > 0
    uno::Reference< uno::XInterface > xShape( maWeakUnoShape );
    OSL_ENSURE( !( !xShapeGuard.is() && mpSvxShape ),
        "SdrObject::getSvxShape: still having IMPL-Pointer to dead object!" );
#endif
	uno::Reference< uno::XInterface > xShape( maWeakUnoShape );  // zhangjf
	if (!xShape.is()&& mpSvxShape)                               // zhangjf
		return NULL;                                         // zhangjf

	return mpSvxShape;
}

In normal case, mpSvxShape and maWeakUnoShape should refer to a same SvxShape object. So it means when accessing mpSvxShape in that case, the referred SvxShape object is an invalid object already. 

If adding tag to the deleted SvxShape object, we can also observe that the referred SvxShape object has the deleted tag.
Comment 9 Andre 2012-06-15 08:03:45 UTC
You found the root cause for mpImpl being NULL.  Very good.

If you replace the lines

	uno::Reference< uno::XInterface > xShape( maWeakUnoShape );  // zhangjf
	if (!xShape.is()&& mpSvxShape)                               // zhangjf
		return NULL;                                         // zhangjf

	return mpSvxShape;

with

    if (mpSvxShape!=NULL && maWeakUnoShape.get() == NULL)
        return NULL;
    else
        return mpSvxShape;

then the whole thing becomes even clearer and a little faster (no type conversion of xShape).

Maybe it would be even better to remove the const attribute from the getSvxShape() method and set mpSvxShape to NULL when the weak pointer is gone.  This might prevent errors in other scenarios.
What do you think?
Comment 10 zhang jianfang 2012-06-15 08:40:15 UTC
It is just a work around fix.  The actual root cause why mpSvxShape is not synchronized with weak object maWeakUnoShape is still not finally determined. I think it still needs more study.

In fact, further study shows that the dangling pointer disappears when applying below code change,

--- fix/svx/source/unodraw/unoshape.cxx.org	2012-06-15 10:21:31.971970900 +0800
+++ fix/svx/source/unodraw/unoshape.cxx	2012-06-15 10:22:35.145867400 +0800
@@ -1165,8 +1165,10 @@
 
 	if( bClearMe )
 	{
-		if( !HasSdrObjectOwnership() )
+		if( !HasSdrObjectOwnership() ){
+			mpObj->setUnoShape( NULL, SdrObject::GrantXShapeAccess() );
 			mpObj.reset( NULL );
+		}
 		if ( !mpImpl->mbDisposing )
 			dispose();
 	}

But I am not sure if the fix is safe.  

@Andre, could you please help to confirm too?  Thanks.
Comment 11 Andre 2012-06-15 09:39:52 UTC
Well, as long as SdrObject and SvxShape are two separate classes, everything remains a workaround.  And as long an SdrObject holds its SvxShape as weak pointer, every access to that weak pointer has to be guarded.  The only strange thing is that it appears to be (but I did not check that) that the reference count of the weak pointer goes to zero without the SvxShape being destroyed.

It might be a good idea to remove the mpSvxShape member altogether.

Regarding you proposed change:

 if( !HasSdrObjectOwnership() ){
    mpObj->setUnoShape( NULL, SdrObject::GrantXShapeAccess()
    ...

I am not sure if it is valid to modify mpObj when it is not owned by the SvxShape ie HasSdrObjectOwnership() is false.  It might be OK to make the setUnoShape(NULL...) call when mpObj points back to "this".
Comment 12 zhang jianfang 2012-06-17 14:43:01 UTC
Created attachment 78355 [details]
Final extra patch for the remaining crash problem


The direct cause of the crash is SdrObject.mpSvxShape points to a dangling SvxShape object, while the weakreference SdrObject.maWeakUnoShape is correct.  The root cause is in SvxShape::Notify(), it sometimes reset the pointer SvxShape.mpObj without notifying SdrObject, then in SvxShape destructor it also has no chance to notify SdrObject to reset the mpSvxShape to the SvxShape object.

But it is hard in SvxShape::Notify() to check if the SvxShape.mpObj->mpSvxShape points back to SvxShape object itself, when the SvxShape object doesn't have ownership to the SdrObject.  So the safe and quick fix still gets back to the original workaround. I attach this final fix patch.

SdrObject.mpSvxShape is only for internal use. I guess it is for cache purpose, so it needn't workout the real pointer to SvxShape object each time when it wants to access it.  So I won't try to remove it now.
Comment 13 zhang jianfang 2012-06-18 13:06:21 UTC
Take over the bug to commit the part 2 fix code.
Comment 14 zhang jianfang 2012-06-18 13:35:12 UTC
Committed to trunk by revision 1351332.
Comment 15 Andre 2012-06-19 07:26:42 UTC
Great work.  Very good analysis and I did not even have to check it in myself :)
Thank you very much.
Comment 16 hdu@apache.org 2013-07-10 15:04:08 UTC
adjusted target to version that will contain the fix