Revert 184179

This is trigger a latent issue in views which results in a memory leak
each time a Widget is destroyed.

> Update scoped_ptr<T>::reset() to more closely match std::unique_ptr<T>.
> 
> Remove the no-op behavior of a scoped_ptr "self-reset", and update the
> reset() method to detect problematic dependencies on the sequence of
> events. Eventually, reset() will be updated to exactly match the
> implementation of std::unique_ptr<T> by setting data_.ptr to the new
> value before deleting the old value.
> 
> However, this will expose latent bugs where a destructor invoked
> transitively by reset() attempts to dereference the same scoped_ptr.
> Relying on the value of get() in this instance will dispatch calls to
> the incorrect object. As a temporary measure to detect this class of
> bugs, set data_.ptr to NULL during deletion so that it results in a
> crash.
> 
> BUG=162971,176091
> 
> Review URL: https://codereview.chromium.org/12223113

TBR=dcheng@chromium.org
Review URL: https://codereview.chromium.org/12317084

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

1 files changed, 19 insertions, 22 deletions

diff --git a/base/memory/scoped_ptr.h b/base/memory/scoped_ptr.h
index 7098e14..c6d91df 100644
--- a/base/memory/scoped_ptr.h
+++ b/base/memory/scoped_ptr.h
@@ -223,29 +223,26 @@ class scoped_ptr_impl {
   }
 
   void reset(T* p) {
-    // This is a self-reset, which is no longer allowed: http://crbug.com/162971
-    if (p != NULL && p == data_.ptr)
-      abort();
-
-    // Note that running data_.ptr = p can lead to undefined behavior if
-    // get_deleter()(get()) deletes this. In order to pevent this, reset()
-    // should update the stored pointer before deleting its old value.
-    //
-    // However, changing reset() to use that behavior may cause current code to
-    // break in unexpected ways. If the destruction of the owned object
-    // dereferences the scoped_ptr when it is destroyed by a call to reset(),
-    // then it will incorrectly dispatch calls to |p| rather than the original
-    // value of |data_.ptr|.
+    // This self-reset check is deprecated.
+    // this->reset(this->get()) currently works, but it is DEPRECATED, and
+    // will be removed once we verify that no one depends on it.
     //
-    // During the transition period, set the stored pointer to NULL while
-    // deleting the object. Eventually, this safety check will be removed to
-    // prevent the scenario initially described from occuring and
-    // http://crbug.com/176091 can be closed.
-    T* old = data_.ptr;
-    data_.ptr = NULL;
-    if (old != NULL)
-      static_cast<D&>(data_)(old);
-    data_.ptr = p;
+    // TODO(ajwong): Change this behavior to match unique_ptr<>.
+    // http://crbug.com/162971
+    if (p != data_.ptr) {
+      if (data_.ptr != NULL) {
+        // Note that this can lead to undefined behavior and memory leaks
+        // in the unlikely but possible case that get_deleter()(get())
+        // indirectly deletes this. The fix is to reset ptr_ before deleting
+        // its old value, but first we need to clean up the code that relies
+        // on the current sequencing.
+        static_cast<D&>(data_)(data_.ptr);
+      }
+      data_.ptr = p;
+    } else {
+      // If p is non-NULL, this is a deprecated self-reset.
+      assert(p == NULL);
+    }
   }
 
   T* get() const { return data_.ptr; }