Revert 88284 - Revert 88151 (see crbug.com/85296) - Fix user-after-free error with ObserverList. ...

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
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88484

[Nico, 2011-06-09 05:36:34]

Lgtm
On Jun 8, 2011 8:41 PM, <akalin@chromium.org> wrote:
> Reviewers: Nico,
>
> Description:
> 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
>
> Please review this at http://codereview.chromium.org/7129036/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src/
>
> Affected files:
> M base/observer_list.h
> M base/observer_list_unittest.cc
> M content/browser/renderer_host/render_view_host.cc
>
>
> Index: base/observer_list.h
> ===================================================================
> --- base/observer_list.h (revision 88483)
> +++ base/observer_list.h (working copy)
> @@ -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_;
> };
> Index: base/observer_list_unittest.cc
> ===================================================================
> --- base/observer_list_unittest.cc (revision 88483)
> +++ base/observer_list_unittest.cc (working copy)
> @@ -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
> Index: content/browser/renderer_host/render_view_host.cc
> ===================================================================
> --- content/browser/renderer_host/render_view_host.cc (revision 88483)
> +++ content/browser/renderer_host/render_view_host.cc (working copy)
> @@ -633,15 +633,11 @@
> 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))
>
>