Apache OpenOffice (AOO) Bugzilla – Issue 113608
animations: All animation nodes are leaked
Last modified: 2022-10-28 12:54:31 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.
thanks zhangjfibm, nice catch
set target to 3.x since not release relevant for 3.4.
Checked in the fix for the first part. SVN revision is 1347931.
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.
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.
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.
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.
(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.
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?
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.
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".
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.
Take over the bug to commit the part 2 fix code.
Committed to trunk by revision 1351332.
Great work. Very good analysis and I did not even have to check it in myself :) Thank you very much.
adjusted target to version that will contain the fix