Vulkan: Fix the AHB leak for AHB backed buffer object

For client buffer backed OpenGL buffer object, we call
InitAndroidExternalMemory which calls AHB acquire. But when buffer
object is released/destroyed, we never call
ReleaseAndroidExternalMemory, which end up leaking AHB.

Bug: b/314791770
Change-Id: I693c74213e73008497a6dfeca93ea62e84c71352
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/5106599
Reviewed-by: Shahbaz Youssefi <syoussefi@chromium.org>
Commit-Queue: Shahbaz Youssefi <syoussefi@chromium.org>
Auto-Submit: Charlie Lao <cclao@google.com>

3 files changed, 51 insertions, 4 deletions

diff --git a/src/libANGLE/renderer/vulkan/vk_helpers.cpp b/src/libANGLE/renderer/vulkan/vk_helpers.cpp
index 419800416e..163ecd3b9d 100644
--- a/src/libANGLE/renderer/vulkan/vk_helpers.cpp
+++ b/src/libANGLE/renderer/vulkan/vk_helpers.cpp
@@ -4789,10 +4789,15 @@ BufferHelper::BufferHelper()
       mCurrentReadAccess(0),
       mCurrentWriteStages(0),
       mCurrentReadStages(0),
-      mSerial()
+      mSerial(),
+      mClientBuffer(nullptr)
 {}
 
-BufferHelper::~BufferHelper() = default;
+BufferHelper::~BufferHelper()
+{
+    // We must have released external buffer properly
+    ASSERT(mClientBuffer == nullptr);
+}
 
 BufferHelper::BufferHelper(BufferHelper &&other)
 {
@@ -4812,6 +4817,7 @@ BufferHelper &BufferHelper::operator=(BufferHelper &&other)
     mCurrentWriteStages      = other.mCurrentWriteStages;
     mCurrentReadStages       = other.mCurrentReadStages;
     mSerial                  = other.mSerial;
+    mClientBuffer            = std::move(other.mClientBuffer);
 
     return *this;
 }
@@ -4916,6 +4922,7 @@ angle::Result BufferHelper::initExternal(ContextVk *contextVk,
     ANGLE_TRY(InitAndroidExternalMemory(contextVk, clientBuffer, memoryProperties, &buffer.get(),
                                         &memoryPropertyFlagsOut, &memoryTypeIndex,
                                         &deviceMemory.get(), &allocatedSize));
+    mClientBuffer = clientBuffer;
 
     mSuballocation.initWithEntireBuffer(
         contextVk, buffer.get(), MemoryAllocationType::BufferExternal, memoryTypeIndex,
@@ -4925,7 +4932,6 @@ angle::Result BufferHelper::initExternal(ContextVk *contextVk,
         uint8_t *ptrOut;
         ANGLE_TRY(map(contextVk, &ptrOut));
     }
-
     return angle::Result::Continue;
 }
 
@@ -5090,6 +5096,11 @@ void BufferHelper::destroy(RendererVk *renderer)
     unmap(renderer);
     mBufferWithUserSize.destroy(renderer->getDevice());
     mSuballocation.destroy(renderer);
+    if (mClientBuffer != nullptr)
+    {
+        ReleaseAndroidExternalMemory(renderer, mClientBuffer);
+        mClientBuffer = nullptr;
+    }
 }
 
 void BufferHelper::release(RendererVk *renderer)
@@ -5111,6 +5122,12 @@ void BufferHelper::release(RendererVk *renderer)
     mUse.reset();
     mWriteUse.reset();
     ASSERT(!mBufferWithUserSize.valid());
+
+    if (mClientBuffer != nullptr)
+    {
+        ReleaseAndroidExternalMemory(renderer, mClientBuffer);
+        mClientBuffer = nullptr;
+    }
 }
 
 void BufferHelper::releaseBufferAndDescriptorSetCache(RendererVk *renderer)
diff --git a/src/libANGLE/renderer/vulkan/vk_helpers.h b/src/libANGLE/renderer/vulkan/vk_helpers.h
index bad0c94bd2..5204f09d40 100644
--- a/src/libANGLE/renderer/vulkan/vk_helpers.h
+++ b/src/libANGLE/renderer/vulkan/vk_helpers.h
@@ -885,6 +885,8 @@ class BufferHelper : public ReadWriteResource
     BufferSerial mSerial;
     // Manages the descriptorSet cache that created with this BufferHelper object.
     DescriptorSetCacheManager mDescriptorSetCacheManager;
+    // For external buffer
+    GLeglClientBufferEXT mClientBuffer;
 };
 
 class BufferPool : angle::NonCopyable
diff --git a/src/tests/gl_tests/ExternalBufferTest.cpp b/src/tests/gl_tests/ExternalBufferTest.cpp
index 465f7d66ae..59720c9cb9 100644
--- a/src/tests/gl_tests/ExternalBufferTest.cpp
+++ b/src/tests/gl_tests/ExternalBufferTest.cpp
@@ -60,7 +60,10 @@ class ExternalBufferTestES31 : public ANGLETest<>
         // Need to grab the stride the implementation might have enforced
         AHardwareBuffer_describe(aHardwareBuffer, &aHardwareBufferDescription);
 
-        memcpy(mappedMemory, data, size);
+        if (data)
+        {
+            memcpy(mappedMemory, data, size);
+        }
 
         EXPECT_EQ(0, AHardwareBuffer_unlock(aHardwareBuffer, nullptr));
         return aHardwareBuffer;
@@ -345,6 +348,31 @@ TEST_P(ExternalBufferTestES31, MapBufferDoesNotCauseOrphaning)
     destroyAndroidHardwareBuffer(aHardwareBuffer);
 }
 
+// Verify that create and destroy external buffer backed by an AHB doesn't leak AHB
+TEST_P(ExternalBufferTestES31, BufferDoesNotLeakAHB)
+{
+    ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_EXT_external_buffer") ||
+                       !IsGLExtensionEnabled("GL_EXT_buffer_storage"));
+
+    // Create and destroy 128M AHB backed buffer in a loop. If we leak AHB, it will fail due to AHB
+    // allocation failure before loop ends.
+    constexpr size_t kBufferSize = 128 * 1024 * 1024;
+    for (int loop = 0; loop < 1000; loop++)
+    {
+        // Create the AHB
+        AHardwareBuffer *aHardwareBuffer;
+        constexpr GLbitfield kFlags = GL_DYNAMIC_STORAGE_BIT_EXT;
+        aHardwareBuffer             = createAndroidHardwareBuffer(kBufferSize, nullptr);
+        GLBuffer buffer;
+        glBindBuffer(GL_SHADER_STORAGE_BUFFER, buffer);
+        glBufferStorageExternalEXT(GL_SHADER_STORAGE_BUFFER, 0, kBufferSize,
+                                   eglGetNativeClientBufferANDROID(aHardwareBuffer), kFlags);
+        ASSERT_GL_NO_ERROR();
+        // Delete the source AHB
+        destroyAndroidHardwareBuffer(aHardwareBuffer);
+    }
+}
+
 GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(ExternalBufferTestES31);
 ANGLE_INSTANTIATE_TEST_ES31(ExternalBufferTestES31);
 }  // namespace angle