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

TBR=jam@chromium.org
Review URL: http://codereview.chromium.org/7134008

TBR=thakis@chromium.org
Review URL: http://codereview.chromium.org/7129036

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

3 files changed, 39 insertions, 16 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
diff --git a/content/browser/renderer_host/render_view_host.cc b/content/browser/renderer_host/render_view_host.cc
index 1e91447..f5aa764 100644
--- a/content/browser/renderer_host/render_view_host.cc
+++ b/content/browser/renderer_host/render_view_host.cc
@@ -633,15 +633,11 @@ bool RenderViewHost::OnMessageReceived(const IPC::Message& msg) {
     if (!content::SwappedOutMessages::CanHandleWhileSwappedOut(msg))
       return true;
 
-  {
-    // delegate_->OnMessageReceived can end up deleting |this|, in which case
-    // the destructor for ObserverListBase::Iterator would access the deleted
-    // observers_.
-    ObserverListBase<RenderViewHostObserver>::Iterator it(observers_);
-    RenderViewHostObserver* observer;
-    while ((observer = it.GetNext()) != NULL)
-      if (observer->OnMessageReceived(msg))
-        return true;
+  ObserverListBase<RenderViewHostObserver>::Iterator it(observers_);
+  RenderViewHostObserver* observer;
+  while ((observer = it.GetNext()) != NULL) {
+    if (observer->OnMessageReceived(msg))
+      return true;
   }
 
   if (delegate_->OnMessageReceived(msg))