Improve Memory Management of Result class

The way in which the Result class is used in Dawn can be fragile
with respect to memory management because the caller of AcquireError
must know they need to delete the returned pointer or a memory leak
will occur. We've had a couple of instances where developers have
accidentally left out the delete call and managed to get past code
review.

This CL changes the Result class so that it assumes the error is
allocated on the heap and forces the caller to use unique_ptr when
calling AcquireError.

Bug:dawn:320
Change-Id: I13ec953b0c37eaafbd6ce93c2f719b4743676acb
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/14960
Reviewed-by: Austin Eng <enga@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Rafael Cintron <rafael.cintron@microsoft.com>

src/common/Result.h

@@ -20,6 +20,7 @@
 
 #include <cstddef>
 #include <cstdint>
+#include <memory>
 #include <type_traits>
 #include <utility>
 
@@ -38,10 +39,10 @@
 template <typename T, typename E>
 class Result;
 
-// The interface of Result<T, E> shoud look like the following.
+// The interface of Result<T, E> should look like the following.
 //  public:
 //    Result(T&& success);
-//    Result(E&& error);
+//    Result(std::unique_ptr<E> error);
 //
 //    Result(Result<T, E>&& other);
 //    Result<T, E>& operator=(Result<T, E>&& other);
@@ -52,18 +53,18 @@ class Result;
 //    bool IsSuccess() const;
 //
 //    T&& AcquireSuccess();
-//    E&& AcquireError();
+//    std::unique_ptr<E> AcquireError();
 
 // Specialization of Result for returning errors only via pointers. It is basically a pointer
 // where nullptr is both Success and Empty.
 template <typename E>
-class DAWN_NO_DISCARD Result<void, E*> {
+class DAWN_NO_DISCARD Result<void, E> {
   public:
     Result();
-    Result(E* error);
+    Result(std::unique_ptr<E> error);
 
-    Result(Result<void, E*>&& other);
-    Result<void, E*>& operator=(Result<void, E>&& other);
+    Result(Result<void, E>&& other);
+    Result<void, E>& operator=(Result<void, E>&& other);
 
     ~Result();
 
@@ -71,10 +72,10 @@ class DAWN_NO_DISCARD Result<void, E*> {
     bool IsSuccess() const;
 
     void AcquireSuccess();
-    E* AcquireError();
+    std::unique_ptr<E> AcquireError();
 
   private:
-    E* mError = nullptr;
+    std::unique_ptr<E> mError;
 };
 
 // Uses SFINAE to try to get alignof(T) but fallback to Default if T isn't defined.
@@ -108,7 +109,7 @@ namespace detail {
 }  // namespace detail
 
 template <typename T, typename E>
-class DAWN_NO_DISCARD Result<T*, E*> {
+class DAWN_NO_DISCARD Result<T*, E> {
   public:
     static_assert(alignof_if_defined_else_default<T, 4> >= 4,
                   "Result<T*, E*> reserves two bits for tagging pointers");
@@ -116,13 +117,13 @@ class DAWN_NO_DISCARD Result<T*, E*> {
                   "Result<T*, E*> reserves two bits for tagging pointers");
 
     Result(T* success);
-    Result(E* error);
+    Result(std::unique_ptr<E> error);
 
     // Support returning a Result<T*, E*> from a Result<TChild*, E*>
     template <typename TChild>
-    Result(Result<TChild*, E*>&& other);
+    Result(Result<TChild*, E>&& other);
     template <typename TChild>
-    Result<T*, E*>& operator=(Result<TChild*, E>&& other);
+    Result<T*, E>& operator=(Result<TChild*, E>&& other);
 
     ~Result();
 
@@ -130,7 +131,7 @@ class DAWN_NO_DISCARD Result<T*, E*> {
     bool IsSuccess() const;
 
     T* AcquireSuccess();
-    E* AcquireError();
+    std::unique_ptr<E> AcquireError();
 
   private:
     template <typename T2, typename E2>
@@ -140,7 +141,7 @@ class DAWN_NO_DISCARD Result<T*, E*> {
 };
 
 template <typename T, typename E>
-class DAWN_NO_DISCARD Result<const T*, E*> {
+class DAWN_NO_DISCARD Result<const T*, E> {
   public:
     static_assert(alignof_if_defined_else_default<T, 4> >= 4,
                   "Result<T*, E*> reserves two bits for tagging pointers");
@@ -148,10 +149,10 @@ class DAWN_NO_DISCARD Result<const T*, E*> {
                   "Result<T*, E*> reserves two bits for tagging pointers");
 
     Result(const T* success);
-    Result(E* error);
+    Result(std::unique_ptr<E> error);
 
-    Result(Result<const T*, E*>&& other);
-    Result<const T*, E*>& operator=(Result<const T*, E>&& other);
+    Result(Result<const T*, E>&& other);
+    Result<const T*, E>& operator=(Result<const T*, E>&& other);
 
     ~Result();
 
@@ -159,7 +160,7 @@ class DAWN_NO_DISCARD Result<const T*, E*> {
     bool IsSuccess() const;
 
     const T* AcquireSuccess();
-    E* AcquireError();
+    std::unique_ptr<E> AcquireError();
 
   private:
     intptr_t mPayload = detail::kEmptyPayload;
@@ -172,7 +173,7 @@ template <typename T, typename E>
 class DAWN_NO_DISCARD Result {
   public:
     Result(T&& success);
-    Result(E&& error);
+    Result(std::unique_ptr<E> error);
 
     Result(Result<T, E>&& other);
     Result<T, E>& operator=(Result<T, E>&& other);
@@ -183,7 +184,7 @@ class DAWN_NO_DISCARD Result {
     bool IsSuccess() const;
 
     T&& AcquireSuccess();
-    E&& AcquireError();
+    std::unique_ptr<E> AcquireError();
 
   private:
     enum PayloadType {
@@ -193,56 +194,52 @@ class DAWN_NO_DISCARD Result {
     };
     PayloadType mType;
 
-    E mError;
+    std::unique_ptr<E> mError;
     T mSuccess;
 };
 
-// Implementation of Result<void, E*>
+// Implementation of Result<void, E>
 template <typename E>
-Result<void, E*>::Result() {
+Result<void, E>::Result() {
 }
 
 template <typename E>
-Result<void, E*>::Result(E* error) : mError(error) {
+Result<void, E>::Result(std::unique_ptr<E> error) : mError(std::move(error)) {
 }
 
 template <typename E>
-Result<void, E*>::Result(Result<void, E*>&& other) : mError(other.mError) {
-    other.mError = nullptr;
+Result<void, E>::Result(Result<void, E>&& other) : mError(std::move(other.mError)) {
 }
 
 template <typename E>
-Result<void, E*>& Result<void, E*>::operator=(Result<void, E>&& other) {
+Result<void, E>& Result<void, E>::operator=(Result<void, E>&& other) {
     ASSERT(mError == nullptr);
-    mError = other.mError;
-    other.mError = nullptr;
+    mError = std::move(other.mError);
     return *this;
 }
 
 template <typename E>
-Result<void, E*>::~Result() {
+Result<void, E>::~Result() {
     ASSERT(mError == nullptr);
 }
 
 template <typename E>
-bool Result<void, E*>::IsError() const {
+bool Result<void, E>::IsError() const {
     return mError != nullptr;
 }
 
 template <typename E>
-bool Result<void, E*>::IsSuccess() const {
+bool Result<void, E>::IsSuccess() const {
     return mError == nullptr;
 }
 
 template <typename E>
-void Result<void, E*>::AcquireSuccess() {
+void Result<void, E>::AcquireSuccess() {
 }
 
 template <typename E>
-E* Result<void, E*>::AcquireError() {
-    E* error = mError;
-    mError = nullptr;
-    return error;
+std::unique_ptr<E> Result<void, E>::AcquireError() {
+    return std::move(mError);
 }
 
 // Implementation details of the tagged pointer Results
@@ -262,25 +259,26 @@ namespace detail {
 
 }  // namespace detail
 
-// Implementation of Result<T*, E*>
+// Implementation of Result<T*, E>
 template <typename T, typename E>
-Result<T*, E*>::Result(T* success) : mPayload(detail::MakePayload(success, detail::Success)) {
+Result<T*, E>::Result(T* success) : mPayload(detail::MakePayload(success, detail::Success)) {
 }
 
 template <typename T, typename E>
-Result<T*, E*>::Result(E* error) : mPayload(detail::MakePayload(error, detail::Error)) {
+Result<T*, E>::Result(std::unique_ptr<E> error)
+    : mPayload(detail::MakePayload(error.release(), detail::Error)) {
 }
 
 template <typename T, typename E>
 template <typename TChild>
-Result<T*, E*>::Result(Result<TChild*, E*>&& other) : mPayload(other.mPayload) {
+Result<T*, E>::Result(Result<TChild*, E>&& other) : mPayload(other.mPayload) {
     other.mPayload = detail::kEmptyPayload;
     static_assert(std::is_same<T, TChild>::value || std::is_base_of<T, TChild>::value, "");
 }
 
 template <typename T, typename E>
 template <typename TChild>
-Result<T*, E*>& Result<T*, E*>::operator=(Result<TChild*, E>&& other) {
+Result<T*, E>& Result<T*, E>::operator=(Result<TChild*, E>&& other) {
     ASSERT(mPayload == detail::kEmptyPayload);
     static_assert(std::is_same<T, TChild>::value || std::is_base_of<T, TChild>::value, "");
     mPayload = other.mPayload;
@@ -289,51 +287,52 @@ Result<T*, E*>& Result<T*, E*>::operator=(Result<TChild*, E>&& other) {
 }
 
 template <typename T, typename E>
-Result<T*, E*>::~Result() {
+Result<T*, E>::~Result() {
     ASSERT(mPayload == detail::kEmptyPayload);
 }
 
 template <typename T, typename E>
-bool Result<T*, E*>::IsError() const {
+bool Result<T*, E>::IsError() const {
     return detail::GetPayloadType(mPayload) == detail::Error;
 }
 
 template <typename T, typename E>
-bool Result<T*, E*>::IsSuccess() const {
+bool Result<T*, E>::IsSuccess() const {
     return detail::GetPayloadType(mPayload) == detail::Success;
 }
 
 template <typename T, typename E>
-T* Result<T*, E*>::AcquireSuccess() {
+T* Result<T*, E>::AcquireSuccess() {
     T* success = detail::GetSuccessFromPayload<T>(mPayload);
     mPayload = detail::kEmptyPayload;
     return success;
 }
 
 template <typename T, typename E>
-E* Result<T*, E*>::AcquireError() {
-    E* error = detail::GetErrorFromPayload<E>(mPayload);
+std::unique_ptr<E> Result<T*, E>::AcquireError() {
+    std::unique_ptr<E> error(detail::GetErrorFromPayload<E>(mPayload));
     mPayload = detail::kEmptyPayload;
-    return error;
+    return std::move(error);
 }
 
 // Implementation of Result<const T*, E*>
 template <typename T, typename E>
-Result<const T*, E*>::Result(const T* success)
+Result<const T*, E>::Result(const T* success)
     : mPayload(detail::MakePayload(success, detail::Success)) {
 }
 
 template <typename T, typename E>
-Result<const T*, E*>::Result(E* error) : mPayload(detail::MakePayload(error, detail::Error)) {
+Result<const T*, E>::Result(std::unique_ptr<E> error)
+    : mPayload(detail::MakePayload(error.release(), detail::Error)) {
 }
 
 template <typename T, typename E>
-Result<const T*, E*>::Result(Result<const T*, E*>&& other) : mPayload(other.mPayload) {
+Result<const T*, E>::Result(Result<const T*, E>&& other) : mPayload(other.mPayload) {
     other.mPayload = detail::kEmptyPayload;
 }
 
 template <typename T, typename E>
-Result<const T*, E*>& Result<const T*, E*>::operator=(Result<const T*, E>&& other) {
+Result<const T*, E>& Result<const T*, E>::operator=(Result<const T*, E>&& other) {
     ASSERT(mPayload == detail::kEmptyPayload);
     mPayload = other.mPayload;
     other.mPayload = detail::kEmptyPayload;
@@ -341,32 +340,32 @@ Result<const T*, E*>& Result<const T*, E*>::operator=(Result<const T*, E>&& othe
 }
 
 template <typename T, typename E>
-Result<const T*, E*>::~Result() {
+Result<const T*, E>::~Result() {
     ASSERT(mPayload == detail::kEmptyPayload);
 }
 
 template <typename T, typename E>
-bool Result<const T*, E*>::IsError() const {
+bool Result<const T*, E>::IsError() const {
     return detail::GetPayloadType(mPayload) == detail::Error;
 }
 
 template <typename T, typename E>
-bool Result<const T*, E*>::IsSuccess() const {
+bool Result<const T*, E>::IsSuccess() const {
     return detail::GetPayloadType(mPayload) == detail::Success;
 }
 
 template <typename T, typename E>
-const T* Result<const T*, E*>::AcquireSuccess() {
+const T* Result<const T*, E>::AcquireSuccess() {
     T* success = detail::GetSuccessFromPayload<T>(mPayload);
     mPayload = detail::kEmptyPayload;
     return success;
 }
 
 template <typename T, typename E>
-E* Result<const T*, E*>::AcquireError() {
-    E* error = detail::GetErrorFromPayload<E>(mPayload);
+std::unique_ptr<E> Result<const T*, E>::AcquireError() {
+    std::unique_ptr<E> error(detail::GetErrorFromPayload<E>(mPayload));
     mPayload = detail::kEmptyPayload;
-    return error;
+    return std::move(error);
 }
 
 // Implementation of Result<T, E>
@@ -375,7 +374,7 @@ Result<T, E>::Result(T&& success) : mType(Success), mSuccess(std::move(success))
 }
 
 template <typename T, typename E>
-Result<T, E>::Result(E&& error) : mType(Error), mError(std::move(error)) {
+Result<T, E>::Result(std::unique_ptr<E> error) : mType(Error), mError(std::move(error)) {
 }
 
 template <typename T, typename E>
@@ -415,7 +414,7 @@ T&& Result<T, E>::AcquireSuccess() {
 }
 
 template <typename T, typename E>
-E&& Result<T, E>::AcquireError() {
+std::unique_ptr<E> Result<T, E>::AcquireError() {
     ASSERT(mType == Error);
     mType = Acquired;
     return std::move(mError);