Allow custom deleters to opt out of self reset checks for scoped_ptr.

The self-reset check makes sense for the default deleters, because it
would otherwise leave a dangling pointer stored in the scoped_ptr.
However, a custom deleter might actually decrement a reference count
under the hood. This self-reset check can make assignment operators
implementation a lot uglier. One example is net's KeyPair: because
there might be a self-assignment, the original code needed to proxy
the incoming scoped_ptrs via a stack temporary before moving them
into their final location.

BUG=418347

Review URL: https://codereview.chromium.org/610533003

Cr-Commit-Position: refs/heads/master@{#299571}

base/memory/scoped_ptr.h

@@ -184,6 +184,17 @@ template <typename T> struct IsNotRefCounted {
   };
 };
 
+template <typename T>
+struct ShouldAbortOnSelfReset {
+  template <typename U>
+  static NoType Test(const typename U::AllowSelfReset*);
+
+  template <typename U>
+  static YesType Test(...);
+
+  static const bool value = sizeof(Test<T>(0)) == sizeof(YesType);
+};
+
 // Minimal implementation of the core logic of scoped_ptr, suitable for
 // reuse in both scoped_ptr and its specializations.
 template <class T, class D>
@@ -222,9 +233,9 @@ class scoped_ptr_impl {
   }
 
   void reset(T* p) {
-    // This is a self-reset, which is no longer allowed: http://crbug.com/162971
-    if (p != nullptr && p == data_.ptr)
-      abort();
+    // This is a self-reset, which is no longer allowed for default deleters:
+    // https://crbug.com/162971
+    assert(!ShouldAbortOnSelfReset<D>::value || p == nullptr || p != data_.ptr);
 
     // Note that running data_.ptr = p can lead to undefined behavior if
     // get_deleter()(get()) deletes this. In order to prevent this, reset()

base/memory/scoped_ptr_unittest.cc

@@ -656,3 +656,41 @@ TEST(ScopedPtrTest, Conversion) {
   scoped_ptr<Super> super2 = SubClassReturn();
   super2 = SubClassReturn();
 }
+
+// Android death tests don't work properly with assert(). Yay.
+#if !defined(NDEBUG) && defined(GTEST_HAS_DEATH_TEST) && !defined(OS_ANDROID)
+TEST(ScopedPtrTest, SelfResetAbortsWithDefaultDeleter) {
+  scoped_ptr<int> x(new int);
+  EXPECT_DEATH(x.reset(x.get()), "");
+}
+
+TEST(ScopedPtrTest, SelfResetAbortsWithDefaultArrayDeleter) {
+  scoped_ptr<int[]> y(new int[4]);
+  EXPECT_DEATH(y.reset(y.get()), "");
+}
+
+TEST(ScopedPtrTest, SelfResetAbortsWithDefaultFreeDeleter) {
+  scoped_ptr<int, base::FreeDeleter> z(static_cast<int*>(malloc(sizeof(int))));
+  EXPECT_DEATH(z.reset(z.get()), "");
+}
+
+// A custom deleter that doesn't opt out should still crash.
+TEST(ScopedPtrTest, SelfResetAbortsWithCustomDeleter) {
+  struct CustomDeleter {
+    inline void operator()(int* x) { delete x; }
+  };
+  scoped_ptr<int, CustomDeleter> x(new int);
+  EXPECT_DEATH(x.reset(x.get()), "");
+}
+#endif
+
+TEST(ScopedPtrTest, SelfResetWithCustomDeleterOptOut) {
+  // A custom deleter should be able to opt out of self-reset abort behavior.
+  struct NoOpDeleter {
+    typedef void AllowSelfReset;
+    inline void operator()(int*) {}
+  };
+  scoped_ptr<int> owner(new int);
+  scoped_ptr<int, NoOpDeleter> x(owner.get());
+  x.reset(x.get());
+}

crypto/scoped_nss_types.h

@@ -16,13 +16,15 @@ namespace crypto {
 
 template <typename Type, void (*Destroyer)(Type*)>
 struct NSSDestroyer {
+  typedef void AllowSelfReset;
   void operator()(Type* ptr) const {
     Destroyer(ptr);
   }
 };
 
 template <typename Type, void (*Destroyer)(Type*, PRBool), PRBool freeit>
 struct NSSDestroyer1 {
+  typedef void AllowSelfReset;
   void operator()(Type* ptr) const {
     Destroyer(ptr, freeit);
   }

crypto/scoped_openssl_types.h

@@ -22,6 +22,7 @@ namespace crypto {
 // base::internal::RunnableAdapter<>, but that's far too heavy weight.
 template <typename Type, void (*Destroyer)(Type*)>
 struct OpenSSLDestroyer {
+  typedef void AllowSelfReset;
   void operator()(Type* ptr) const { Destroyer(ptr); }
 };
 

net/ssl/openssl_client_key_store.cc

@@ -6,6 +6,7 @@
 
 #include <openssl/evp.h>
 #include <openssl/x509.h>
+#include <algorithm>
 
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/singleton.h"
@@ -50,15 +51,14 @@ OpenSSLClientKeyStore::KeyPair::KeyPair(const KeyPair& other)
       private_key(EVP_PKEY_dup(other.private_key.get())) {
 }
 
-void OpenSSLClientKeyStore::KeyPair::operator=(const KeyPair& other) {
-  // Use a temporary ScopedEVP_PKEY because scoped_ptr does not allow resetting
-  // to the current value, even though it's safe here.
-  crypto::ScopedEVP_PKEY public_key_tmp(EVP_PKEY_dup(other.public_key.get()));
-  crypto::ScopedEVP_PKEY private_key_tmp(EVP_PKEY_dup(other.private_key.get()));
-  public_key.reset();
-  public_key = public_key_tmp.Pass();
-  private_key.reset();
-  private_key = private_key_tmp.Pass();
+void OpenSSLClientKeyStore::KeyPair::operator=(KeyPair other) {
+  swap(other);
+}
+
+void OpenSSLClientKeyStore::KeyPair::swap(KeyPair& other) {
+  using std::swap;
+  swap(public_key, other.public_key);
+  swap(private_key, other.private_key);
 }
 
 int OpenSSLClientKeyStore::FindKeyPairIndex(EVP_PKEY* public_key) {

net/ssl/openssl_client_key_store.h

@@ -73,7 +73,9 @@ class NET_EXPORT OpenSSLClientKeyStore {
    public:
     explicit KeyPair(EVP_PKEY* pub_key, EVP_PKEY* priv_key);
     KeyPair(const KeyPair& other);
-    void operator=(const KeyPair& other);
+    // Intentionally pass by value, in order to use the copy-and-swap idiom.
+    void operator=(KeyPair other);
+    void swap(KeyPair& other);
     ~KeyPair();
 
     crypto::ScopedEVP_PKEY public_key;