Revert 88284 - Revert 88151 (see crbug.com/85296) - 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.
Relanding 88151 now that sync fixes (88483, 88472) are in.
BUG=84919
Review URL: http://codereview.chromium.org/7127001
[email protected]
Review URL: http://codereview.chromium.org/7134008
[email protected]
Review URL: http://codereview.chromium.org/7129036
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@88484 0039d316-1c4b-4281-b951-d872f2087c98
diff --git a/base/observer_list.h b/base/observer_list.h
index b8c2ae427..d30cc6ef 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 @@
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 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 @@
}
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 8315a2b4..d0d2001 100644
--- a/base/observer_list_unittest.cc
+++ b/base/observer_list_unittest.cc
@@ -422,4 +422,27 @@
<< "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
