Fixes Vulkan command buffer leaks when an error occurs.

- Uses an anonymous function to delete command pool/buffers.
- Shuffles the code around a bit so that the CommandPoolAndBuffer are
  clearly next to the EncodingContext stuff to make it clear that we
  may be able to consolidate them in the future.

Bug: chromium:1372772
Change-Id: I92a1d0333b7a85d439b5963a58db69ac685c03a4
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/112181
Reviewed-by: Austin Eng <enga@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Loko Kung <lokokung@google.com>

src/dawn/native/vulkan/CommandRecordingContext.h

@@ -19,11 +19,21 @@
 
 #include "dawn/common/vulkan_platform.h"
 #include "dawn/native/vulkan/BufferVk.h"
+#include "dawn/native/vulkan/VulkanFunctions.h"
 
 namespace dawn::native::vulkan {
 
 class Texture;
 
+// Wrapping class that currently associates a command buffer to it's corresponding pool.
+// TODO(dawn:1601) Revisit this structure since it is where the 1:1 mapping is implied.
+//                 Also consider reusing this in CommandRecordingContext below instead of
+//                 flattening the pool and command buffer again.
+struct CommandPoolAndBuffer {
+    VkCommandPool pool = VK_NULL_HANDLE;
+    VkCommandBuffer commandBuffer = VK_NULL_HANDLE;
+};
+
 // Used to track operations that are handled after recording.
 // Currently only tracks semaphores, but may be used to do barrier coalescing in the future.
 struct CommandRecordingContext {

src/dawn/native/vulkan/DeviceVk.cpp

@@ -69,6 +69,23 @@ class ScopedSignalSemaphore : public NonMovable {
     VkSemaphore mSemaphore = VK_NULL_HANDLE;
 };
 
+// Destroys command pool/buffer.
+// TODO(dawn:1601) Revisit this and potentially bake into pool/buffer objects instead.
+void DestroyCommandPoolAndBuffer(const VulkanFunctions& fn,
+                                 VkDevice device,
+                                 const CommandPoolAndBuffer& commands) {
+    // The VkCommandBuffer memory should be wholly owned by the pool and freed when it is
+    // destroyed, but that's not the case in some drivers and they leak memory. So we call
+    // FreeCommandBuffers before DestroyCommandPool to be safe.
+    // TODO(enga): Only do this on a known list of bad drivers.
+    if (commands.pool != VK_NULL_HANDLE) {
+        if (commands.commandBuffer != VK_NULL_HANDLE) {
+            fn.FreeCommandBuffers(device, commands.pool, 1, &commands.commandBuffer);
+        }
+        fn.DestroyCommandPool(device, commands.pool, nullptr);
+    }
+}
+
 }  // namespace
 
 // static
@@ -745,7 +762,7 @@ MaybeError Device::SplitRecordingContext(CommandRecordingContext* recordingConte
     return {};
 }
 
-ResultOrError<Device::CommandPoolAndBuffer> Device::BeginVkCommandBuffer() {
+ResultOrError<CommandPoolAndBuffer> Device::BeginVkCommandBuffer() {
     CommandPoolAndBuffer commands;
 
     // First try to recycle unused command pools.
@@ -754,19 +771,7 @@ ResultOrError<Device::CommandPoolAndBuffer> Device::BeginVkCommandBuffer() {
         mUnusedCommands.pop_back();
         DAWN_TRY_WITH_CLEANUP(
             CheckVkSuccess(fn.ResetCommandPool(mVkDevice, commands.pool, 0), "vkResetCommandPool"),
-            {
+            { DestroyCommandPoolAndBuffer(fn, mVkDevice, commands); });
-                // vkResetCommandPool failed (it may return out-of-memory).
-                // Free the commands in the cleanup step before returning to
-                // reclaim memory.
-
-                // The VkCommandBuffer memory should be wholly owned by the
-                // pool and freed when it is destroyed, but that's not the
-                // case in some drivers and they leak memory. So we call
-                // FreeCommandBuffers before DestroyCommandPool to be safe.
-                // TODO(enga): Only do this on a known list of bad drivers.
-                fn.FreeCommandBuffers(mVkDevice, commands.pool, 1, &commands.commandBuffer);
-                fn.DestroyCommandPool(mVkDevice, commands.pool, nullptr);
-            });
     } else {
         // Create a new command pool for our commands and allocate the command buffer.
         VkCommandPoolCreateInfo createInfo;
@@ -786,9 +791,10 @@ ResultOrError<Device::CommandPoolAndBuffer> Device::BeginVkCommandBuffer() {
         allocateInfo.level = VK_COMMAND_BUFFER_LEVEL_PRIMARY;
         allocateInfo.commandBufferCount = 1;
 
-        DAWN_TRY(CheckVkSuccess(
+        DAWN_TRY_WITH_CLEANUP(CheckVkSuccess(fn.AllocateCommandBuffers(mVkDevice, &allocateInfo,
-            fn.AllocateCommandBuffers(mVkDevice, &allocateInfo, &commands.commandBuffer),
+                                                                       &commands.commandBuffer),
-            "vkAllocateCommandBuffers"));
+                                             "vkAllocateCommandBuffers"),
+                              { DestroyCommandPoolAndBuffer(fn, mVkDevice, commands); });
     }
 
     // Start the recording of commands in the command buffer.
@@ -798,8 +804,9 @@ ResultOrError<Device::CommandPoolAndBuffer> Device::BeginVkCommandBuffer() {
     beginInfo.flags = VK_COMMAND_BUFFER_USAGE_ONE_TIME_SUBMIT_BIT;
     beginInfo.pInheritanceInfo = nullptr;
 
-    DAWN_TRY(CheckVkSuccess(fn.BeginCommandBuffer(commands.commandBuffer, &beginInfo),
+    DAWN_TRY_WITH_CLEANUP(CheckVkSuccess(fn.BeginCommandBuffer(commands.commandBuffer, &beginInfo),
-                            "vkBeginCommandBuffer"));
+                                         "vkBeginCommandBuffer"),
+                          { DestroyCommandPoolAndBuffer(fn, mVkDevice, commands); });
 
     return commands;
 }
@@ -1127,13 +1134,8 @@ void Device::DestroyImpl() {
     // Immediately tag the recording context as unused so we don't try to submit it in Tick.
     mRecordingContext.needsSubmit = false;
     if (mRecordingContext.commandPool != VK_NULL_HANDLE) {
-        // The VkCommandBuffer memory should be wholly owned by the pool and freed when it is
+        DestroyCommandPoolAndBuffer(
-        // destroyed, but that's not the case in some drivers and the leak memory.
+            fn, mVkDevice, {mRecordingContext.commandPool, mRecordingContext.commandBuffer});
-        // So we call FreeCommandBuffers before DestroyCommandPool to be safe.
-        // TODO(enga): Only do this on a known list of bad drivers.
-        fn.FreeCommandBuffers(mVkDevice, mRecordingContext.commandPool, 1,
-                              &mRecordingContext.commandBuffer);
-        fn.DestroyCommandPool(mVkDevice, mRecordingContext.commandPool, nullptr);
     }
 
     for (VkSemaphore semaphore : mRecordingContext.waitSemaphores) {
@@ -1147,12 +1149,7 @@ void Device::DestroyImpl() {
     ASSERT(mCommandsInFlight.Empty());
 
     for (const CommandPoolAndBuffer& commands : mUnusedCommands) {
-        // The VkCommandBuffer memory should be wholly owned by the pool and freed when it is
+        DestroyCommandPoolAndBuffer(fn, mVkDevice, commands);
-        // destroyed, but that's not the case in some drivers and the leak memory.
-        // So we call FreeCommandBuffers before DestroyCommandPool to be safe.
-        // TODO(enga): Only do this on a known list of bad drivers.
-        fn.FreeCommandBuffers(mVkDevice, commands.pool, 1, &commands.commandBuffer);
-        fn.DestroyCommandPool(mVkDevice, commands.pool, nullptr);
     }
     mUnusedCommands.clear();
 

src/dawn/native/vulkan/DeviceVk.h

@@ -210,11 +210,6 @@ class Device final : public DeviceBase {
     const std::string mDebugPrefix;
     std::vector<std::string> mDebugMessages;
 
-    struct CommandPoolAndBuffer {
-        VkCommandPool pool = VK_NULL_HANDLE;
-        VkCommandBuffer commandBuffer = VK_NULL_HANDLE;
-    };
-
     MaybeError PrepareRecordingContext();
     ResultOrError<CommandPoolAndBuffer> BeginVkCommandBuffer();
     void RecycleCompletedCommands();