Fix memory leak, scene graph layers are never deleted from memory 15/115115/11
authorNick Holland <nick.holland@partner.samsung.com>
Thu, 16 Feb 2017 09:21:52 +0000 (09:21 +0000)
committerNick Holland <nick.holland@partner.samsung.com>
Mon, 27 Feb 2017 10:44:46 +0000 (10:44 +0000)
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

dali/devel-api/common/owner-container.h
dali/internal/update/common/discard-queue.h
dali/internal/update/manager/update-manager.cpp
dali/internal/update/nodes/node-declarations.h
dali/internal/update/nodes/node.cpp
dali/internal/update/nodes/node.h
dali/internal/update/nodes/scene-graph-layer.cpp
dali/internal/update/nodes/scene-graph-layer.h

index 108d1a4..3ac0df2 100644 (file)
@@ -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
index d521493..e71fe61 100644 (file)
@@ -120,7 +120,7 @@ private:
   RenderQueue& mRenderQueue; ///< Used to send GL clean-up messages for the next Render.
 
   // Messages are queued here following the current update buffer number
-  NodeOwnerContainer           mNodeQueue[2];
+  OwnerContainer< Node* >      mNodeQueue[2];
   ShaderQueue                  mShaderQueue[2];
   RendererQueue                mRendererQueue[2];
   CameraQueue                  mCameraQueue[2];
index 8725481..9e7a6fd 100644 (file)
@@ -184,7 +184,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
@@ -192,7 +192,7 @@ struct UpdateManager::Impl
     {
       root->OnDestroy();
 
-      delete root;
+      Node::Delete( root );
       root = NULL;
     }
 
@@ -200,7 +200,7 @@ struct UpdateManager::Impl
     {
       systemLevelRoot->OnDestroy();
 
-      delete systemLevelRoot;
+      Node::Delete( systemLevelRoot );
       systemLevelRoot = NULL;
     }
 
index 853cb59..131ba19 100644 (file)
@@ -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;
index 633ded9..f462feb 100644 (file)
@@ -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<Dali::Internal::SceneGraph::Node> 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( NULL ),
   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<Node*>( ptr ) );
-}
-
 void Node::OnDestroy()
 {
   // Animators, Constraints etc. should be disconnected from the child's properties.
@@ -320,6 +336,22 @@ void Node::RecursiveDisconnectFromSceneGraph( BufferIndex updateBufferIndex )
 
 } // namespace SceneGraph
 
+template <>
+void OwnerPointer<Dali::Internal::SceneGraph::Node>::Reset()
+{
+  if( mObject != NULL )
+  {
+    Dali::Internal::SceneGraph::Node::Delete( mObject );
+    mObject = NULL;
+  }
+}
+
 } // namespace Internal
 
+template <>
+void OwnerContainer<Dali::Internal::SceneGraph::Node*>::Delete(Dali::Internal::SceneGraph::Node* pointer)
+{
+  Dali::Internal::SceneGraph::Node::Delete( pointer );
+}
+
 } // namespace Dali
index a170add..e2ba292 100644 (file)
@@ -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;
   }
 
   /**
@@ -335,15 +329,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.
    */
@@ -772,6 +757,12 @@ protected:
    */
   Node();
 
+  /**
+   * Protected virtual destructor; See also Node::Delete( Node* )
+   * Kept protected to allow destructor chaining from layer
+   */
+  virtual ~Node();
+
 private: // from NodeDataProvider
 
   /**
@@ -873,7 +864,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;
 };
@@ -1004,8 +995,16 @@ inline void SetClippingModeMessage( EventThreadServices& eventThreadServices, co
 
 } // namespace SceneGraph
 
+// Template specialisation for OwnerPointer<Node>, because delete is protected
+template <>
+void OwnerPointer<Dali::Internal::SceneGraph::Node>::Reset();
+
 } // namespace Internal
 
+// Template specialisations for OwnerContainer<Node*>, because delete is protected
+template <>
+void OwnerContainer<Dali::Internal::SceneGraph::Node*>::Delete( Dali::Internal::SceneGraph::Node* pointer );
+
 } // namespace Dali
 
 #endif // DALI_INTERNAL_SCENE_GRAPH_NODE_H
index 573ccee..4519171 100644 (file)
@@ -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;
index e036df1..d3ea452 100644 (file)
@@ -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);