diff options
| -rw-r--r-- | base/observer_list.h | 18 | ||||
| -rw-r--r-- | base/observer_list_unittest.cc | 23 | ||||
| -rw-r--r-- | content/browser/renderer_host/render_view_host.cc | 14 |
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)) |