Fix base::Observers impl: Copy the whole list of observers before we start iterating them (fix issue 427)

There are cases where we need to modify the list of observers of certain
entity when we are in a notification loop (i.e. iterating its observers).
E.g. A general update notification about the current document to all its
observers could create the mini editor, which is a DocumentObserver, so
a new observer is added to the list in the same notification loop.

Anyway, as we cannot modify the observer list (std::vector) when we are
notifying them (any modification in the std::vector invalidates
its iterators), the fix is quite easy (but not optimal): we can create
a copy of the observers list so we can iterate the list.

Note: If we have performance issues about this, we could try a std::list,
but at the moment this fix is quite enough.

src/base/observers.h

@@ -1,5 +1,5 @@
 // Aseprite Base Library
-// Copyright (c) 2001-2013 David Capello
+// Copyright (c) 2001-2014 David Capello
 //
 // This file is released under the terms of the MIT license.
 // Read LICENSE.txt for more information.
@@ -14,47 +14,20 @@
 namespace base {
 
 template<typename T>
-class Observers
-{
+class Observers {
 public:
   typedef T observer_type;
   typedef std::vector<observer_type*> list_type;
   typedef typename list_type::iterator iterator;
   typedef typename list_type::const_iterator const_iterator;
 
-  iterator begin() { return m_observers.begin(); }
-  iterator end() { return m_observers.end(); }
-  const_iterator begin() const { return m_observers.begin(); }
-  const_iterator end() const { return m_observers.end(); }
-
   bool empty() const { return m_observers.empty(); }
   size_t size() const { return m_observers.size(); }
 
-  Observers() : m_notifyingObservers(0) {
-  }
-
-  ~Observers() {
-#if _DEBUG
-    ASSERT(m_notifyingObservers == 0);
-
-    bool allEmpty = true;
-    for (iterator
-           it = this->begin(),
-           end = this->end(); it != end; ++it) {
-      if (*it) {
-        allEmpty = false;
-      }
-    }
-    ASSERT(allEmpty);
-#endif
-  }
-
   // Adds the observer in the collection. The observer is owned by the
   // collection and will be destroyed calling the T::dispose() member
   // function.
   void addObserver(observer_type* observer) {
-    clearNulls();
-
     ASSERT(std::find(m_observers.begin(), m_observers.end(), observer) == m_observers.end() && "You've tried to add an observer that already is in the collection");
     m_observers.push_back(observer);
   }
@@ -62,92 +35,59 @@ public:
   // Removes the observer from the collection. After calling this
   // function you own the observer so you have to dispose it.
   void removeObserver(observer_type* observer) {
-    clearNulls();
-
     iterator it = std::find(m_observers.begin(), m_observers.end(), observer);
-    if (it != end())
-      *it = (observer_type*)NULL;
+    if (it != m_observers.end())
+      m_observers.erase(it);
     else {
       ASSERT(false && "You've tried to remove an observer that isn't in the collection");
     }
   }
 
   void notifyObservers(void (observer_type::*method)()) {
-    {
-      IncDec incdec(m_notifyingObservers);
-      for (iterator
-             it = this->begin(),
-             end = this->end(); it != end; ++it) {
-        if (*it)
-          ((*it)->*method)();
-      }
+    list_type copy = m_observers;
+    for (iterator
+           it = copy.begin(),
+           end = copy.end(); it != end; ++it) {
+      if (*it)
+        ((*it)->*method)();
     }
-    clearNulls();
   }
 
   template<typename A1>
   void notifyObservers(void (observer_type::*method)(A1), A1 a1) {
-    {
-      IncDec incdec(m_notifyingObservers);
-      for (iterator
-             it = this->begin(),
-             end = this->end(); it != end; ++it) {
-        if (*it)
-          ((*it)->*method)(a1);
-      }
+    list_type copy = m_observers;
+    for (iterator
+           it = copy.begin(),
+           end = copy.end(); it != end; ++it) {
+      if (*it)
+        ((*it)->*method)(a1);
     }
-    clearNulls();
   }
 
   template<typename A1, typename A2>
   void notifyObservers(void (observer_type::*method)(A1, A2), A1 a1, A2 a2) {
-    {
-      IncDec incdec(m_notifyingObservers);
-      for (iterator
-             it = this->begin(),
-             end = this->end(); it != end; ++it) {
-        if (*it)
-          ((*it)->*method)(a1, a2);
-      }
+    list_type copy = m_observers;
+    for (iterator
+           it = copy.begin(),
+           end = copy.end(); it != end; ++it) {
+      if (*it)
+        ((*it)->*method)(a1, a2);
     }
-    clearNulls();
   }
 
   template<typename A1, typename A2, typename A3>
   void notifyObservers(void (observer_type::*method)(A1, A2, A3), A1 a1, A2 a2, A3 a3) {
-    {
-      IncDec incdec(m_notifyingObservers);
-      for (iterator
-             it = this->begin(),
-             end = this->end(); it != end; ++it) {
-        if (*it)
-          ((*it)->*method)(a1, a2, a3);
-      }
+    list_type copy = m_observers;
+    for (iterator
+           it = copy.begin(),
+           end = copy.end(); it != end; ++it) {
+      if (*it)
+        ((*it)->*method)(a1, a2, a3);
     }
-    clearNulls();
   }
 
 private:
-  void clearNulls() {
-    if (m_notifyingObservers > 0)
-      return;
-
-    iterator it;
-    while ((it = std::find(
-          m_observers.begin(),
-          m_observers.end(), (observer_type*)NULL)) != m_observers.end()) {
-      m_observers.erase(it);
-    }
-  }
-
-  struct IncDec {
-    int& m_value;
-    IncDec(int& value) : m_value(value) { ++m_value; }
-    ~IncDec() { --m_value; }
-  };
-
   list_type m_observers;
-  int m_notifyingObservers;
 };
 
 } // namespace base