From 69c68d01b2420e77283166ffcc7ddb4b0fbc2505 Mon Sep 17 00:00:00 2001
From: Rafael Cintron <rafael.cintron@microsoft.com>
Date: Fri, 10 Jan 2020 17:58:28 +0000
Subject: [PATCH] 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>
---
 BUILD.gn                                      |   1 -
 src/common/Result.h                           | 125 +++++++-------
 src/dawn_native/Device.cpp                    |   3 +-
 src/dawn_native/Device.h                      |   2 +-
 src/dawn_native/EncodingContext.h             |   3 +-
 src/dawn_native/Error.cpp                     |  35 ----
 src/dawn_native/Error.h                       |  63 +++----
 src/dawn_native/ErrorData.cpp                 |  10 +-
 src/dawn_native/ErrorData.h                   |   6 +-
 src/dawn_native/Instance.cpp                  |   3 +-
 src/tests/unittests/ErrorTests.cpp            |  38 ++---
 src/tests/unittests/ResultTests.cpp           | 158 +++++++++---------
 .../white_box/VulkanErrorInjectorTests.cpp    |   2 +-
 13 files changed, 194 insertions(+), 255 deletions(-)
 delete mode 100644 src/dawn_native/Error.cpp

diff --git a/BUILD.gn b/BUILD.gn
index 5287bee66d..c7df5827aa 100644
--- a/BUILD.gn
+++ b/BUILD.gn
@@ -204,7 +204,6 @@ source_set("libdawn_native_sources") {
     "src/dawn_native/DynamicUploader.h",
     "src/dawn_native/EncodingContext.cpp",
     "src/dawn_native/EncodingContext.h",
-    "src/dawn_native/Error.cpp",
     "src/dawn_native/Error.h",
     "src/dawn_native/ErrorData.cpp",
     "src/dawn_native/ErrorData.h",
diff --git a/src/common/Result.h b/src/common/Result.h
index 3e33052fb3..7dc5e2df9e 100644
--- a/src/common/Result.h
+++ b/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);
diff --git a/src/dawn_native/Device.cpp b/src/dawn_native/Device.cpp
index 3926a89286..f5609dc9c7 100644
--- a/src/dawn_native/Device.cpp
+++ b/src/dawn_native/Device.cpp
@@ -118,10 +118,9 @@ namespace dawn_native {
         HandleError(type, message);
     }
 
-    void DeviceBase::ConsumeError(ErrorData* error) {
+    void DeviceBase::ConsumeError(std::unique_ptr<ErrorData> error) {
         ASSERT(error != nullptr);
         HandleError(error->GetType(), error->GetMessage().c_str());
-        delete error;
     }
 
     void DeviceBase::SetUncapturedErrorCallback(wgpu::ErrorCallback callback, void* userdata) {
diff --git a/src/dawn_native/Device.h b/src/dawn_native/Device.h
index aa6471fbc5..9d0947d2b7 100644
--- a/src/dawn_native/Device.h
+++ b/src/dawn_native/Device.h
@@ -251,7 +251,7 @@ namespace dawn_native {
 
         void SetDefaultToggles();
 
-        void ConsumeError(ErrorData* error);
+        void ConsumeError(std::unique_ptr<ErrorData> error);
 
         // Destroy is used to clean up and release resources used by device, does not wait for GPU
         // or check errors.
diff --git a/src/dawn_native/EncodingContext.h b/src/dawn_native/EncodingContext.h
index c16d544c0f..1eef6c5b26 100644
--- a/src/dawn_native/EncodingContext.h
+++ b/src/dawn_native/EncodingContext.h
@@ -41,9 +41,8 @@ namespace dawn_native {
         // Functions to handle encoder errors
         void HandleError(wgpu::ErrorType type, const char* message);
 
-        inline void ConsumeError(ErrorData* error) {
+        inline void ConsumeError(std::unique_ptr<ErrorData> error) {
             HandleError(error->GetType(), error->GetMessage().c_str());
-            delete error;
         }
 
         inline bool ConsumedError(MaybeError maybeError) {
diff --git a/src/dawn_native/Error.cpp b/src/dawn_native/Error.cpp
deleted file mode 100644
index 195afb2e02..0000000000
--- a/src/dawn_native/Error.cpp
+++ /dev/null
@@ -1,35 +0,0 @@
-// Copyright 2018 The Dawn Authors
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-#include "dawn_native/Error.h"
-
-#include "dawn_native/ErrorData.h"
-
-namespace dawn_native {
-
-    ErrorData* MakeError(InternalErrorType type,
-                         std::string message,
-                         const char* file,
-                         const char* function,
-                         int line) {
-        ErrorData* error = new ErrorData(type, message);
-        error->AppendBacktrace(file, function, line);
-        return error;
-    }
-
-    void AppendBacktrace(ErrorData* error, const char* file, const char* function, int line) {
-        error->AppendBacktrace(file, function, line);
-    }
-
-}  // namespace dawn_native
diff --git a/src/dawn_native/Error.h b/src/dawn_native/Error.h
index b4b968ae13..7f9dbcbc1e 100644
--- a/src/dawn_native/Error.h
+++ b/src/dawn_native/Error.h
@@ -16,23 +16,20 @@
 #define DAWNNATIVE_ERROR_H_
 
 #include "common/Result.h"
+#include "dawn_native/ErrorData.h"
 
 #include <string>
 
 namespace dawn_native {
 
-    // This is the content of an error value for MaybeError or ResultOrError, split off to its own
-    // file to avoid having all files including headers like <string> and <vector>
-    class ErrorData;
-
     enum class InternalErrorType : uint32_t { Validation, DeviceLost, Unimplemented, OutOfMemory };
 
     // MaybeError and ResultOrError are meant to be used as return value for function that are not
     // expected to, but might fail. The handling of error is potentially much slower than successes.
-    using MaybeError = Result<void, ErrorData*>;
+    using MaybeError = Result<void, ErrorData>;
 
     template <typename T>
-    using ResultOrError = Result<T, ErrorData*>;
+    using ResultOrError = Result<T, ErrorData>;
 
     // Returning a success is done like so:
     //   return {}; // for Error
@@ -44,7 +41,7 @@ namespace dawn_native {
     // but shorthand version for specific error types are preferred:
     //   return DAWN_VALIDATION_ERROR("My error message");
 #define DAWN_MAKE_ERROR(TYPE, MESSAGE) \
-    ::dawn_native::MakeError(TYPE, MESSAGE, __FILE__, __func__, __LINE__)
+    ::dawn_native::ErrorData::Create(TYPE, MESSAGE, __FILE__, __func__, __LINE__)
 #define DAWN_VALIDATION_ERROR(MESSAGE) DAWN_MAKE_ERROR(InternalErrorType::Validation, MESSAGE)
 #define DAWN_DEVICE_LOST_ERROR(MESSAGE) DAWN_MAKE_ERROR(InternalErrorType::DeviceLost, MESSAGE)
 #define DAWN_UNIMPLEMENTED_ERROR(MESSAGE) DAWN_MAKE_ERROR(InternalErrorType::Unimplemented, MESSAGE)
@@ -57,43 +54,33 @@ namespace dawn_native {
     // When Errors aren't handled explicitly, calls to functions returning errors should be
     // wrapped in an DAWN_TRY. It will return the error if any, otherwise keep executing
     // the current function.
-#define DAWN_TRY(EXPR)                                                           \
-    {                                                                            \
-        auto DAWN_LOCAL_VAR = EXPR;                                              \
-        if (DAWN_UNLIKELY(DAWN_LOCAL_VAR.IsError())) {                           \
-            ::dawn_native::ErrorData* error = DAWN_LOCAL_VAR.AcquireError();     \
-            ::dawn_native::AppendBacktrace(error, __FILE__, __func__, __LINE__); \
-            return {std::move(error)};                                           \
-        }                                                                        \
-    }                                                                            \
-    for (;;)                                                                     \
+#define DAWN_TRY(EXPR)                                                                       \
+    {                                                                                        \
+        auto DAWN_LOCAL_VAR = EXPR;                                                          \
+        if (DAWN_UNLIKELY(DAWN_LOCAL_VAR.IsError())) {                                       \
+            std::unique_ptr<::dawn_native::ErrorData> error = DAWN_LOCAL_VAR.AcquireError(); \
+            error->AppendBacktrace(__FILE__, __func__, __LINE__);                            \
+            return {std::move(error)};                                                       \
+        }                                                                                    \
+    }                                                                                        \
+    for (;;)                                                                                 \
     break
 
     // DAWN_TRY_ASSIGN is the same as DAWN_TRY for ResultOrError and assigns the success value, if
     // any, to VAR.
-#define DAWN_TRY_ASSIGN(VAR, EXPR)                                               \
-    {                                                                            \
-        auto DAWN_LOCAL_VAR = EXPR;                                              \
-        if (DAWN_UNLIKELY(DAWN_LOCAL_VAR.IsError())) {                           \
-            ErrorData* error = DAWN_LOCAL_VAR.AcquireError();                    \
-            ::dawn_native::AppendBacktrace(error, __FILE__, __func__, __LINE__); \
-            return {std::move(error)};                                           \
-        }                                                                        \
-        VAR = DAWN_LOCAL_VAR.AcquireSuccess();                                   \
-    }                                                                            \
-    for (;;)                                                                     \
+#define DAWN_TRY_ASSIGN(VAR, EXPR)                                            \
+    {                                                                         \
+        auto DAWN_LOCAL_VAR = EXPR;                                           \
+        if (DAWN_UNLIKELY(DAWN_LOCAL_VAR.IsError())) {                        \
+            std::unique_ptr<ErrorData> error = DAWN_LOCAL_VAR.AcquireError(); \
+            error->AppendBacktrace(__FILE__, __func__, __LINE__);             \
+            return {std::move(error)};                                        \
+        }                                                                     \
+        VAR = DAWN_LOCAL_VAR.AcquireSuccess();                                \
+    }                                                                         \
+    for (;;)                                                                  \
     break
 
-    // Implementation detail of DAWN_TRY and DAWN_TRY_ASSIGN's adding to the Error's backtrace.
-    void AppendBacktrace(ErrorData* error, const char* file, const char* function, int line);
-
-    // Implementation detail of DAWN_MAKE_ERROR
-    ErrorData* MakeError(InternalErrorType type,
-                         std::string message,
-                         const char* file,
-                         const char* function,
-                         int line);
-
 }  // namespace dawn_native
 
 #endif  // DAWNNATIVE_ERROR_H_
diff --git a/src/dawn_native/ErrorData.cpp b/src/dawn_native/ErrorData.cpp
index 2cd01da277..bdfed7fdaa 100644
--- a/src/dawn_native/ErrorData.cpp
+++ b/src/dawn_native/ErrorData.cpp
@@ -19,7 +19,15 @@
 
 namespace dawn_native {
 
-    ErrorData::ErrorData() = default;
+    std::unique_ptr<ErrorData> ErrorData::Create(InternalErrorType type,
+                                                 std::string message,
+                                                 const char* file,
+                                                 const char* function,
+                                                 int line) {
+        std::unique_ptr<ErrorData> error = std::make_unique<ErrorData>(type, message);
+        error->AppendBacktrace(file, function, line);
+        return error;
+    }
 
     ErrorData::ErrorData(InternalErrorType type, std::string message)
         : mType(type), mMessage(std::move(message)) {
diff --git a/src/dawn_native/ErrorData.h b/src/dawn_native/ErrorData.h
index a73d90dd23..27004de2f0 100644
--- a/src/dawn_native/ErrorData.h
+++ b/src/dawn_native/ErrorData.h
@@ -33,7 +33,11 @@ namespace dawn_native {
 
     class ErrorData {
       public:
-        ErrorData();
+        static std::unique_ptr<ErrorData> Create(InternalErrorType type,
+                                                 std::string message,
+                                                 const char* file,
+                                                 const char* function,
+                                                 int line);
         ErrorData(InternalErrorType type, std::string message);
 
         struct BacktraceRecord {
diff --git a/src/dawn_native/Instance.cpp b/src/dawn_native/Instance.cpp
index 7219ab6a7b..735c61717f 100644
--- a/src/dawn_native/Instance.cpp
+++ b/src/dawn_native/Instance.cpp
@@ -177,11 +177,10 @@ namespace dawn_native {
 
     bool InstanceBase::ConsumedError(MaybeError maybeError) {
         if (maybeError.IsError()) {
-            ErrorData* error = maybeError.AcquireError();
+            std::unique_ptr<ErrorData> error = maybeError.AcquireError();
 
             ASSERT(error != nullptr);
             dawn::InfoLog() << error->GetMessage();
-            delete error;
 
             return true;
         }
diff --git a/src/tests/unittests/ErrorTests.cpp b/src/tests/unittests/ErrorTests.cpp
index f7d5bc82ea..514c740865 100644
--- a/src/tests/unittests/ErrorTests.cpp
+++ b/src/tests/unittests/ErrorTests.cpp
@@ -43,9 +43,8 @@ TEST(ErrorTests, Error_Error) {
     MaybeError result = ReturnError();
     ASSERT_TRUE(result.IsError());
 
-    ErrorData* errorData = result.AcquireError();
+    std::unique_ptr<ErrorData> errorData = result.AcquireError();
     ASSERT_EQ(errorData->GetMessage(), dummyErrorMessage);
-    delete errorData;
 }
 
 // Check returning a success ResultOrError with an implicit conversion
@@ -68,9 +67,8 @@ TEST(ErrorTests, ResultOrError_Error) {
     ResultOrError<int*> result = ReturnError();
     ASSERT_TRUE(result.IsError());
 
-    ErrorData* errorData = result.AcquireError();
+    std::unique_ptr<ErrorData> errorData = result.AcquireError();
     ASSERT_EQ(errorData->GetMessage(), dummyErrorMessage);
-    delete errorData;
 }
 
 // Check DAWN_TRY handles successes correctly.
@@ -109,9 +107,8 @@ TEST(ErrorTests, TRY_Error) {
     MaybeError result = Try();
     ASSERT_TRUE(result.IsError());
 
-    ErrorData* errorData = result.AcquireError();
+    std::unique_ptr<ErrorData> errorData = result.AcquireError();
     ASSERT_EQ(errorData->GetMessage(), dummyErrorMessage);
-    delete errorData;
 }
 
 // Check DAWN_TRY adds to the backtrace.
@@ -136,13 +133,10 @@ TEST(ErrorTests, TRY_AddsToBacktrace) {
     MaybeError doubleResult = DoubleTry();
     ASSERT_TRUE(doubleResult.IsError());
 
-    ErrorData* singleData = singleResult.AcquireError();
-    ErrorData* doubleData = doubleResult.AcquireError();
+    std::unique_ptr<ErrorData> singleData = singleResult.AcquireError();
+    std::unique_ptr<ErrorData> doubleData = doubleResult.AcquireError();
 
     ASSERT_EQ(singleData->GetBacktrace().size() + 1, doubleData->GetBacktrace().size());
-
-    delete singleData;
-    delete doubleData;
 }
 
 // Check DAWN_TRY_ASSIGN handles successes correctly.
@@ -188,9 +182,8 @@ TEST(ErrorTests, TRY_RESULT_Error) {
     ResultOrError<int*> result = Try();
     ASSERT_TRUE(result.IsError());
 
-    ErrorData* errorData = result.AcquireError();
+    std::unique_ptr<ErrorData> errorData = result.AcquireError();
     ASSERT_EQ(errorData->GetMessage(), dummyErrorMessage);
-    delete errorData;
 }
 
 // Check DAWN_TRY_ASSIGN adds to the backtrace.
@@ -215,13 +208,10 @@ TEST(ErrorTests, TRY_RESULT_AddsToBacktrace) {
     ResultOrError<int*> doubleResult = DoubleTry();
     ASSERT_TRUE(doubleResult.IsError());
 
-    ErrorData* singleData = singleResult.AcquireError();
-    ErrorData* doubleData = doubleResult.AcquireError();
+    std::unique_ptr<ErrorData> singleData = singleResult.AcquireError();
+    std::unique_ptr<ErrorData> doubleData = doubleResult.AcquireError();
 
     ASSERT_EQ(singleData->GetBacktrace().size() + 1, doubleData->GetBacktrace().size());
-
-    delete singleData;
-    delete doubleData;
 }
 
 // Check a ResultOrError can be DAWN_TRY_ASSIGNED in a function that returns an Error
@@ -241,9 +231,8 @@ TEST(ErrorTests, TRY_RESULT_ConversionToError) {
     MaybeError result = Try();
     ASSERT_TRUE(result.IsError());
 
-    ErrorData* errorData = result.AcquireError();
+    std::unique_ptr<ErrorData> errorData = result.AcquireError();
     ASSERT_EQ(errorData->GetMessage(), dummyErrorMessage);
-    delete errorData;
 }
 
 // Check a ResultOrError can be DAWN_TRY_ASSIGNED in a function that returns an Error
@@ -264,9 +253,8 @@ TEST(ErrorTests, TRY_RESULT_ConversionToErrorNonPointer) {
     MaybeError result = Try();
     ASSERT_TRUE(result.IsError());
 
-    ErrorData* errorData = result.AcquireError();
+    std::unique_ptr<ErrorData> errorData = result.AcquireError();
     ASSERT_EQ(errorData->GetMessage(), dummyErrorMessage);
-    delete errorData;
 }
 
 // Check a MaybeError can be DAWN_TRIED in a function that returns an ResultOrError
@@ -284,9 +272,8 @@ TEST(ErrorTests, TRY_ConversionToErrorOrResult) {
     ResultOrError<int*> result = Try();
     ASSERT_TRUE(result.IsError());
 
-    ErrorData* errorData = result.AcquireError();
+    std::unique_ptr<ErrorData> errorData = result.AcquireError();
     ASSERT_EQ(errorData->GetMessage(), dummyErrorMessage);
-    delete errorData;
 }
 
 // Check a MaybeError can be DAWN_TRIED in a function that returns an ResultOrError
@@ -304,9 +291,8 @@ TEST(ErrorTests, TRY_ConversionToErrorOrResultNonPointer) {
     ResultOrError<int> result = Try();
     ASSERT_TRUE(result.IsError());
 
-    ErrorData* errorData = result.AcquireError();
+    std::unique_ptr<ErrorData> errorData = result.AcquireError();
     ASSERT_EQ(errorData->GetMessage(), dummyErrorMessage);
-    delete errorData;
 }
 
 }  // anonymous namespace
diff --git a/src/tests/unittests/ResultTests.cpp b/src/tests/unittests/ResultTests.cpp
index d991462b13..dd87e22a93 100644
--- a/src/tests/unittests/ResultTests.cpp
+++ b/src/tests/unittests/ResultTests.cpp
@@ -23,8 +23,8 @@ void TestError(Result<T, E>* result, E expectedError) {
     ASSERT_TRUE(result->IsError());
     ASSERT_FALSE(result->IsSuccess());
 
-    E storedError = result->AcquireError();
-    ASSERT_EQ(storedError, expectedError);
+    std::unique_ptr<E> storedError = result->AcquireError();
+    ASSERT_EQ(*storedError, expectedError);
 }
 
 template<typename T, typename E>
@@ -42,94 +42,88 @@ static const float dummyConstSuccess = 42.0f;
 
 // Result<void, E*>
 
-// Test constructing an error Result<void, E*>
+// Test constructing an error Result<void, E>
 TEST(ResultOnlyPointerError, ConstructingError) {
-    Result<void, int*> result(&dummyError);
-    TestError(&result, &dummyError);
+    Result<void, int> result(std::make_unique<int>(dummyError));
+    TestError(&result, dummyError);
 }
 
-// Test moving an error Result<void, E*>
+// Test moving an error Result<void, E>
 TEST(ResultOnlyPointerError, MovingError) {
-    Result<void, int*> result(&dummyError);
-    Result<void, int*> movedResult(std::move(result));
-    TestError(&movedResult, &dummyError);
+    Result<void, int> result(std::make_unique<int>(dummyError));
+    Result<void, int> movedResult(std::move(result));
+    TestError(&movedResult, dummyError);
 }
 
-// Test returning an error Result<void, E*>
+// Test returning an error Result<void, E>
 TEST(ResultOnlyPointerError, ReturningError) {
-    auto CreateError = []() -> Result<void, int*> {
-        return {&dummyError};
-    };
+    auto CreateError = []() -> Result<void, int> { return {std::make_unique<int>(dummyError)}; };
 
-    Result<void, int*> result = CreateError();
-    TestError(&result, &dummyError);
+    Result<void, int> result = CreateError();
+    TestError(&result, dummyError);
 }
 
-// Test constructing a success Result<void, E*>
+// Test constructing a success Result<void, E>
 TEST(ResultOnlyPointerError, ConstructingSuccess) {
-    Result<void, int*> result;
+    Result<void, int> result;
     ASSERT_TRUE(result.IsSuccess());
     ASSERT_FALSE(result.IsError());
 }
 
-// Test moving a success Result<void, E*>
+// Test moving a success Result<void, E>
 TEST(ResultOnlyPointerError, MovingSuccess) {
-    Result<void, int*> result;
-    Result<void, int*> movedResult(std::move(result));
+    Result<void, int> result;
+    Result<void, int> movedResult(std::move(result));
     ASSERT_TRUE(movedResult.IsSuccess());
     ASSERT_FALSE(movedResult.IsError());
 }
 
-// Test returning a success Result<void, E*>
+// Test returning a success Result<void, E>
 TEST(ResultOnlyPointerError, ReturningSuccess) {
-    auto CreateError = []() -> Result<void, int*> {
-        return {};
-    };
+    auto CreateError = []() -> Result<void, int> { return {}; };
 
-    Result<void, int*> result = CreateError();
+    Result<void, int> result = CreateError();
     ASSERT_TRUE(result.IsSuccess());
     ASSERT_FALSE(result.IsError());
 }
 
 // Result<T*, E*>
 
-// Test constructing an error Result<T*, E*>
+// Test constructing an error Result<T*, E>
 TEST(ResultBothPointer, ConstructingError) {
-    Result<float*, int*> result(&dummyError);
-    TestError(&result, &dummyError);
+    Result<float*, int> result(std::make_unique<int>(dummyError));
+    TestError(&result, dummyError);
 }
 
-// Test moving an error Result<T*, E*>
+// Test moving an error Result<T*, E>
 TEST(ResultBothPointer, MovingError) {
-    Result<float*, int*> result(&dummyError);
-    Result<float*, int*> movedResult(std::move(result));
-    TestError(&movedResult, &dummyError);
+    Result<float*, int> result(std::make_unique<int>(dummyError));
+    Result<float*, int> movedResult(std::move(result));
+    TestError(&movedResult, dummyError);
 }
 
-// Test returning an error Result<T*, E*>
+// Test returning an error Result<T*, E>
 TEST(ResultBothPointer, ReturningError) {
-    auto CreateError = []() -> Result<float*, int*> {
-        return {&dummyError};
-    };
+    auto CreateError = []() -> Result<float*, int> { return {std::make_unique<int>(dummyError)}; };
 
-    Result<float*, int*> result = CreateError();
-    TestError(&result, &dummyError);
+    Result<float*, int> result = CreateError();
+    TestError(&result, dummyError);
 }
 
-// Test constructing a success Result<T*, E*>
+// Test constructing a success Result<T*, E>
 TEST(ResultBothPointer, ConstructingSuccess) {
-    Result<float*, int*> result(&dummySuccess);
+    Result<float*, int> result(&dummySuccess);
     TestSuccess(&result, &dummySuccess);
 }
 
-// Test moving a success Result<T*, E*>
+// Test moving a success Result<T*, E>
 TEST(ResultBothPointer, MovingSuccess) {
-    Result<float*, int*> result(&dummySuccess);
-    Result<float*, int*> movedResult(std::move(result));
+    Result<float*, int> result(&dummySuccess);
+    Result<float*, int> movedResult(std::move(result));
     TestSuccess(&movedResult, &dummySuccess);
 }
 
-// Test returning a success Result<T*, E*>
+// Test returning a success Result<T*, E>
 TEST(ResultBothPointer, ReturningSuccess) {
     auto CreateSuccess = []() -> Result<float*, int*> {
         return {&dummySuccess};
@@ -139,7 +133,7 @@ TEST(ResultBothPointer, ReturningSuccess) {
     TestSuccess(&result, &dummySuccess);
 }
 
-// Tests converting from a Result<TChild*, E*>
+// Tests converting from a Result<TChild*, E>
 TEST(ResultBothPointer, ConversionFromChildClass) {
     struct T {
         int a;
@@ -149,62 +143,64 @@ TEST(ResultBothPointer, ConversionFromChildClass) {
     TChild child;
     T* childAsT = &child;
     {
-        Result<T*, int*> result(&child);
+        Result<T*, int> result(&child);
         TestSuccess(&result, childAsT);
     }
     {
-        Result<TChild*, int*> resultChild(&child);
-        Result<T*, int*> result(std::move(resultChild));
+        Result<TChild*, int> resultChild(&child);
+        Result<T*, int> result(std::move(resultChild));
         TestSuccess(&result, childAsT);
     }
     {
-        Result<TChild*, int*> resultChild(&child);
-        Result<T*, int*> result = std::move(resultChild);
+        Result<TChild*, int> resultChild(&child);
+        Result<T*, int> result = std::move(resultChild);
         TestSuccess(&result, childAsT);
     }
 }
 
-// Result<const T*, E*>
+// Result<const T*, E>
 
-// Test constructing an error Result<const T*, E*>
+// Test constructing an error Result<const T*, E>
 TEST(ResultBothPointerWithConstResult, ConstructingError) {
-    Result<const float*, int*> result(&dummyError);
-    TestError(&result, &dummyError);
+    Result<const float*, int> result(std::make_unique<int>(dummyError));
+    TestError(&result, dummyError);
 }
 
-// Test moving an error Result<const T*, E*>
+// Test moving an error Result<const T*, E>
 TEST(ResultBothPointerWithConstResult, MovingError) {
-    Result<const float*, int*> result(&dummyError);
-    Result<const float*, int*> movedResult(std::move(result));
-    TestError(&movedResult, &dummyError);
+    Result<const float*, int> result(std::make_unique<int>(dummyError));
+    Result<const float*, int> movedResult(std::move(result));
+    TestError(&movedResult, dummyError);
 }
 
 // Test returning an error Result<const T*, E*>
 TEST(ResultBothPointerWithConstResult, ReturningError) {
-    auto CreateError = []() -> Result<const float*, int*> { return {&dummyError}; };
+    auto CreateError = []() -> Result<const float*, int> {
+        return {std::make_unique<int>(dummyError)};
+    };
 
-    Result<const float*, int*> result = CreateError();
-    TestError(&result, &dummyError);
+    Result<const float*, int> result = CreateError();
+    TestError(&result, dummyError);
 }
 
 // Test constructing a success Result<const T*, E*>
 TEST(ResultBothPointerWithConstResult, ConstructingSuccess) {
-    Result<const float*, int*> result(&dummyConstSuccess);
+    Result<const float*, int> result(&dummyConstSuccess);
     TestSuccess(&result, &dummyConstSuccess);
 }
 
 // Test moving a success Result<const T*, E*>
 TEST(ResultBothPointerWithConstResult, MovingSuccess) {
-    Result<const float*, int*> result(&dummyConstSuccess);
-    Result<const float*, int*> movedResult(std::move(result));
+    Result<const float*, int> result(&dummyConstSuccess);
+    Result<const float*, int> movedResult(std::move(result));
     TestSuccess(&movedResult, &dummyConstSuccess);
 }
 
 // Test returning a success Result<const T*, E*>
 TEST(ResultBothPointerWithConstResult, ReturningSuccess) {
-    auto CreateSuccess = []() -> Result<const float*, int*> { return {&dummyConstSuccess}; };
+    auto CreateSuccess = []() -> Result<const float*, int> { return {&dummyConstSuccess}; };
 
-    Result<const float*, int*> result = CreateSuccess();
+    Result<const float*, int> result = CreateSuccess();
     TestSuccess(&result, &dummyConstSuccess);
 }
 
@@ -212,47 +208,45 @@ TEST(ResultBothPointerWithConstResult, ReturningSuccess) {
 
 // Test constructing an error Result<T, E>
 TEST(ResultGeneric, ConstructingError) {
-    Result<std::vector<float>, int*> result(&dummyError);
-    TestError(&result, &dummyError);
+    Result<std::vector<float>, int> result(std::make_unique<int>(dummyError));
+    TestError(&result, dummyError);
 }
 
 // Test moving an error Result<T, E>
 TEST(ResultGeneric, MovingError) {
-    Result<std::vector<float>, int*> result(&dummyError);
-    Result<std::vector<float>, int*> movedResult(std::move(result));
-    TestError(&movedResult, &dummyError);
+    Result<std::vector<float>, int> result(std::make_unique<int>(dummyError));
+    Result<std::vector<float>, int> movedResult(std::move(result));
+    TestError(&movedResult, dummyError);
 }
 
 // Test returning an error Result<T, E>
 TEST(ResultGeneric, ReturningError) {
-    auto CreateError = []() -> Result<std::vector<float>, int*> {
-        return {&dummyError};
+    auto CreateError = []() -> Result<std::vector<float>, int> {
+        return {std::make_unique<int>(dummyError)};
     };
 
-    Result<std::vector<float>, int*> result = CreateError();
-    TestError(&result, &dummyError);
+    Result<std::vector<float>, int> result = CreateError();
+    TestError(&result, dummyError);
 }
 
 // Test constructing a success Result<T, E>
 TEST(ResultGeneric, ConstructingSuccess) {
-    Result<std::vector<float>, int*> result({1.0f});
+    Result<std::vector<float>, int> result({1.0f});
     TestSuccess(&result, {1.0f});
 }
 
 // Test moving a success Result<T, E>
 TEST(ResultGeneric, MovingSuccess) {
-    Result<std::vector<float>, int*> result({1.0f});
-    Result<std::vector<float>, int*> movedResult(std::move(result));
+    Result<std::vector<float>, int> result({1.0f});
+    Result<std::vector<float>, int> movedResult(std::move(result));
     TestSuccess(&movedResult, {1.0f});
 }
 
 // Test returning a success Result<T, E>
 TEST(ResultGeneric, ReturningSuccess) {
-    auto CreateSuccess = []() -> Result<std::vector<float>, int*> {
-        return {{1.0f}};
-    };
+    auto CreateSuccess = []() -> Result<std::vector<float>, int> { return {{1.0f}}; };
 
-    Result<std::vector<float>, int*> result = CreateSuccess();
+    Result<std::vector<float>, int> result = CreateSuccess();
     TestSuccess(&result, {1.0f});
 }
 
diff --git a/src/tests/white_box/VulkanErrorInjectorTests.cpp b/src/tests/white_box/VulkanErrorInjectorTests.cpp
index 938bd5dd72..bd4a49e1ae 100644
--- a/src/tests/white_box/VulkanErrorInjectorTests.cpp
+++ b/src/tests/white_box/VulkanErrorInjectorTests.cpp
@@ -61,7 +61,7 @@ TEST_P(VulkanErrorInjectorTests, InjectErrorOnCreateBuffer) {
         if (err.IsError()) {
             // The handle should never be written to, even for mock failures.
             EXPECT_EQ(buffer, VK_NULL_HANDLE);
-            delete err.AcquireError();
+            err.AcquireError();
             return false;
         }
         EXPECT_NE(buffer, VK_NULL_HANDLE);