Change nonstd::unique_ptr to take a custom deleter.

third_party/base/nonstd_unique_ptr.h

Index: third_party/base/nonstd_unique_ptr.h
diff --git a/third_party/base/nonstd_unique_ptr.h b/third_party/base/nonstd_unique_ptr.h
index 1d1c43f42fb4ac706cb1ce01795b03f986e6c58a..0038db15e28f7934ff17c448ae0ad38bd4db9739 100644
--- a/third_party/base/nonstd_unique_ptr.h
+++ b/third_party/base/nonstd_unique_ptr.h
@@ -73,6 +73,11 @@
 #include <stddef.h>
 #include <stdlib.h>
 
+#include <algorithm>

[Jeffrey Yasskin, 2015/09/19 00:00:47]

I don't see any uses of these two headers.

You are using <ostream>.

[Lei Zhang, 2015/09/23 00:38:56]

Done.

+#include <sstream>
+
+#include "template_util.h"
+
 namespace nonstd {
 
 // Replacement for move, but doesn't allow things that are already
@@ -82,47 +87,155 @@ T&& move(T& t) {
   return static_cast<T&&>(t);
 }
 
+// Function object which deletes its parameter, which must be a pointer.
+// If C is an array type, invokes 'delete[]' on the parameter; otherwise,
+// invokes 'delete'. The default deleter for scoped_ptr<T>.

[Jeffrey Yasskin, 2015/09/19 00:00:47]

s/scoped_ptr/unique_ptr/

[Lei Zhang, 2015/09/23 00:38:55]

Done.

+template <class T>
+struct DefaultDeleter {
+  DefaultDeleter() {}
+  template <typename U>
+  DefaultDeleter(const DefaultDeleter<U>& other) {
+    // IMPLEMENTATION NOTE: C++11 20.7.1.1.2p2 only provides this constructor
+    // if U* is implicitly convertible to T* and U is not an array type.
+    //
+    // Correct implementation should use SFINAE to disable this
+    // constructor. However, since there are no other 1-argument constructors,
+    // using a static_assert() based on is_convertible<> and requiring
+    // complete types is simpler and will cause compile failures for equivalent
+    // misuses.
+    //
+    // Note, the is_convertible<U*, T*> check also ensures that U is not an
+    // array. T is guaranteed to be a non-array, so any U* where U is an array
+    // cannot convert to T*.
+    enum { T_must_be_complete = sizeof(T) };
+    enum { U_must_be_complete = sizeof(U) };
+    static_assert((pdfium::base::is_convertible<U*, T*>::value),
+                  "U_ptr_must_implicitly_convert_to_T_ptr");
+  }
+  inline void operator()(T* ptr) const {
+    enum { type_must_be_complete = sizeof(T) };
+    delete ptr;
+  }
+};
+
+// Specialization of DefaultDeleter for array types.
+template <class T>
+struct DefaultDeleter<T[]> {
+  inline void operator()(T* ptr) const {
+    enum { type_must_be_complete = sizeof(T) };
+    delete[] ptr;
+  }
+
+ private:
+  // Disable this operator for any U != T because it is undefined to execute
+  // an array delete when the static type of the array mismatches the dynamic
+  // type.
+  //
+  // References:
+  //   C++98 [expr.delete]p3
+  //   http://cplusplus.github.com/LWG/lwg-defects.html#938
+  template <typename U>
+  void operator()(U* array) const;
+};
+
+template <class T, int n>
+struct DefaultDeleter<T[n]> {
+  // Never allow someone to declare something like scoped_ptr<int[10]>.
+  static_assert(sizeof(T) == -1, "do_not_use_array_with_size_as_type");
+};
+
+namespace internal {
+
+template <typename T>
+struct ShouldAbortOnSelfReset {

[Jeffrey Yasskin, 2015/09/19 00:00:47]

Can you drop this, and just use the C++11 behavior of reset() (call the deleter
on the original value if it's non-null)?

[Lei Zhang, 2015/09/23 00:38:55]

So, let the caller shoot themselves in the foot with a double free?

{
  Foo* raw_foo = new Foo;
  std::unique_ptr<Foo> foo(raw_foo);
  foo.reset(raw_foo);  // free once
}  // double free

[Jeffrey Yasskin, 2015/09/23 00:53:34]

Yeah. ASan should save us in a lot of cases, and we'll lose the protection
anyway when we go to a C++11 library.

+  template <typename U>
+  static pdfium::base::internal::NoType Test(const typename U::AllowSelfReset*);
+
+  template <typename U>
+  static pdfium::base::internal::YesType Test(...);
+
+  static const bool value =
+      sizeof(Test<T>(0)) == sizeof(pdfium::base::internal::YesType);
+};
+
 // Common implementation for both pointers to elements and pointers to
 // arrays. These are differentiated below based on the need to invoke
 // delete vs. delete[] as appropriate.
-template <class C>
+template <class C, class D>
 class unique_ptr_base {
  public:
-
   // The element type
   typedef C element_type;
 
-  explicit unique_ptr_base(C* p) : ptr_(p) { }
+  explicit unique_ptr_base(C* p) : data_(p) {}
+
+  // Initializer for deleters that have data parameters.
+  unique_ptr_base(C* p, const D& d) : data_(p, d) {}
 
   // Move constructor.
-  unique_ptr_base(unique_ptr_base<C>&& that) {
-    ptr_ = that.ptr_;
-    that.ptr_ = nullptr;
+  unique_ptr_base(unique_ptr_base<C, D>&& that)
+      : data_(that.release(), that.get_deleter()) {}
+
+  ~unique_ptr_base() {
+    enum { type_must_be_complete = sizeof(C) };
+    if (data_.ptr != nullptr) {
+      // Not using get_deleter() saves one function call in non-optimized
+      // builds.
+      static_cast<D&>(data_)(data_.ptr);
+    }
+  }
+
+  void reset(C* p = nullptr) {
+    // 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()
+    // 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|.
+    //
+    // During the transition period, set the stored pointer to nullptr 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.
+    C* old = data_.ptr;
+    data_.ptr = nullptr;
+    if (old != nullptr)
+      static_cast<D&>(data_)(old);
+    data_.ptr = p;
   }
 
   // Accessors to get the owned object.
   // operator* and operator-> will assert() if there is no current object.
   C& operator*() const {

[Jeffrey Yasskin, 2015/09/19 00:00:47]

Could you disable unique_ptr<T[]>::operator* and operator->? They don't exist in
std::unique_ptr for arrays, and I'd like as much compatibility as possible.

[Lei Zhang, 2015/09/23 00:38:55]

Moved out of the base impl.

-    assert(ptr_ != NULL);
-    return *ptr_;
+    assert(data_.ptr != nullptr);
+    return *data_.ptr;
   }
   C* operator->() const  {
-    assert(ptr_ != NULL);
-    return ptr_;
+    assert(data_.ptr != nullptr);
+    return data_.ptr;
   }
-  C* get() const { return ptr_; }
+  C* get() const { return data_.ptr; }
+  D& get_deleter() { return data_; }
+  const D& get_deleter() const { return data_; }
 
   // Comparison operators.
   // These return whether two unique_ptr refer to the same object, not just to
   // two different but equal objects.
-  bool operator==(C* p) const { return ptr_ == p; }
-  bool operator!=(C* p) const { return ptr_ != p; }
+  bool operator==(C* p) const { return data_.ptr == p; }
+  bool operator!=(C* p) const { return data_.ptr != p; }
 
   // Swap two scoped pointers.
   void swap(unique_ptr_base& p2) {
-    C* tmp = ptr_;
-    ptr_ = p2.ptr_;
-    p2.ptr_ = tmp;
+    Data tmp = data_;
+    data_ = p2.data_;
+    p2.data_ = tmp;
   }
 
   // Release a pointer.
@@ -131,58 +244,58 @@ class unique_ptr_base {
   // After this operation, this object will hold a NULL pointer,
   // and will not own the object any more.
   C* release() {
-    C* retVal = ptr_;
-    ptr_ = NULL;
-    return retVal;
+    C* ptr = data_.ptr;
+    data_.ptr = nullptr;
+    return ptr;
   }
 
   // Allow promotion to bool for conditional statements.
-  explicit operator bool() const { return ptr_ != NULL; }
+  explicit operator bool() const { return data_.ptr != nullptr; }
 
  protected:
-  C* ptr_;
+  // Use the empty base class optimization to allow us to have a D
+  // member, while avoiding any space overhead for it when D is an
+  // empty class.  See e.g. http://www.cantrip.org/emptyopt.html for a good
+  // discussion of this technique.
+  struct Data : public D {
+    explicit Data(C* ptr_in) : ptr(ptr_in) {}
+    Data(C* ptr_in, const D& other) : D(other), ptr(ptr_in) {}
+    C* ptr;
+  };
+
+  Data data_;
 };
 
+}  // namespace internal
+
 // Implementation for ordinary pointers using delete.
-template <class C>
-class unique_ptr : public unique_ptr_base<C> {
+template <class C, class D = DefaultDeleter<C>>
+class unique_ptr : public internal::unique_ptr_base<C, D> {
  public:
-  using unique_ptr_base<C>::ptr_;
+  // Constructor.  Defaults to initializing with nullptr.
+  unique_ptr() : internal::unique_ptr_base<C, D>(nullptr) {}
 
-  // Constructor. Defaults to initializing with NULL. There is no way
-  // to create an uninitialized unique_ptr. The input parameter must be
-  // allocated with new (not new[] - see below).
-  explicit unique_ptr(C* p = NULL) : unique_ptr_base<C>(p) { }
+  // Constructor.  Takes ownership of p.
+  explicit unique_ptr(C* p) : internal::unique_ptr_base<C, D>(p) {}
 
-  // Move constructor.
-  unique_ptr(unique_ptr<C>&& that) : unique_ptr_base<C>(nonstd::move(that)) {}
+  // Constructor.  Allows initialization of a stateful deleter.
+  unique_ptr(C* p, const D& d) : internal::unique_ptr_base<C, D>(p, d) {}

[Jeffrey Yasskin, 2015/09/19 00:00:47]

This isn't quite the same as C++11, but it should be good enough. The C++11
interface is moderately complicated.

[Lei Zhang, 2015/09/23 00:38:55]

Acknowledged.

 
-  // Destructor.  If there is a C object, delete it.
-  // We don't need to test ptr_ == NULL because C++ does that for us.
-  ~unique_ptr() {
-    enum { type_must_be_complete = sizeof(C) };
-    delete ptr_;
-  }
+  // Constructor.  Allows construction from a nullptr.
+  unique_ptr(decltype(nullptr)) : internal::unique_ptr_base<C, D>(nullptr) {}
 
-  // Reset.  Deletes the current owned object, if any.
-  // Then takes ownership of a new object, if given.
-  // this->reset(this->get()) works.
-  void reset(C* p = NULL) {
-    if (p != ptr_) {
-      enum { type_must_be_complete = sizeof(C) };
-      C* old_ptr = ptr_;
-      ptr_ = p;
-      delete old_ptr;
-    }
+  // Move constructor.
+  unique_ptr(unique_ptr&& that)
+      : internal::unique_ptr_base<C, D>(nonstd::move(that)) {}
+
+  // operator=.  Allows assignment from a nullptr. Deletes the currently owned
+  // object, if any.
+  unique_ptr& operator=(decltype(nullptr)) {
+    this->reset();
+    return *this;
   }
 
-private:
-  // Forbid comparison of unique_ptr types.  If C2 != C, it totally doesn't
-  // make sense, and if C2 == C, it still doesn't make sense because you should
-  // never have the same object owned by two different unique_ptrs.
-  template <class C2> bool operator==(unique_ptr<C2> const& p2) const;
-  template <class C2> bool operator!=(unique_ptr<C2> const& p2) const;
-
+ private:
   // Disallow evil constructors. It doesn't make sense to make a copy of
   // something that's allegedly unique.
   unique_ptr(const unique_ptr&) = delete;
@@ -190,48 +303,43 @@ private:
 };
 
 // Specialization for arrays using delete[].
-template <class C>
-class unique_ptr<C[]> : public unique_ptr_base<C> {
+template <class C, class D>
+class unique_ptr<C[], D> : public internal::unique_ptr_base<C, D> {
  public:
-  using unique_ptr_base<C>::ptr_;
-
-  // Constructor. Defaults to initializing with NULL. There is no way
-  // to create an uninitialized unique_ptr. The input parameter must be
-  // allocated with new[] (not new - see above).
-  explicit unique_ptr(C* p = NULL) : unique_ptr_base<C>(p) { }
+  // Constructor.  Defaults to initializing with nullptr.
+  unique_ptr() : internal::unique_ptr_base<C, D>(nullptr) {}
+
+  // Constructor. Stores the given array. Note that the argument's type
+  // must exactly match T*. In particular:
+  // - it cannot be a pointer to a type derived from T, because it is
+  //   inherently unsafe in the general case to access an array through a
+  //   pointer whose dynamic type does not match its static type (eg., if
+  //   T and the derived types had different sizes access would be
+  //   incorrectly calculated). Deletion is also always undefined
+  //   (C++98 [expr.delete]p3). If you're doing this, fix your code.
+  // - it cannot be const-qualified differently from T per unique_ptr spec
+  //   (http://cplusplus.github.com/LWG/lwg-active.html#2118). Users wanting
+  //   to work around this may use const_cast<const T*>().
+  explicit unique_ptr(C* p) : internal::unique_ptr_base<C, D>(p) {}

[Jeffrey Yasskin, 2015/09/19 00:00:48]

You need to also copy the "template <typename U> explicit scoped_ptr(U* array);"
in order to block non-C arguments.

And also "template <typename U> void reset(U* array);"

[Lei Zhang, 2015/09/23 00:38:55]

Done.

+
+  // Constructor.  Allows construction from a nullptr.
+  unique_ptr(decltype(nullptr)) : internal::unique_ptr_base<C, D>(nullptr) {}
 
   // Move constructor.
-  unique_ptr(unique_ptr<C>&& that) : unique_ptr_base<C>(nonstd::move(that)) {}
-
-  // Destructor.  If there is a C object, delete it.
-  // We don't need to test ptr_ == NULL because C++ does that for us.
-  ~unique_ptr() {
-    enum { type_must_be_complete = sizeof(C) };
-    delete[] ptr_;
-  }
-
-  // Reset.  Deletes the current owned object, if any.
-  // Then takes ownership of a new object, if given.
-  // this->reset(this->get()) works.
-  void reset(C* p = NULL) {
-    if (p != ptr_) {
-      enum { type_must_be_complete = sizeof(C) };
-      C* old_ptr = ptr_;
-      ptr_ = p;
-      delete[] old_ptr;
-    }
+  unique_ptr(unique_ptr&& that)
+      : internal::unique_ptr_base<C, D>(nonstd::move(that)) {}
+
+  // operator=.  Allows assignment from a nullptr. Deletes the currently owned
+  // array, if any.
+  unique_ptr& operator=(decltype(nullptr)) {
+    this->reset();
+    return *this;
   }
 
   // Support indexing since it is holding array.
-  C& operator[] (size_t i) { return ptr_[i]; }
-
-private:
-  // Forbid comparison of unique_ptr types.  If C2 != C, it totally doesn't
-  // make sense, and if C2 == C, it still doesn't make sense because you should
-  // never have the same object owned by two different unique_ptrs.
-  template <class C2> bool operator==(unique_ptr<C2> const& p2) const;
-  template <class C2> bool operator!=(unique_ptr<C2> const& p2) const;
+  C& operator[](size_t i) { return this->data_.ptr[i]; }
 
+ private:
   // Disallow evil constructors.  It doesn't make sense to make a copy of
   // something that's allegedly unique.
   unique_ptr(const unique_ptr&) = delete;
@@ -254,6 +362,11 @@ bool operator!=(C* p1, const unique_ptr<C>& p2) {
   return p1 != p2.get();
 }
 
+template <typename T>
+std::ostream& operator<<(std::ostream& out, const unique_ptr<T>& p) {
+  return out << p.get();
+}
+
 }  // namespace nonstd
 
 #endif  // NONSTD_UNIQUE_PTR_H_