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

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

1 files changed, 22 insertions, 19 deletions

diff --git a/base/memory/scoped_ptr.h b/base/memory/scoped_ptr.h
index c6d91df..7098e14 100644
--- a/base/memory/scoped_ptr.h
+++ b/base/memory/scoped_ptr.h
@@ -223,26 +223,29 @@ class scoped_ptr_impl {
   }
 
   void reset(T* p) {
-    // 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.
+    // 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.
     //
-    // 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);
-    }
+    // 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|.
+    //
+    // 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;
   }
 
   T* get() const { return data_.ptr; }