Fixed a potential memory corruption due to update and render potentially using the...

Fixed by not using double buffering with index but having two separate containers

==5511== ----------------------------------------------------------------
==5511== Possible data race during read of size 4 at 0x14ECD29C by thread #4
==5511== Locks held: none
==5511==    at 0x55ADFB1: Dali::Internal::SceneGraph::UpdateManager::SaveBinary(Dali::IntrusivePtr<Dali::Internal::ShaderData>) (update-manager.cpp:632)
==5511==    by 0x5599DF4: Dali::Internal::ProgramController::StoreBinary(Dali::IntrusivePtr<Dali::Internal::ShaderData>) (program-controller.cpp:151)
==5511==    by 0x5598CFD: Dali::Internal::Program::Load() (program.cpp:559)
==5511==    by 0x5598DB4: Dali::Internal::Program::Use() (program.cpp:135)
==5511==    by 0x559799C: Dali::Internal::SceneGraph::Renderer::Render(Dali::Internal::Context&, Dali::Internal::SceneGraph::TextureCache&, unsigned int, Dali::Internal::SceneGraph::Shader&, Dali::Matrix const&, Dali::Matrix const&, Dali::Matrix const&, float, bool) (scene-graph-renderer.cpp:213)
==5511==    by 0x55891B2: Dali::Internal::Render::ProcessRenderInstruction(Dali::Internal::SceneGraph::RenderInstruction const&, Dali::Internal::Context&, Dali::Internal::SceneGraph::TextureCache&, Dali::Internal::SceneGraph::Shader&, unsigned int, float) (render-algorithms.cpp:146)
==5511==    by 0x558BDE5: Dali::Internal::SceneGraph::RenderManager::DoRender(Dali::Internal::SceneGraph::RenderInstruction&, Dali::Internal::SceneGraph::Shader&, float) (render-manager.cpp:558)
==5511==    by 0x558C423: Dali::Internal::SceneGraph::RenderManager::Render(Dali::Integration::RenderStatus&) (render-manager.cpp:430)
==5511==    by 0x551D25C: Dali::Internal::Core::Render(Dali::Integration::RenderStatus&) (core-impl.cpp:278)
==5511==    by 0x4E72B04: Dali::Internal::Adaptor::RenderThread::Run() (render-thread.cpp:199)
==5511==    by 0x4E72B58: Dali::Internal::Adaptor::RenderThread::InternalThreadEntryFunc(void*) (render-thread.h:197)
==5511==    by 0x4C30E26: ??? (in /usr/lib/valgrind/vgpreload_helgrind-amd64-linux.so)
==5511==
==5511== This conflicts with a previous write of size 4 by thread #3
==5511== Locks held: none
==5511==    at 0x559F1EF: Dali::Internal::SceneGraph::SceneGraphBuffers::Swap() (scene-graph-buffers.cpp:45)
==5511==    by 0x55ADADA: Dali::Internal::SceneGraph::UpdateManager::Update(float, unsigned int, unsigned int) (update-manager.cpp:1121)
==5511==    by 0x551D229: Dali::Internal::Core::Update(float, unsigned int, unsigned int, Dali::Integration::UpdateStatus&) (core-impl.cpp:261)
==5511==    by 0x4E74438: Dali::Internal::Adaptor::UpdateThread::Run() (update-thread.cpp:119)
==5511==    by 0x4E744A8: Dali::Internal::Adaptor::UpdateThread::InternalThreadEntryFunc(void*) (update-thread.h:107)
==5511==    by 0x4C30E26: ??? (in /usr/lib/valgrind/vgpreload_helgrind-amd64-linux.so)
==5511==    by 0x908A181: start_thread (pthread_create.c:312)
==5511==    by 0x5E8EFBC: clone (clone.S:111)
==5511==
==5511== Address 0x14ECD29C is 12 bytes inside a block of size 24 alloc'd
==5511==    at 0x4C2C460: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_helgrind-amd64-linux.so)
==5511==    by 0x551D745: Dali::Internal::Core::Core(Dali::Integration::RenderController&, Dali::Integration::PlatformAbstraction&, Dali::Integration::GlAbstraction&, Dali::Integration::GlSyncAbstraction&, Dali::Integration::GestureManager&, Dali::ResourcePolicy::DataRetention) (core-impl.cpp:149)
==5511==    by 0x55BB824: Dali::Integration::Core::New(Dali::Integration::RenderController&, Dali::Integration::PlatformAbstraction&, Dali::Integration::GlAbstraction&, Dali::Integration::GlSyncAbstraction&, Dali::Integration::GestureManager&, Dali::ResourcePolicy::DataRetention) (core.cpp:37)
==5511==    by 0x4E9E9E8: Dali::Internal::Adaptor::Adaptor::Initialize(Dali::Configuration::ContextLoss) (adaptor-impl.cpp:139)
==5511==    by 0x4E9FC3C: Dali::Internal::Adaptor::Adaptor::New(Dali::Any, Dali::RenderSurface*, Dali::Configuration::ContextLoss, Dali::Internal::Adaptor::EnvironmentOptions*) (adaptor-impl.cpp:79)
==5511==    by 0x4E9FD05: Dali::Internal::Adaptor::Adaptor::New(Dali::Window, Dali::Configuration::ContextLoss, Dali::Internal::Adaptor::EnvironmentOptions*) (adaptor-impl.cpp:89)
==5511==    by 0x4EA22BB: Dali::Internal::Adaptor::Application::CreateAdaptor() (application-impl.cpp:134)
==5511==    by 0x4EA2407: Dali::Internal::Adaptor::Application::OnInit() (application-impl.cpp:175)
==5511==    by 0x4EB8B88: Dali::Internal::Adaptor::Framework::AppStatusHandler(int, void*) (framework-tizen.cpp:380)
==5511==    by 0x7D6BC9D: app_appcore_create (app_main.c:149)
==5511==    by 0x7F6E819: appcore_efl_main (appcore-efl.c:16)
==5511==    by 0x7D6BDEC: app_efl_main (app_main.c:116)
==5511== ----------------------------------------------------------------

Change-Id: I1bdcfd8c00d9f37b0dd1f3f580f1bd69d4e0a20b

diff --git a/dali/internal/update/manager/update-manager.cpp b/dali/internal/update/manager/update-manager.cpp
index 0288a02..516fa93 100644 (file)
--- a/dali/internal/update/manager/update-manager.cpp
+++ b/dali/internal/update/manager/update-manager.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2014 Samsung Electronics Co., Ltd.
+ * Copyright (c) 2015 Samsung Electronics Co., Ltd.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -21,6 +21,7 @@
 // INTERNAL INCLUDES
 #include <dali/public-api/common/stage.h>
 #include <dali/devel-api/common/set-wrapper.h>
+#include <dali/devel-api/common/mutex.h>
 
 #include <dali/integration-api/core.h>
 #include <dali/integration-api/render-controller.h>
@@ -131,8 +132,7 @@ typedef OwnerContainer< Shader* >              ShaderContainer;
 typedef ShaderContainer::Iterator              ShaderIter;
 typedef ShaderContainer::ConstIterator         ShaderConstIter;
 
-typedef std::vector<Internal::ShaderDataPtr> ShaderDataBatchedQueue;
-typedef ShaderDataBatchedQueue::iterator        ShaderDataBatchedQueueIterator;
+typedef std::vector<Internal::ShaderDataPtr>   ShaderDataBinaryQueue;
 
 typedef OwnerContainer<PanGesture*>            GestureContainer;
 typedef GestureContainer::Iterator             GestureIter;
@@ -276,12 +276,14 @@ struct UpdateManager::Impl
   ObjectOwnerContainer<Geometry>      geometries;                    ///< A container of geometries
   ObjectOwnerContainer<Material>      materials;                     ///< A container of materials
   ObjectOwnerContainer<Sampler>       samplers;                      ///< A container of samplers
-  ObjectOwnerContainer<PropertyBuffer> propertyBuffers;             ///< A container of property buffers
+  ObjectOwnerContainer<PropertyBuffer> propertyBuffers;              ///< A container of property buffers
 
   ShaderContainer                     shaders;                       ///< A container of owned shaders
 
   MessageQueue                        messageQueue;                  ///< The messages queued from the event-thread
-  ShaderDataBatchedQueue              compiledShaders[2];            ///< Shaders compiled on Render thread are inserted here for update thread to pass on to event thread.
+  ShaderDataBinaryQueue               renderCompiledShaders;         ///< Shaders compiled on Render thread are inserted here for update thread to pass on to event thread.
+  ShaderDataBinaryQueue               updateCompiledShaders;         ///< Shaders to be sent from Update to Event
+  Mutex                               compiledShaderMutex;           ///< lock to ensure no corruption on the renderCompiledShaders
 
   float                               keepRenderingSeconds;          ///< Set via Dali::Stage::KeepRendering
   bool                                animationFinishedDuringUpdate; ///< Flag whether any animations finished during the Update()
@@ -629,7 +631,11 @@ void UpdateManager::SaveBinary( Internal::ShaderDataPtr shaderData )
 {
   DALI_ASSERT_DEBUG( shaderData && "No NULL shader data pointers please." );
   DALI_ASSERT_DEBUG( shaderData->GetBufferSize() > 0 && "Shader binary empty so nothing to save." );
-  mImpl->compiledShaders[mSceneGraphBuffers.GetUpdateBufferIndex() > 0 ? 0 : 1].push_back( shaderData );
+  {
+    // lock as update might be sending previously compiled shaders to event thread
+    Mutex::ScopedLock lock( mImpl->compiledShaderMutex );
+    mImpl->renderCompiledShaders.push_back( shaderData );
+  }
 }
 
 RenderTaskList* UpdateManager::GetRenderTaskList( bool systemLevel )
@@ -919,16 +925,24 @@ void UpdateManager::ForwardCompiledShadersToEventThread()
   DALI_ASSERT_DEBUG( (mImpl->shaderSaver != 0) && "shaderSaver should be wired-up during startup." );
   if( mImpl->shaderSaver )
   {
-    ShaderDataBatchedQueue& compiledShaders = mImpl->compiledShaders[mSceneGraphBuffers.GetUpdateBufferIndex()];
-    if( compiledShaders.size() > 0 )
+    // lock and swap the queues
+    {
+      // render might be attempting to send us more binaries at the same time
+      Mutex::ScopedLock lock( mImpl->compiledShaderMutex );
+      mImpl->renderCompiledShaders.swap( mImpl->updateCompiledShaders );
+    }
+
+    if( mImpl->updateCompiledShaders.size() > 0 )
     {
       ShaderSaver& factory = *mImpl->shaderSaver;
-      ShaderDataBatchedQueueIterator i   = compiledShaders.begin();
-      ShaderDataBatchedQueueIterator end = compiledShaders.end();
+      ShaderDataBinaryQueue::iterator i   = mImpl->updateCompiledShaders.begin();
+      ShaderDataBinaryQueue::iterator end = mImpl->updateCompiledShaders.end();
       for( ; i != end; ++i )
       {
         mImpl->notificationManager.QueueMessage( ShaderCompiledMessage( factory, *i ) );
       }
+      // we don't need them in update anymore
+      mImpl->updateCompiledShaders.clear();
     }
   }
 }
@@ -1009,7 +1023,7 @@ unsigned int UpdateManager::Update( float elapsedSeconds,
   // 6) Post Process Ids of resources updated by renderer
   mImpl->resourceManager.PostProcessResources( bufferIndex );
 
-  // 6.1) Forward a batch of compiled shader programs to event thread for saving
+  // 6.1) Forward compiled shader programs to event thread for saving
   ForwardCompiledShadersToEventThread();
 
   // Although the scene-graph may not require an update, we still need to synchronize double-buffered