Fix thread safety issues in TimelineDecoder and associated unit tests

Enforce serialized access to TimelineDecoder::m_Model by removing public
access funtion and replacing with an 'Apply' method taking a lambda and
uses a std::lock.

Use the new lambda when invoking callbacks.

Change-Id: I6ea2fbca990736f3be63e80897f175421f19f0c1
Signed-off-by: Matthew Bentham <matthew.bentham@arm.com>

3 files changed, 64 insertions, 52 deletions

diff --git a/profiling/server/include/timelineDecoder/TimelineDecoder.hpp b/profiling/server/include/timelineDecoder/TimelineDecoder.hpp
index ea4b144860..9776ec91f9 100644
--- a/profiling/server/include/timelineDecoder/TimelineDecoder.hpp
+++ b/profiling/server/include/timelineDecoder/TimelineDecoder.hpp
@@ -6,6 +6,8 @@
 
 #include "ITimelineDecoder.hpp"
 
+#include <mutex>
+#include <utility>
 #include <vector>
 
 namespace arm
@@ -40,7 +42,11 @@ public:
     virtual TimelineStatus CreateLabel(const Label &) override;
     virtual TimelineStatus CreateRelationship(const Relationship &) override;
 
-    const Model& GetModel();
+    template<class F>
+    decltype(auto) ApplyToModel(F&& f){
+        std::lock_guard<std::mutex> lock(m_ModelMutex);
+        return f(m_Model);
+    }
 
     TimelineStatus SetEntityCallback(const OnNewEntityCallback);
     TimelineStatus SetEventClassCallback(const OnNewEventClassCallback);
@@ -54,6 +60,7 @@ public:
 
 private:
     Model m_Model;
+    std::mutex m_ModelMutex;
 
     OnNewEntityCallback m_OnNewEntityCallback;
     OnNewEventClassCallback m_OnNewEventClassCallback;
@@ -69,4 +76,4 @@ private:
 };
 
 } // namespace pipe
-} // namespace arm
\ No newline at end of file
+} // namespace arm
diff --git a/profiling/server/src/timelineDecoder/TimelineDecoder.cpp b/profiling/server/src/timelineDecoder/TimelineDecoder.cpp
index df967de1f5..6eaaff6e54 100644
--- a/profiling/server/src/timelineDecoder/TimelineDecoder.cpp
+++ b/profiling/server/src/timelineDecoder/TimelineDecoder.cpp
@@ -20,8 +20,9 @@ TimelineDecoder::TimelineStatus TimelineDecoder::CreateEntity(const Entity &enti
     {
         return TimelineStatus::TimelineStatus_Fail;
     }
-    m_OnNewEntityCallback(m_Model, entity);
-
+    ApplyToModel([&](Model& m){
+        m_OnNewEntityCallback(m, entity);
+    });
     return TimelineStatus::TimelineStatus_Success;
 }
 
@@ -31,7 +32,9 @@ TimelineDecoder::TimelineStatus TimelineDecoder::CreateEventClass(const EventCla
     {
         return TimelineStatus::TimelineStatus_Fail;
     }
-    m_OnNewEventClassCallback(m_Model, eventClass);
+    ApplyToModel([&](Model& m){
+        m_OnNewEventClassCallback(m, eventClass);
+    });
 
     return TimelineStatus::TimelineStatus_Success;
 }
@@ -42,7 +45,9 @@ TimelineDecoder::TimelineStatus TimelineDecoder::CreateEvent(const Event &event)
     {
         return TimelineStatus::TimelineStatus_Fail;
     }
-    m_OnNewEventCallback(m_Model, event);
+    ApplyToModel([&](Model& m){
+        m_OnNewEventCallback(m, event);
+    });
 
     return TimelineStatus::TimelineStatus_Success;
 }
@@ -53,7 +58,9 @@ TimelineDecoder::TimelineStatus TimelineDecoder::CreateLabel(const Label &label)
     {
         return TimelineStatus::TimelineStatus_Fail;
     }
-    m_OnNewLabelCallback(m_Model, label);
+    ApplyToModel([&](Model& m){
+        m_OnNewLabelCallback(m, label);
+    });
 
     return TimelineStatus::TimelineStatus_Success;
 }
@@ -64,15 +71,12 @@ TimelineDecoder::TimelineStatus TimelineDecoder::CreateRelationship(const Relati
     {
         return TimelineStatus::TimelineStatus_Fail;
     }
-    m_OnNewRelationshipCallback(m_Model, relationship);
+    ApplyToModel([&](Model& m){
+        m_OnNewRelationshipCallback(m, relationship);
+    });
     return TimelineStatus::TimelineStatus_Success;
 }
 
-const TimelineDecoder::Model &TimelineDecoder::GetModel()
-{
-    return m_Model;
-}
-
 TimelineDecoder::TimelineStatus TimelineDecoder::SetEntityCallback(OnNewEntityCallback cb)
 {
     if (cb == nullptr)
@@ -323,4 +327,4 @@ void TimelineDecoder::printRelationships()
 }
 
 } // namespace pipe
-} // namespace arm
\ No newline at end of file
+} // namespace arm
diff --git a/profiling/server/src/timelineDecoder/tests/TimelineTests.cpp b/profiling/server/src/timelineDecoder/tests/TimelineTests.cpp
index f09848f52c..82c16fee79 100644
--- a/profiling/server/src/timelineDecoder/tests/TimelineTests.cpp
+++ b/profiling/server/src/timelineDecoder/tests/TimelineTests.cpp
@@ -154,8 +154,6 @@ TEST_CASE("TimelineCaptureTest")
     arm::pipe::PacketVersionResolver packetVersionResolver;
 
     arm::pipe::TimelineDecoder timelineDecoder;
-    const arm::pipe::TimelineDecoder::Model& model = timelineDecoder.GetModel();
-
 
     arm::pipe::TimelineCaptureCommandHandler timelineCaptureCommandHandler(
         1, 1, packetVersionResolver.ResolvePacketVersion(1, 1).GetEncodedValue(), timelineDecoder,
@@ -238,25 +236,27 @@ TEST_CASE("TimelineCaptureTest")
                                            timelineCaptureCommandHandler);
     }
 
-    for (unsigned long i = 0; i < 10; ++i)
-    {
-        CHECK(model.m_Entities[i].m_Guid == entityGuid);
+    timelineDecoder.ApplyToModel([&](const arm::pipe::TimelineDecoder::Model& model){
+        for (unsigned long i = 0; i < 10; ++i)
+        {
+            CHECK(model.m_Entities[i].m_Guid == entityGuid);
 
-        CHECK(model.m_EventClasses[i].m_Guid == eventClassGuid);
+            CHECK(model.m_EventClasses[i].m_Guid == eventClassGuid);
 
-        CHECK(model.m_Events[i].m_TimeStamp == timestamp);
-        CHECK(model.m_Events[i].m_ThreadId == uint64ThreadId);
-        CHECK(model.m_Events[i].m_Guid == eventGuid);
+            CHECK(model.m_Events[i].m_TimeStamp == timestamp);
+            CHECK(model.m_Events[i].m_ThreadId == uint64ThreadId);
+            CHECK(model.m_Events[i].m_Guid == eventGuid);
 
-        CHECK(model.m_Labels[i].m_Guid == labelGuid);
-        CHECK(model.m_Labels[i].m_Name == labelName);
+            CHECK(model.m_Labels[i].m_Guid == labelGuid);
+            CHECK(model.m_Labels[i].m_Name == labelName);
 
-        CHECK(model.m_Relationships[i].m_RelationshipType ==
-            arm::pipe::ITimelineDecoder::RelationshipType::DataLink);
-        CHECK(model.m_Relationships[i].m_Guid == relationshipGuid);
-        CHECK(model.m_Relationships[i].m_HeadGuid == headGuid);
-        CHECK(model.m_Relationships[i].m_TailGuid == tailGuid);
-    }
+            CHECK(model.m_Relationships[i].m_RelationshipType ==
+                arm::pipe::ITimelineDecoder::RelationshipType::DataLink);
+            CHECK(model.m_Relationships[i].m_Guid == relationshipGuid);
+            CHECK(model.m_Relationships[i].m_HeadGuid == headGuid);
+            CHECK(model.m_Relationships[i].m_TailGuid == tailGuid);
+        }
+    });
 }
 
 TEST_CASE("TimelineCaptureTestMultipleStringsInBuffer")
@@ -270,7 +270,6 @@ TEST_CASE("TimelineCaptureTestMultipleStringsInBuffer")
     arm::pipe::PacketVersionResolver packetVersionResolver;
 
     arm::pipe::TimelineDecoder timelineDecoder;
-    const arm::pipe::TimelineDecoder::Model& model = timelineDecoder.GetModel();
 
     arm::pipe::TimelineCaptureCommandHandler timelineCaptureCommandHandler(
         1, 1, packetVersionResolver.ResolvePacketVersion(1, 1).GetEncodedValue(), timelineDecoder,
@@ -343,30 +342,32 @@ TEST_CASE("TimelineCaptureTestMultipleStringsInBuffer")
     SendTimelinePacketToCommandHandler(bufferManager.GetReadableBuffer()->GetReadableData(),
                                        timelineCaptureCommandHandler);
 
-    for ( unsigned long i = 0; i < 9; ++i )
-    {
-        CHECK(model.m_Entities[i].m_Guid == entityGuid);
+    timelineDecoder.ApplyToModel([&](const arm::pipe::TimelineDecoder::Model& model){
+        for ( unsigned long i = 0; i < 9; ++i )
+        {
+            CHECK(model.m_Entities[i].m_Guid == entityGuid);
 
-        CHECK(model.m_EventClasses[i].m_Guid == eventClassGuid);
+            CHECK(model.m_EventClasses[i].m_Guid == eventClassGuid);
 
-        CHECK(model.m_Labels[i].m_Guid == labelGuid);
+            CHECK(model.m_Labels[i].m_Guid == labelGuid);
 
-        CHECK(model.m_Events[i].m_TimeStamp == timestamp);
-        CHECK(model.m_Events[i].m_ThreadId == uint64ThreadId);
-        CHECK(model.m_Events[i].m_Guid == eventGuid);
+            CHECK(model.m_Events[i].m_TimeStamp == timestamp);
+            CHECK(model.m_Events[i].m_ThreadId == uint64ThreadId);
+            CHECK(model.m_Events[i].m_Guid == eventGuid);
 
-        CHECK(model.m_Relationships[i].m_RelationshipType ==
-            arm::pipe::ITimelineDecoder::RelationshipType::DataLink);
-        CHECK(model.m_Relationships[i].m_Guid == relationshipGuid);
-        CHECK(model.m_Relationships[i].m_HeadGuid == headGuid);
-        CHECK(model.m_Relationships[i].m_TailGuid == tailGuid);
-    }
-    for ( unsigned long i = 0; i < 9; i += 3 )
-    {
-        CHECK(model.m_Labels[i].m_Name == labelName);
-        CHECK(model.m_Labels[i+1].m_Name == labelName2);
-        CHECK(model.m_Labels[i+2].m_Name == labelName3);
-    }
+            CHECK(model.m_Relationships[i].m_RelationshipType ==
+                arm::pipe::ITimelineDecoder::RelationshipType::DataLink);
+            CHECK(model.m_Relationships[i].m_Guid == relationshipGuid);
+            CHECK(model.m_Relationships[i].m_HeadGuid == headGuid);
+            CHECK(model.m_Relationships[i].m_TailGuid == tailGuid);
+        }
+        for ( unsigned long i = 0; i < 9; i += 3 )
+        {
+            CHECK(model.m_Labels[i].m_Name == labelName);
+            CHECK(model.m_Labels[i+1].m_Name == labelName2);
+            CHECK(model.m_Labels[i+2].m_Name == labelName3);
+        }
+    });
 }
 
 }