Make ConsumedError require handling the returned bool.

This change makes it harder to misuse ConsumedError which can cause
device loss. When it is a known error, instead use HandleError to
bypass the "unlikely" if clause.

Bug: dawn:1336
Change-Id: I3052db343fe4080b257f1c2f9535f743a0e78526
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/120384
Reviewed-by: Austin Eng <enga@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Loko Kung <lokokung@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>

src/dawn/native/Buffer.cpp

@@ -506,9 +506,7 @@ void BufferBase::APIUnmap() {
     if (GetDevice()->ConsumedError(ValidateUnmap(), "calling %s.Unmap().", this)) {
         return;
     }
-    if (GetDevice()->ConsumedError(Unmap(), "calling %s.Unmap().", this)) {
+    DAWN_UNUSED(GetDevice()->ConsumedError(Unmap(), "calling %s.Unmap().", this));
-        return;
-    }
 }
 
 MaybeError BufferBase::Unmap() {

src/dawn/native/ComputePassEncoder.cpp

@@ -151,7 +151,7 @@ ObjectType ComputePassEncoder::GetType() const {
 
 void ComputePassEncoder::APIEnd() {
     if (mEnded && IsValidationEnabled()) {
-        GetDevice()->ConsumedError(DAWN_VALIDATION_ERROR("%s was already ended.", this));
+        GetDevice()->HandleError(DAWN_VALIDATION_ERROR("%s was already ended.", this));
         return;
     }
 

src/dawn/native/Device.cpp

@@ -469,6 +469,7 @@ void DeviceBase::APIDestroy() {
 void DeviceBase::HandleError(std::unique_ptr<ErrorData> error,
                              InternalErrorType additionalAllowedErrors,
                              WGPUDeviceLostReason lost_reason) {
+    AppendDebugLayerMessages(error.get());
     InternalErrorType allowedErrors =
         InternalErrorType::Validation | InternalErrorType::DeviceLost | additionalAllowedErrors;
     InternalErrorType type = error->GetType();
@@ -545,7 +546,6 @@ void DeviceBase::HandleError(std::unique_ptr<ErrorData> error,
 void DeviceBase::ConsumeError(std::unique_ptr<ErrorData> error,
                               InternalErrorType additionalAllowedErrors) {
     ASSERT(error != nullptr);
-    AppendDebugLayerMessages(error.get());
     HandleError(std::move(error), additionalAllowedErrors);
 }
 
@@ -1203,21 +1203,24 @@ BufferBase* DeviceBase::APICreateErrorBuffer(const BufferDescriptor* desc) {
     // The validation errors on BufferDescriptor should be prior to any OOM errors when
     // MapppedAtCreation == false.
     MaybeError maybeError = ValidateBufferDescriptor(this, &fakeDescriptor);
-    if (maybeError.IsError()) {
-        ConsumedError(maybeError.AcquireError(), InternalErrorType::OutOfMemory,
-                      "calling %s.CreateBuffer(%s).", this, desc);
-    } else {
-        const DawnBufferDescriptorErrorInfoFromWireClient* clientErrorInfo = nullptr;
-        FindInChain(desc->nextInChain, &clientErrorInfo);
-        if (clientErrorInfo != nullptr && clientErrorInfo->outOfMemory) {
-            ConsumedError(DAWN_OUT_OF_MEMORY_ERROR("Failed to allocate memory for buffer mapping"),
-                          InternalErrorType::OutOfMemory);
-        }
-    }
 
     // Set the size of the error buffer to 0 as this function is called only when an OOM happens at
     // the client side.
     fakeDescriptor.size = 0;
+
+    if (maybeError.IsError()) {
+        std::unique_ptr<ErrorData> error = maybeError.AcquireError();
+        error->AppendContext("calling %s.CreateBuffer(%s).", this, desc);
+        HandleError(std::move(error), InternalErrorType::OutOfMemory);
+    } else {
+        const DawnBufferDescriptorErrorInfoFromWireClient* clientErrorInfo = nullptr;
+        FindInChain(desc->nextInChain, &clientErrorInfo);
+        if (clientErrorInfo != nullptr && clientErrorInfo->outOfMemory) {
+            HandleError(DAWN_OUT_OF_MEMORY_ERROR("Failed to allocate memory for buffer mapping"),
+                        InternalErrorType::OutOfMemory);
+        }
+    }
+
     return BufferBase::MakeError(this, &fakeDescriptor);
 }
 
@@ -1401,7 +1404,7 @@ void DeviceBase::APIInjectError(wgpu::ErrorType type, const char* message) {
 }
 
 void DeviceBase::APIValidateTextureDescriptor(const TextureDescriptor* desc) {
-    ConsumedError(ValidateTextureDescriptor(this, desc));
+    DAWN_UNUSED(ConsumedError(ValidateTextureDescriptor(this, desc)));
 }
 
 QueueBase* DeviceBase::GetQueue() const {

src/dawn/native/Device.h

@@ -76,7 +76,11 @@ class DeviceBase : public RefCountedWithExternalCount {
                      InternalErrorType additionalAllowedErrors = InternalErrorType::None,
                      WGPUDeviceLostReason lost_reason = WGPUDeviceLostReason_Undefined);
 
-    bool ConsumedError(MaybeError maybeError,
+    // Variants of ConsumedError must use the returned boolean to handle failure cases since an
+    // error may cause a device loss and further execution may be undefined. This is especially
+    // true for the ResultOrError variants.
+    [[nodiscard]] bool ConsumedError(
+        MaybeError maybeError,
         InternalErrorType additionalAllowedErrors = InternalErrorType::None) {
         if (DAWN_UNLIKELY(maybeError.IsError())) {
             ConsumeError(maybeError.AcquireError(), additionalAllowedErrors);
@@ -86,7 +90,8 @@ class DeviceBase : public RefCountedWithExternalCount {
     }
 
     template <typename T>
-    bool ConsumedError(ResultOrError<T> resultOrError,
+    [[nodiscard]] bool ConsumedError(
+        ResultOrError<T> resultOrError,
         T* result,
         InternalErrorType additionalAllowedErrors = InternalErrorType::None) {
         if (DAWN_UNLIKELY(resultOrError.IsError())) {
@@ -98,7 +103,7 @@ class DeviceBase : public RefCountedWithExternalCount {
     }
 
     template <typename... Args>
-    bool ConsumedError(MaybeError maybeError,
+    [[nodiscard]] bool ConsumedError(MaybeError maybeError,
                                      InternalErrorType additionalAllowedErrors,
                                      const char* formatStr,
                                      const Args&... args) {
@@ -114,13 +119,14 @@ class DeviceBase : public RefCountedWithExternalCount {
     }
 
     template <typename... Args>
-    bool ConsumedError(MaybeError maybeError, const char* formatStr, Args&&... args) {
+    [[nodiscard]] bool ConsumedError(MaybeError maybeError,
-        return ConsumedError(std::move(maybeError), InternalErrorType::None, formatStr,
+                                     const char* formatStr,
-                             std::forward<Args>(args)...);
+                                     const Args&... args) {
+        return ConsumedError(std::move(maybeError), InternalErrorType::None, formatStr, args...);
     }
 
     template <typename T, typename... Args>
-    bool ConsumedError(ResultOrError<T> resultOrError,
+    [[nodiscard]] bool ConsumedError(ResultOrError<T> resultOrError,
                                      T* result,
                                      InternalErrorType additionalAllowedErrors,
                                      const char* formatStr,
@@ -138,12 +144,12 @@ class DeviceBase : public RefCountedWithExternalCount {
     }
 
     template <typename T, typename... Args>
-    bool ConsumedError(ResultOrError<T> resultOrError,
+    [[nodiscard]] bool ConsumedError(ResultOrError<T> resultOrError,
                                      T* result,
                                      const char* formatStr,
-                       Args&&... args) {
+                                     const Args&... args) {
         return ConsumedError(std::move(resultOrError), result, InternalErrorType::None, formatStr,
-                             std::forward<Args>(args)...);
+                             args...);
     }
 
     MaybeError ValidateObject(const ApiObjectBase* object) const;

src/dawn/native/Queue.cpp

@@ -281,7 +281,7 @@ void QueueBase::APIWriteBuffer(BufferBase* buffer,
                                uint64_t bufferOffset,
                                const void* data,
                                size_t size) {
-    GetDevice()->ConsumedError(WriteBuffer(buffer, bufferOffset, data, size));
+    DAWN_UNUSED(GetDevice()->ConsumedError(WriteBuffer(buffer, bufferOffset, data, size)));
 }
 
 MaybeError QueueBase::WriteBuffer(BufferBase* buffer,
@@ -322,8 +322,8 @@ void QueueBase::APIWriteTexture(const ImageCopyTexture* destination,
                                 size_t dataSize,
                                 const TextureDataLayout* dataLayout,
                                 const Extent3D* writeSize) {
-    GetDevice()->ConsumedError(
+    DAWN_UNUSED(GetDevice()->ConsumedError(
-        WriteTextureInternal(destination, data, dataSize, *dataLayout, writeSize));
+        WriteTextureInternal(destination, data, dataSize, *dataLayout, writeSize)));
 }
 
 MaybeError QueueBase::WriteTextureInternal(const ImageCopyTexture* destination,
@@ -389,16 +389,16 @@ void QueueBase::APICopyTextureForBrowser(const ImageCopyTexture* source,
                                          const ImageCopyTexture* destination,
                                          const Extent3D* copySize,
                                          const CopyTextureForBrowserOptions* options) {
-    GetDevice()->ConsumedError(
+    DAWN_UNUSED(GetDevice()->ConsumedError(
-        CopyTextureForBrowserInternal(source, destination, copySize, options));
+        CopyTextureForBrowserInternal(source, destination, copySize, options)));
 }
 
 void QueueBase::APICopyExternalTextureForBrowser(const ImageCopyExternalTexture* source,
                                                  const ImageCopyTexture* destination,
                                                  const Extent3D* copySize,
                                                  const CopyTextureForBrowserOptions* options) {
-    GetDevice()->ConsumedError(
+    DAWN_UNUSED(GetDevice()->ConsumedError(
-        CopyExternalTextureForBrowserInternal(source, destination, copySize, options));
+        CopyExternalTextureForBrowserInternal(source, destination, copySize, options)));
 }
 
 MaybeError QueueBase::CopyTextureForBrowserInternal(const ImageCopyTexture* source,

src/dawn/native/RenderPassEncoder.cpp

@@ -137,7 +137,7 @@ void RenderPassEncoder::TrackQueryAvailability(QuerySetBase* querySet, uint32_t
 
 void RenderPassEncoder::APIEnd() {
     if (mEnded && IsValidationEnabled()) {
-        GetDevice()->ConsumedError(DAWN_VALIDATION_ERROR("%s was already ended.", this));
+        GetDevice()->HandleError(DAWN_VALIDATION_ERROR("%s was already ended.", this));
         return;
     }
 

src/dawn/native/SwapChain.cpp

@@ -35,16 +35,16 @@ class ErrorSwapChain final : public SwapChainBase {
                       wgpu::TextureUsage allowedUsage,
                       uint32_t width,
                       uint32_t height) override {
-        GetDevice()->ConsumedError(DAWN_VALIDATION_ERROR("%s is an error swapchain.", this));
+        GetDevice()->HandleError(DAWN_VALIDATION_ERROR("%s is an error swapchain.", this));
     }
 
     TextureViewBase* APIGetCurrentTextureView() override {
-        GetDevice()->ConsumedError(DAWN_VALIDATION_ERROR("%s is an error swapchain.", this));
+        GetDevice()->HandleError(DAWN_VALIDATION_ERROR("%s is an error swapchain.", this));
         return TextureViewBase::MakeError(GetDevice());
     }
 
     void APIPresent() override {
-        GetDevice()->ConsumedError(DAWN_VALIDATION_ERROR("%s is an error swapchain.", this));
+        GetDevice()->HandleError(DAWN_VALIDATION_ERROR("%s is an error swapchain.", this));
     }
 };
 
@@ -300,7 +300,7 @@ void NewSwapChainBase::APIConfigure(wgpu::TextureFormat format,
                                     wgpu::TextureUsage allowedUsage,
                                     uint32_t width,
                                     uint32_t height) {
-    GetDevice()->ConsumedError(
+    GetDevice()->HandleError(
         DAWN_VALIDATION_ERROR("Configure is invalid for surface-based swapchains."));
 }
 

src/dawn/tests/unittests/validation/DeviceValidationTests.cpp

@@ -226,6 +226,6 @@ TEST_F(DeviceTickValidationTest, DestroyDeviceBeforeInternalTick) {
     ExpectDeviceDestruction();
     device.Destroy();
     dawn::native::DeviceBase* nativeDevice = dawn::native::FromAPI(device.Get());
-    ASSERT_DEVICE_ERROR(nativeDevice->ConsumedError(nativeDevice->Tick()),
+    ASSERT_DEVICE_ERROR(EXPECT_TRUE(nativeDevice->ConsumedError(nativeDevice->Tick())),
                         HasSubstr("[Device] is lost."));
 }