From 28d2c6ef7e807c53a8bee76c86525f254fc4c54c Mon Sep 17 00:00:00 2001
From: Justin Berger <j.david.berger@gmail.com>
Date: Mon, 18 Sep 2017 17:28:21 -0600
Subject: test: Added additional unit test to variable_watch

This tests adding a variable_watch inside the callback to an existing
callback
---
 .../RunCMake/variable_watch/ModifyWatchInCallback.cmake | 17 +++++++++++++++++
 Tests/RunCMake/variable_watch/RunCMakeTest.cmake        |  1 +
 2 files changed, 18 insertions(+)
 create mode 100644 Tests/RunCMake/variable_watch/ModifyWatchInCallback.cmake

diff --git a/Tests/RunCMake/variable_watch/ModifyWatchInCallback.cmake b/Tests/RunCMake/variable_watch/ModifyWatchInCallback.cmake
new file mode 100644
index 0000000..1dee837
--- /dev/null
+++ b/Tests/RunCMake/variable_watch/ModifyWatchInCallback.cmake
@@ -0,0 +1,17 @@
+function (watch2)
+
+endfunction ()
+
+function (watch1)
+  variable_watch(watched watch2)
+  variable_watch(watched watch2)
+  variable_watch(watched watch2)
+  variable_watch(watched watch2)
+  variable_watch(watched watch2)
+  variable_watch(watched watch2)
+endfunction ()
+
+variable_watch(watched watch1)
+variable_watch(watched watch2)
+
+set(access "${watched}")
diff --git a/Tests/RunCMake/variable_watch/RunCMakeTest.cmake b/Tests/RunCMake/variable_watch/RunCMakeTest.cmake
index 9becb4c..2fa6275 100644
--- a/Tests/RunCMake/variable_watch/RunCMakeTest.cmake
+++ b/Tests/RunCMake/variable_watch/RunCMakeTest.cmake
@@ -3,3 +3,4 @@ include(RunCMake)
 run_cmake(ModifiedAccess)
 run_cmake(NoWatcher)
 run_cmake(WatchTwice)
+run_cmake(ModifyWatchInCallback)
-- 
cgit v0.12


From 4c0edbd725976f947398f1513cef3f36ec05cc5f Mon Sep 17 00:00:00 2001
From: Justin Berger <j.david.berger@gmail.com>
Date: Sat, 22 Jul 2017 17:23:11 -0600
Subject: variable_watch: Made it safe to add/remove watches in access
 callbacks

---
 Source/cmVariableWatch.cxx | 35 ++++++++++++++++-------------------
 Source/cmVariableWatch.h   |  3 ++-
 2 files changed, 18 insertions(+), 20 deletions(-)

diff --git a/Source/cmVariableWatch.cxx b/Source/cmVariableWatch.cxx
index 0e072b8..bd5d19c 100644
--- a/Source/cmVariableWatch.cxx
+++ b/Source/cmVariableWatch.cxx
@@ -2,11 +2,9 @@
    file Copyright.txt or https://cmake.org/licensing for details.  */
 #include "cmVariableWatch.h"
 
-#include "cmAlgorithms.h"
-
-#include "cm_auto_ptr.hxx"
-#include <algorithm>
+#include <memory>
 #include <utility>
+#include <vector>
 
 static const char* const cmVariableWatchAccessStrings[] = {
   "READ_ACCESS",     "UNKNOWN_READ_ACCESS", "UNKNOWN_DEFINED_ACCESS",
@@ -25,35 +23,27 @@ cmVariableWatch::cmVariableWatch()
 {
 }
 
-template <typename C>
-void deleteAllSecond(typename C::value_type it)
-{
-  cmDeleteAll(it.second);
-}
-
 cmVariableWatch::~cmVariableWatch()
 {
-  std::for_each(this->WatchMap.begin(), this->WatchMap.end(),
-                deleteAllSecond<cmVariableWatch::StringToVectorOfPairs>);
 }
 
 bool cmVariableWatch::AddWatch(const std::string& variable, WatchMethod method,
                                void* client_data /*=0*/,
                                DeleteData delete_data /*=0*/)
 {
-  CM_AUTO_PTR<cmVariableWatch::Pair> p(new cmVariableWatch::Pair);
+  auto p = std::make_shared<cmVariableWatch::Pair>();
   p->Method = method;
   p->ClientData = client_data;
   p->DeleteDataCall = delete_data;
   cmVariableWatch::VectorOfPairs& vp = this->WatchMap[variable];
-  for (cmVariableWatch::Pair* pair : vp) {
+  for (auto& pair : vp) {
     if (pair->Method == method && client_data &&
         client_data == pair->ClientData) {
       // Callback already exists
       return false;
     }
   }
-  vp.push_back(p.release());
+  vp.push_back(std::move(p));
   return true;
 }
 
@@ -70,7 +60,6 @@ void cmVariableWatch::RemoveWatch(const std::string& variable,
         // If client_data is NULL, we want to disconnect all watches against
         // the given method; otherwise match ClientData as well.
         (!client_data || (client_data == (*it)->ClientData))) {
-      delete *it;
       vp->erase(it);
       return;
     }
@@ -84,9 +73,17 @@ bool cmVariableWatch::VariableAccessed(const std::string& variable,
   cmVariableWatch::StringToVectorOfPairs::const_iterator mit =
     this->WatchMap.find(variable);
   if (mit != this->WatchMap.end()) {
-    const cmVariableWatch::VectorOfPairs* vp = &mit->second;
-    for (cmVariableWatch::Pair* it : *vp) {
-      it->Method(variable, access_type, it->ClientData, newValue, mf);
+    // The strategy here is to copy the list of callbacks, and ignore
+    // new callbacks that existing ones may add.
+    std::vector<std::weak_ptr<Pair>> vp(mit->second.begin(),
+                                        mit->second.end());
+    for (auto& weak_it : vp) {
+      // In the case where a callback was removed, the weak_ptr will not be
+      // lockable, and so this ensures we don't attempt to call into freed
+      // memory
+      if (auto it = weak_it.lock()) {
+        it->Method(variable, access_type, it->ClientData, newValue, mf);
+      }
     }
     return true;
   }
diff --git a/Source/cmVariableWatch.h b/Source/cmVariableWatch.h
index 05a0a56..27d1b12 100644
--- a/Source/cmVariableWatch.h
+++ b/Source/cmVariableWatch.h
@@ -6,6 +6,7 @@
 #include "cmConfigure.h" // IWYU pragma: keep
 
 #include <map>
+#include <memory> // IWYU pragma: keep
 #include <string>
 #include <vector>
 
@@ -79,7 +80,7 @@ protected:
     }
   };
 
-  typedef std::vector<Pair*> VectorOfPairs;
+  typedef std::vector<std::shared_ptr<Pair>> VectorOfPairs;
   typedef std::map<std::string, VectorOfPairs> StringToVectorOfPairs;
 
   StringToVectorOfPairs WatchMap;
-- 
cgit v0.12