From: Nick Holland Date: Thu, 16 Feb 2017 09:21:52 +0000 (+0000) Subject: [3.0] Fix memory leak, scene graph layers are never deleted from memory X-Git-Tag: accepted/tizen/3.0/common/20170303.085417~1 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=refs%2Fchanges%2F61%2F116661%2F1;p=platform%2Fcore%2Fuifw%2Fdali-core.git [3.0] Fix memory leak, scene graph layers are never deleted from memory Reported by Valgrind. We have a memory pool for Nodes which works fine for creating /deleting nodes. Scene Graph Layers inherit from Nodes, but don't use the memory pool. However when either a Node or a Layer is deleted, the overloaded Node operator delete( void* ptr ) is called, which tries to free the Node from the memory pool. Unfortunately for Layer it was never in the memory pool so no memory is free'd. Fix includes: node destructor is protected layer destructor is private node delete operator removed Change-Id: Icfe83f35b4f29d5b774cde392ff976ad299481c5 --- diff --git a/dali/devel-api/common/owner-container.h b/dali/devel-api/common/owner-container.h index 108d1a4..3ac0df2 100644 --- a/dali/devel-api/common/owner-container.h +++ b/dali/devel-api/common/owner-container.h @@ -78,7 +78,7 @@ public: */ Iterator Erase( Iterator position ) { - delete (*position); + Delete (*position); return Vector< T >::Erase( position ); } @@ -103,7 +103,7 @@ public: ConstIterator end = Vector< T >::End(); for( Iterator iter = Vector< T >::Begin(); iter != end; ++iter ) { - delete (*iter); + Delete (*iter); } Vector< T >::Clear(); } @@ -120,7 +120,7 @@ public: ConstIterator end = Vector< T >::End(); for( Iterator iter = Vector< T >::Begin() + size; iter != end; ++iter ) { - delete (*iter); + Delete (*iter); } } Vector< T >::Resize( size ); @@ -166,6 +166,17 @@ private: // Undefined assignment operator. OwnerContainer& operator=( const OwnerContainer& ); + /** + * @brief delete the contents of the pointer + * Function provided to allow classes to provide a custom destructor through template specialisation + * @param pointer to the object + */ + void Delete( T pointer ) + { + delete pointer; + } + + }; } // namespace Dali diff --git a/dali/internal/update/common/discard-queue.h b/dali/internal/update/common/discard-queue.h index a8a35ef..7619119 100644 --- a/dali/internal/update/common/discard-queue.h +++ b/dali/internal/update/common/discard-queue.h @@ -120,13 +120,13 @@ private: RenderQueue& mRenderQueue; ///< Used to send GL clean-up messages for the next Render. // Messages are queued here when the update buffer index == 0 - NodeOwnerContainer mNodeQueue0; + OwnerContainer< Node* > mNodeQueue0; ShaderQueue mShaderQueue0; RendererQueue mRendererQueue0; CameraQueue mCameraQueue0; // Messages are queued here when the update buffer index == 1 - NodeOwnerContainer mNodeQueue1; + OwnerContainer< Node* > mNodeQueue1; ShaderQueue mShaderQueue1; RendererQueue mRendererQueue1; CameraQueue mCameraQueue1; diff --git a/dali/internal/update/manager/update-manager.cpp b/dali/internal/update/manager/update-manager.cpp index 951c4c5..d060699 100644 --- a/dali/internal/update/manager/update-manager.cpp +++ b/dali/internal/update/manager/update-manager.cpp @@ -191,7 +191,7 @@ struct UpdateManager::Impl for(;iter!=endIter;++iter) { (*iter)->OnDestroy(); - delete(*iter); + Node::Delete(*iter); } // If there is root, reset it, otherwise do nothing as rendering was never started @@ -199,7 +199,7 @@ struct UpdateManager::Impl { root->OnDestroy(); - delete root; + Node::Delete( root ); root = NULL; } @@ -207,7 +207,7 @@ struct UpdateManager::Impl { systemLevelRoot->OnDestroy(); - delete systemLevelRoot; + Node::Delete( systemLevelRoot ); systemLevelRoot = NULL; } diff --git a/dali/internal/update/nodes/node-declarations.h b/dali/internal/update/nodes/node-declarations.h index 853cb59..131ba19 100644 --- a/dali/internal/update/nodes/node-declarations.h +++ b/dali/internal/update/nodes/node-declarations.h @@ -34,8 +34,6 @@ namespace SceneGraph class Node; -typedef OwnerContainer< Node* > NodeOwnerContainer; - typedef Dali::Vector< Node* > NodeContainer; typedef NodeContainer::Iterator NodeIter; typedef NodeContainer::ConstIterator NodeConstIter; diff --git a/dali/internal/update/nodes/node.cpp b/dali/internal/update/nodes/node.cpp index 1465543..ebf6565 100644 --- a/dali/internal/update/nodes/node.cpp +++ b/dali/internal/update/nodes/node.cpp @@ -27,7 +27,8 @@ namespace //Unnamed namespace { -//Memory pool used to allocate new nodes. Memory used by this pool will be released when shutting down DALi +//Memory pool used to allocate new nodes. Memory used by this pool will be released when process dies +// or DALI library is unloaded Dali::Internal::MemoryPoolObjectAllocator gNodeMemoryPool; } @@ -43,11 +44,30 @@ namespace SceneGraph const PositionInheritanceMode Node::DEFAULT_POSITION_INHERITANCE_MODE( INHERIT_PARENT_POSITION ); const ColorMode Node::DEFAULT_COLOR_MODE( USE_OWN_MULTIPLY_PARENT_ALPHA ); + Node* Node::New() { return new ( gNodeMemoryPool.AllocateRawThreadSafe() ) Node(); } +void Node::Delete( Node* node ) +{ + // check we have a node not a layer + if( !node->mIsLayer ) + { + // Manually call the destructor + node->~Node(); + + // Mark the memory it used as free in the memory pool + gNodeMemoryPool.FreeThreadSafe( node ); + } + else + { + // not in the pool, just delete it. + delete node; + } +} + Node::Node() : mTransformManager(0), mTransformId( INVALID_TRANSFORM_ID ), @@ -75,7 +95,8 @@ Node::Node() mDrawMode( DrawMode::NORMAL ), mColorMode( DEFAULT_COLOR_MODE ), mClippingMode( ClippingMode::DISABLED ), - mIsRoot( false ) + mIsRoot( false ), + mIsLayer( false ) { mUniformMapChanged[0] = 0u; mUniformMapChanged[1] = 0u; @@ -89,11 +110,6 @@ Node::~Node() } } -void Node::operator delete( void* ptr ) -{ - gNodeMemoryPool.FreeThreadSafe( static_cast( ptr ) ); -} - void Node::OnDestroy() { // Animators, Constraints etc. should be disconnected from the child's properties. @@ -297,6 +313,22 @@ void Node::RecursiveDisconnectFromSceneGraph( BufferIndex updateBufferIndex ) } // namespace SceneGraph +template <> +void OwnerPointer::Reset() +{ + if( mObject != NULL ) + { + Dali::Internal::SceneGraph::Node::Delete( mObject ); + mObject = NULL; + } +} + } // namespace Internal +template <> +void OwnerContainer::Delete(Dali::Internal::SceneGraph::Node* pointer) +{ + Dali::Internal::SceneGraph::Node::Delete( pointer ); +} + } // namespace Dali diff --git a/dali/internal/update/nodes/node.h b/dali/internal/update/nodes/node.h index 01bc76f..66e954c 100644 --- a/dali/internal/update/nodes/node.h +++ b/dali/internal/update/nodes/node.h @@ -107,15 +107,9 @@ public: static Node* New(); /** - * Virtual destructor + * Deletes a Node. */ - virtual ~Node(); - - /** - * Overriden delete operator - * Deletes the node from its global memory pool - */ - void operator delete( void* ptr ); + static void Delete( Node* node ); /** * Called during UpdateManager::DestroyNode shortly before Node is destroyed. @@ -130,7 +124,7 @@ public: */ bool IsLayer() { - return (GetLayer() != NULL); + return mIsLayer; } /** @@ -351,15 +345,6 @@ public: int GetDirtyFlags() const; /** - * Query whether a node is clean. - * @return True if the node is clean. - */ - bool IsClean() const - { - return ( NothingFlag == GetDirtyFlags() ); - } - - /** * Retrieve the parent-origin of the node. * @return The parent-origin. */ @@ -788,6 +773,12 @@ protected: */ Node(); + /** + * Protected virtual destructor; See also Node::Delete( Node* ) + * Kept protected to allow destructor chaining from layer + */ + virtual ~Node(); + private: // from NodeDataProvider /** @@ -889,7 +880,7 @@ protected: ColorMode mColorMode:2; ///< Determines whether mWorldColor is inherited, 2 bits is enough ClippingMode::Type mClippingMode:2; ///< The clipping mode of this node bool mIsRoot:1; ///< True if the node cannot have a parent - + bool mIsLayer:1; ///< True if the node is a layer // Changes scope, should be at end of class DALI_LOG_OBJECT_STRING_DECLARATION; }; @@ -1020,8 +1011,16 @@ inline void SetClippingModeMessage( EventThreadServices& eventThreadServices, co } // namespace SceneGraph +// Template specialisation for OwnerPointer, because delete is protected +template <> +void OwnerPointer::Reset(); + } // namespace Internal +// Template specialisations for OwnerContainer, because delete is protected +template <> +void OwnerContainer::Delete( Dali::Internal::SceneGraph::Node* pointer ); + } // namespace Dali #endif // DALI_INTERNAL_SCENE_GRAPH_NODE_H diff --git a/dali/internal/update/nodes/scene-graph-layer.cpp b/dali/internal/update/nodes/scene-graph-layer.cpp index 573ccee..4519171 100644 --- a/dali/internal/update/nodes/scene-graph-layer.cpp +++ b/dali/internal/update/nodes/scene-graph-layer.cpp @@ -33,6 +33,8 @@ namespace SceneGraph SceneGraph::Layer* Layer::New() { + // Layers are currently heap allocated, unlike Nodes which are in a memory pool + // However Node::Delete( layer ) will correctly delete a layer / node depending on type return new Layer(); } @@ -45,6 +47,9 @@ Layer::Layer() mDepthTestDisabled( true ), mIsDefaultSortFunction( true ) { + // set a flag the node to say this is a layer + mIsLayer = true; + // layer starts off dirty mAllChildTransformsClean[ 0 ] = false; mAllChildTransformsClean[ 1 ] = false; diff --git a/dali/internal/update/nodes/scene-graph-layer.h b/dali/internal/update/nodes/scene-graph-layer.h index e036df1..d3ea452 100644 --- a/dali/internal/update/nodes/scene-graph-layer.h +++ b/dali/internal/update/nodes/scene-graph-layer.h @@ -82,11 +82,6 @@ public: static SceneGraph::Layer* New(); /** - * Virtual destructor - */ - virtual ~Layer(); - - /** * From Node, to convert a node to a layer. * @return The layer. */ @@ -217,6 +212,11 @@ private: // Undefined Layer(const Layer&); + /** + * Virtual destructor + */ + virtual ~Layer(); + // Undefined Layer& operator=(const Layer& rhs);