Vulkan: Fix image leak in staged updates in ImageHelper

When removing superseding updates, the superseded update was not
released, causing a memory leak.  This change also makes
SubresourceUpdate non-copyable and correctly implements the move
assignment operator such that swap between different update types are
correct.  As a result, the destructor can now ASSERT that the image is
not leaked.

Bug: chromium:1146516
Bug: chromium:1163354
Change-Id: I7531c91d8559c23b2e09159118fe645d12fc601f
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/2613201
Reviewed-by: Jamie Madill <jmadill@chromium.org>
Reviewed-by: Tim Van Patten <timvp@google.com>
Commit-Queue: Shahbaz Youssefi <syoussefi@chromium.org>

src/libANGLE/renderer/vulkan/vk_helpers.cpp

@@ -5551,7 +5551,7 @@ angle::Result ImageHelper::flushStagedUpdates(ContextVk *contextVk,
         return angle::Result::Continue;
     }
 
-    removeSupersededUpdates(skipLevelsMask);
+    removeSupersededUpdates(contextVk, skipLevelsMask);
 
     // If a clear is requested and we know it was previously cleared with the same value, we drop
     // the clear.
@@ -5639,7 +5639,7 @@ angle::Result ImageHelper::flushStagedUpdates(ContextVk *contextVk,
             if (isUpdateLevelOutsideRange || areUpdateLayersOutsideRange ||
                 skipLevelsMask.test(updateMipLevelVk.get()))
             {
-                updatesToKeep.emplace_back(update);
+                updatesToKeep.emplace_back(std::move(update));
                 continue;
             }
 
@@ -5813,7 +5813,7 @@ bool ImageHelper::hasStagedUpdatesInLevels(gl::LevelIndex levelStart, gl::LevelI
     return false;
 }
 
-void ImageHelper::removeSupersededUpdates(gl::TexLevelMask skipLevelsMask)
+void ImageHelper::removeSupersededUpdates(ContextVk *contextVk, gl::TexLevelMask skipLevelsMask)
 {
     if (mLayerCount > 64)
     {
@@ -5822,6 +5822,8 @@ void ImageHelper::removeSupersededUpdates(gl::TexLevelMask skipLevelsMask)
         return;
     }
 
+    RendererVk *renderer = contextVk->getRenderer();
+
     // Go over updates in reverse order, and mark the layers they completely overwrite.  If an
     // update is encountered whose layers are all already marked, that update is superseded by
     // future updates, so it can be dropped.  This tracking is done per level.  If the aspect being
@@ -5837,7 +5839,7 @@ void ImageHelper::removeSupersededUpdates(gl::TexLevelMask skipLevelsMask)
     // Note: this lambda only needs |this|, but = is specified because clang warns about kIndex* not
     // needing capture, while MSVC fails to compile without capturing them.
     auto markLayersAndDropSuperseded = [=, &supersededLayers,
-                                        &levelExtents](const SubresourceUpdate &update) {
+                                        &levelExtents](SubresourceUpdate &update) {
         uint32_t updateBaseLayer, updateLayerCount;
         update.getDestSubresource(mLayerCount, &updateBaseLayer, &updateLayerCount);
 
@@ -5861,6 +5863,7 @@ void ImageHelper::removeSupersededUpdates(gl::TexLevelMask skipLevelsMask)
 
         if (isColorOrDepthSuperseded && isStencilSuperseded)
         {
+            update.release(renderer);
             return true;
         }
 
@@ -6272,6 +6275,11 @@ angle::Result ImageHelper::readPixels(ContextVk *contextVk,
 ImageHelper::SubresourceUpdate::SubresourceUpdate() : updateSource(UpdateSource::Buffer), buffer{}
 {}
 
+ImageHelper::SubresourceUpdate::~SubresourceUpdate()
+{
+    ASSERT(updateSource != UpdateSource::Image || image.image == nullptr);
+}
+
 ImageHelper::SubresourceUpdate::SubresourceUpdate(BufferHelper *bufferHelperIn,
                                                   const VkBufferImageCopy &copyRegionIn)
     : updateSource(UpdateSource::Buffer), buffer{bufferHelperIn, copyRegionIn}
@@ -6295,39 +6303,45 @@ ImageHelper::SubresourceUpdate::SubresourceUpdate(VkImageAspectFlags aspectFlags
         imageIndex.hasLayer() ? imageIndex.getLayerCount() : VK_REMAINING_ARRAY_LAYERS;
 }
 
-ImageHelper::SubresourceUpdate::SubresourceUpdate(const SubresourceUpdate &other)
+ImageHelper::SubresourceUpdate::SubresourceUpdate(SubresourceUpdate &&other)
     : updateSource(other.updateSource)
 {
-    if (updateSource == UpdateSource::Clear)
+    switch (updateSource)
     {
-        clear = other.clear;
-    }
-    else if (updateSource == UpdateSource::Buffer)
-    {
-        buffer = other.buffer;
-    }
-    else
-    {
-        image = other.image;
+        case UpdateSource::Clear:
+            clear = other.clear;
+            break;
+        case UpdateSource::Buffer:
+            buffer = other.buffer;
+            break;
+        case UpdateSource::Image:
+            image             = other.image;
+            other.image.image = nullptr;
+            break;
+        default:
+            UNREACHABLE();
     }
 }
 
-ImageHelper::SubresourceUpdate &ImageHelper::SubresourceUpdate::operator=(
-    const SubresourceUpdate &other)
+ImageHelper::SubresourceUpdate &ImageHelper::SubresourceUpdate::operator=(SubresourceUpdate &&other)
 {
-    updateSource = other.updateSource;
-    if (updateSource == UpdateSource::Clear)
-    {
-        clear = other.clear;
-    }
-    else if (updateSource == UpdateSource::Buffer)
-    {
-        buffer = other.buffer;
-    }
-    else
-    {
-        image = other.image;
-    }
+    // Given that the update is a union of three structs, we can't use std::swap on the fields.  For
+    // example, |this| may be an Image update and |other| may be a Buffer update.
+    // The following could work:
+    //
+    // SubresourceUpdate oldThis;
+    // Set oldThis to this->field based on updateSource
+    // Set this->otherField to other.otherField based on other.updateSource
+    // Set other.field to oldThis->field based on updateSource
+    // std::Swap(updateSource, other.updateSource);
+    //
+    // It's much simpler to just swap the memory instead.
+
+    SubresourceUpdate oldThis;
+    memcpy(&oldThis, this, sizeof(*this));
+    memcpy(this, &other, sizeof(*this));
+    memcpy(&other, &oldThis, sizeof(*this));
+
     return *this;
 }
 

src/libANGLE/renderer/vulkan/vk_helpers.h

@@ -1792,17 +1792,18 @@ class ImageHelper final : public Resource, public angle::Subject
         VkImageCopy copyRegion;
     };
 
-    struct SubresourceUpdate
+    struct SubresourceUpdate : angle::NonCopyable
     {
         SubresourceUpdate();
+        ~SubresourceUpdate();
         SubresourceUpdate(BufferHelper *bufferHelperIn, const VkBufferImageCopy &copyRegion);
         SubresourceUpdate(ImageHelper *image, const VkImageCopy &copyRegion);
         SubresourceUpdate(VkImageAspectFlags aspectFlags,
                           const VkClearValue &clearValue,
                           const gl::ImageIndex &imageIndex);
-        SubresourceUpdate(const SubresourceUpdate &other);
+        SubresourceUpdate(SubresourceUpdate &&other);
 
-        SubresourceUpdate &operator=(const SubresourceUpdate &other);
+        SubresourceUpdate &operator=(SubresourceUpdate &&other);
 
         void release(RendererVk *renderer);
 
@@ -1824,7 +1825,7 @@ class ImageHelper final : public Resource, public angle::Subject
     // Called from flushStagedUpdates, removes updates that are later superseded by another.  This
     // cannot be done at the time the updates were staged, as the image is not created (and thus the
     // extents are not known).
-    void removeSupersededUpdates(gl::TexLevelMask skipLevelsMask);
+    void removeSupersededUpdates(ContextVk *contextVk, gl::TexLevelMask skipLevelsMask);
 
     void initImageMemoryBarrierStruct(VkImageAspectFlags aspectMask,
                                       ImageLayout newLayout,