Fix user-after-free error with ObserverList. The problem is that if an ObserverListBase::Iterator is on the stack and one of the observers deletes the object holding the list, Iterator's destructor will use the deleted list.

BUG=84919
Review URL: http://codereview.chromium.org/7127001

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@88151 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 34 insertions, 7 deletions

diff --git a/base/observer_list.h b/base/observer_list.h
index b8c2ae4..d30cc6e 100644
--- a/base/observer_list.h
+++ b/base/observer_list.h
@@ -12,6 +12,7 @@
 
 #include "base/basictypes.h"
 #include "base/logging.h"
+#include "base/memory/weak_ptr.h"
 
 ///////////////////////////////////////////////////////////////////////////////
 //
@@ -61,7 +62,8 @@ template <typename ObserverType>
 class ObserverListThreadSafe;
 
 template <class ObserverType>
-class ObserverListBase {
+class ObserverListBase
+    : public base::SupportsWeakPtr<ObserverListBase<ObserverType> > {
  public:
   // Enumeration of which observers are notified.
   enum NotificationType {
@@ -79,21 +81,23 @@ class ObserverListBase {
   class Iterator {
    public:
     Iterator(ObserverListBase<ObserverType>& list)
-        : list_(list),
+        : list_(list.AsWeakPtr()),
           index_(0),
           max_index_(list.type_ == NOTIFY_ALL ?
                      std::numeric_limits<size_t>::max() :
                      list.observers_.size()) {
-      ++list_.notify_depth_;
+      ++list_->notify_depth_;
     }
 
     ~Iterator() {
-      if (--list_.notify_depth_ == 0)
-        list_.Compact();
+      if (list_ && --list_->notify_depth_ == 0)
+        list_->Compact();
     }
 
     ObserverType* GetNext() {
-      ListType& observers = list_.observers_;
+      if (!list_)
+        return NULL;
+      ListType& observers = list_->observers_;
       // Advance if the current element is null
       size_t max_index = std::min(max_index_, observers.size());
       while (index_ < max_index && !observers[index_])
@@ -102,7 +106,7 @@ class ObserverListBase {
     }
 
    private:
-    ObserverListBase<ObserverType>& list_;
+    base::WeakPtr<ObserverListBase<ObserverType> > list_;
     size_t index_;
     size_t max_index_;
   };
diff --git a/base/observer_list_unittest.cc b/base/observer_list_unittest.cc
index 8315a2b..d0d2001 100644
--- a/base/observer_list_unittest.cc
+++ b/base/observer_list_unittest.cc
@@ -422,4 +422,27 @@ TEST(ObserverListTest, ClearNotifyExistingOnly) {
       << "Adder should not observe, so sum should still be 0.";
 }
 
+class ListDestructor : public Foo {
+ public:
+  explicit ListDestructor(ObserverList<Foo>* list) : list_(list) {}
+  virtual void Observe(int x) {
+    delete list_;
+  }
+  virtual ~ListDestructor() { }
+  int total;
+ private:
+  ObserverList<Foo>* list_;
+};
+
+
+TEST(ObserverListTest, IteratorOutlivesList) {
+  ObserverList<Foo>* observer_list = new ObserverList<Foo>;
+  ListDestructor a(observer_list);
+  observer_list->AddObserver(&a);
+
+  FOR_EACH_OBSERVER(Foo, *observer_list, Observe(0));
+  // If this test fails, there'll be Valgrind errors when this function goes out
+  // of scope.
+}
+
 }  // namespace