BugFix Safe access to xmldoc in snapshots and undo history

Added mutex to Action manager for access to undo history and added mutex to session's snapshots. Lock and unlock those mutex for all write access and long read access.

src/ActionManager.cpp

@@ -42,7 +42,7 @@
 using namespace tinyxml2;
 
 
-Action::Action(): history_step_(0), history_max_step_(0), locked_(false),
+Action::Action(): history_step_(0), history_max_step_(0),
     snapshot_id_(0), snapshot_node_(nullptr), interpolator_(nullptr), interpolator_node_(nullptr)
 {
 
@@ -64,13 +64,19 @@ void Action::init()
 
 // must be called in a thread running in parrallel of the rendering
 // (needs opengl update to get thumbnail)
-void captureMixerSession(Session *se, tinyxml2::XMLDocument *doc, std::string node, std::string label)
+void captureMixerSession(Session *se, std::string node, std::string label, tinyxml2::XMLDocument *doc = nullptr)
 {
     if (se != nullptr) {
 
+        tinyxml2::XMLDocument *_doc = doc;
+        if (doc == nullptr) {
+            _doc = se->snapshots()->xmlDoc_;
+            se->snapshots()->access_.lock();
+        }
+
         // create node
-        XMLElement *sessionNode = doc->NewElement( node.c_str() );
+        XMLElement *sessionNode = _doc->NewElement( node.c_str() );
-        doc->InsertEndChild(sessionNode);
+        _doc->InsertEndChild(sessionNode);
         // label describes the action
         sessionNode->SetAttribute("label", label.c_str() );
         // label describes the action
@@ -81,7 +87,7 @@ void captureMixerSession(Session *se, tinyxml2::XMLDocument *doc, std::string no
         // get the thumbnail (requires one opengl update to render)
         FrameBufferImage *thumbnail = se->renderThumbnail();
         if (thumbnail) {
-            XMLElement *imageelement = SessionVisitor::ImageToXML(thumbnail, doc);
+            XMLElement *imageelement = SessionVisitor::ImageToXML(thumbnail, _doc);
             if (imageelement)
                 sessionNode->InsertEndChild(imageelement);
             delete thumbnail;
@@ -91,34 +97,52 @@ void captureMixerSession(Session *se, tinyxml2::XMLDocument *doc, std::string no
         sessionNode->SetAttribute("activationThreshold", se->activationThreshold());
 
         // save all sources using source visitor
-        SessionVisitor sv(doc, sessionNode);
+        SessionVisitor sv(_doc, sessionNode);
         for (auto iter = se->begin(); iter != se->end(); ++iter, sv.setRoot(sessionNode) )
             (*iter)->accept(sv);
 
+        if (doc == nullptr)
+            se->snapshots()->access_.unlock();
     }
 }
 
+void Action::storeSession(Session *se, std::string label)
+{
+    //
+    if (!Action::manager().history_access_.try_lock())
+        return;
+
+    // incremental naming of history nodes
+    Action::manager().history_step_++;
+
+    // erase future
+    for (uint e = Action::manager().history_step_; e <= Action::manager().history_max_step_; e++) {
+        XMLElement *node = Action::manager().history_doc_.FirstChildElement( HISTORY_NODE(e).c_str() );
+        if ( node )
+            Action::manager().history_doc_.DeleteChild(node);
+    }
+    Action::manager().history_max_step_ = Action::manager().history_step_;
+
+    // capture current session
+    captureMixerSession(se,
+                        HISTORY_NODE(Action::manager().history_step_),
+                        label,
+                        &Action::manager().history_doc_);
+
+    Action::manager().history_access_.unlock();
+}
+
+
 void Action::store(const std::string &label)
 {
     // TODO: set a maximum amount of stored steps? (even very big, just to ensure memory limit)
 
     // ignore if locked or if no label is given
-    if (locked_ || label.empty())
+    if (label.empty())
         return;
 
-    // incremental naming of history nodes
-    history_step_++;
-
-    // erase future
-    for (uint e = history_step_; e <= history_max_step_; e++) {
-        XMLElement *node = history_doc_.FirstChildElement( HISTORY_NODE(e).c_str() );
-        if ( node )
-            history_doc_.DeleteChild(node);
-    }
-    history_max_step_ = history_step_;
-
     // threaded capturing state of current session
-    std::thread(captureMixerSession, Mixer::manager().session(), &history_doc_, HISTORY_NODE(history_step_), label).detach();
+    std::thread(Action::storeSession, Mixer::manager().session(), label).detach();
 
 #ifdef ACTION_DEBUG
     Log::Info("Action stored %d '%s'", history_step_, label.c_str());
@@ -184,8 +208,7 @@ FrameBufferImage *Action::thumbnail(uint s) const
 
 void Action::restore(uint target)
 {
-    // lock
+    history_access_.lock();
-    locked_ = true;
 
     // get history node of target step
     history_step_ = CLAMP(target, 1, history_max_step_);
@@ -203,8 +226,7 @@ void Action::restore(uint target)
         Mixer::manager().restore(sessionNode);
     }
 
-    // free
+    history_access_.unlock();
-    locked_ = false;
 }
 
 
@@ -220,9 +242,9 @@ void Action::takeSnapshot(Session *se, const std::string &label, bool create_thr
 
         if (create_thread)
             // threaded capture state of current session
-            std::thread(captureMixerSession, se, se->snapshots()->xmlDoc_, SNAPSHOT_NODE(id), label).detach();
+            std::thread(captureMixerSession, se, SNAPSHOT_NODE(id), label, nullptr).detach();
         else
-            captureMixerSession(se, se->snapshots()->xmlDoc_, SNAPSHOT_NODE(id), label);
+            captureMixerSession(se, SNAPSHOT_NODE(id), label);
 
 #ifdef ACTION_DEBUG
         Log::Info("Snapshot stored %d '%s'", id, label.c_str());
@@ -232,10 +254,6 @@ void Action::takeSnapshot(Session *se, const std::string &label, bool create_thr
 
 void Action::snapshot(const std::string &label)
 {
-    // ignore if locked
-    if (locked_)
-        return;
-
     // ensure label is unique
     std::string snap_label = BaseToolkit::uniqueName(label, labels());
 
@@ -250,7 +268,12 @@ void Action::open(uint64_t snapshotid)
     {
         // get snapshot node of target in current session
         Session *se = Mixer::manager().session();
-        snapshot_node_ = se->snapshots()->xmlDoc_->FirstChildElement( SNAPSHOT_NODE(snapshotid).c_str() );
+        if (se) {
+            se->snapshots()->access_.lock();
+            snapshot_node_ = se->snapshots()->xmlDoc_->FirstChildElement(
+                SNAPSHOT_NODE(snapshotid).c_str());
+            se->snapshots()->access_.unlock();
+        }
 
         if (snapshot_node_)
             snapshot_id_ = snapshotid;
@@ -263,10 +286,6 @@ void Action::open(uint64_t snapshotid)
 
 void Action::replace(uint64_t snapshotid)
 {
-    // ignore if locked or if no label is given
-    if (locked_)
-        return;
-
     if (snapshotid > 0)
         open(snapshotid);
 
@@ -277,14 +296,18 @@ void Action::replace(uint64_t snapshotid)
         // remove previous node
         Session *se = Mixer::manager().session();
         if (se) {
+
+            se->snapshots()->access_.lock();
             se->snapshots()->xmlDoc_->DeleteChild( snapshot_node_ );
+            se->snapshots()->access_.unlock();
 
             // threaded capture state of current session
-            std::thread(captureMixerSession, se, se->snapshots()->xmlDoc_, SNAPSHOT_NODE(snapshot_id_), label).detach();
+            std::thread(captureMixerSession, se, SNAPSHOT_NODE(snapshot_id_), label, nullptr).detach();
 
 #ifdef ACTION_DEBUG
             Log::Info("Snapshot replaced %d '%s'", snapshot_id_, label.c_str());
 #endif
+
         }
     }
 }
@@ -378,8 +401,10 @@ void Action::remove(uint64_t snapshotid)
     if (snapshot_node_) {
         // remove
         Session *se = Mixer::manager().session();
+        se->snapshots()->access_.lock();
         se->snapshots()->xmlDoc_->DeleteChild( snapshot_node_ );
         se->snapshots()->keys_.remove( snapshot_id_ );
+        se->snapshots()->access_.unlock();
     }
 
     snapshot_node_ = nullptr;
@@ -388,9 +413,6 @@ void Action::remove(uint64_t snapshotid)
 
 void Action::restore(uint64_t snapshotid)
 {
-    // lock
-    locked_ = true;
-
     if (snapshotid > 0)
         open(snapshotid);
 
@@ -398,9 +420,6 @@ void Action::restore(uint64_t snapshotid)
         // actually restore
         Mixer::manager().restore(snapshot_node_);
 
-    // free
-    locked_ = false;
-
     store("Snapshot " + label(snapshot_id_));
 }
 
@@ -516,8 +535,7 @@ static void saveSnapshot(const std::string& filename, tinyxml2::XMLElement *snap
 
 void Action::saveas(const std::string& filename, uint64_t snapshotid)
 {
-    // ignore if locked or if no label is given
+    if (filename.empty())
-    if (locked_)
         return;
 
     if (snapshotid > 0)

src/ActionManager.h

@@ -3,7 +3,7 @@
 
 #include <list>
 #include <string>
-#include <atomic>
+#include <mutex>
 
 #include <tinyxml2.h>
 
@@ -62,11 +62,11 @@ public:
     void interpolate (float val, uint64_t snapshotid = 0);
 
 private:
-
     tinyxml2::XMLDocument history_doc_;
     uint history_step_;
     uint history_max_step_;
-    std::atomic<bool> locked_;
+    std::mutex history_access_;
+    static void storeSession(Session *se, std::string label);
     void restore(uint target);
 
     uint64_t snapshot_id_;

src/Session.cpp

@@ -970,6 +970,7 @@ Metronome::Synchronicity Session::inputSynchrony(uint input)
 
 void Session::applySnapshot(uint64_t key)
 {
+    snapshots_.access_.lock();
     tinyxml2::XMLElement *snapshot_node_ = snapshots_.xmlDoc_->FirstChildElement( SNAPSHOT_NODE(key).c_str() );;
     if (snapshot_node_) {
 
@@ -1008,6 +1009,7 @@ void Session::applySnapshot(uint64_t key)
 
         ++View::need_deep_update_;
     }
+    snapshots_.access_.unlock();
 }
 
 

src/Session.h

@@ -2,7 +2,6 @@
 #define SESSION_H
 
 #include <mutex>
-#include <variant>
 
 #include "SourceList.h"
 #include "RenderView.h"
@@ -34,6 +33,7 @@ struct SessionSnapshots {
 
     tinyxml2::XMLDocument *xmlDoc_;
     std::list<uint64_t> keys_;
+    std::mutex access_;
 };
 
 class Session

src/SessionCreator.cpp

@@ -216,8 +216,10 @@ void SessionCreator::loadSnapshots(XMLElement *snapshotsNode)
             std::istringstream nodename( N->Name() );
             nodename >> c >> id;
 
+            session_->snapshots()->access_.lock();
             session_->snapshots()->keys_.push_back(id);
             session_->snapshots()->xmlDoc_->InsertEndChild( N->DeepClone(session_->snapshots()->xmlDoc_) );
+            session_->snapshots()->access_.unlock();
         }
     }
 }

src/SessionVisitor.cpp

@@ -157,10 +157,17 @@ void SessionVisitor::saveSnapshots(tinyxml2::XMLDocument *doc, Session *session)
 {
     if (doc != nullptr && session != nullptr)
     {
+        // create element
         XMLElement *snapshots = doc->NewElement("Snapshots");
+
+        // access to session snapshot
+        session->snapshots()->access_.lock();
         const XMLElement* N = session->snapshots()->xmlDoc_->FirstChildElement();
         for( ; N ; N=N->NextSiblingElement())
             snapshots->InsertEndChild( N->DeepClone( doc ));
+        session->snapshots()->access_.unlock();
+
+        // insert element
         doc->InsertEndChild(snapshots);
     }
 }