commit 35df626efa2ac803d9a578594914d26dc4f27ab6
author Takahiro <hogehoge@gachapin.jp> Mon Feb 27 15:14:13 2023 +0000
committer Dawn LUCI CQ <dawn-scoped@luci-project-accounts.iam.gserviceaccount.com> Mon Feb 27 15:14:13 2023 +0000
tree 27c6a89e92869f640f9d8787f7dd1b6d9097dde5
parent f61577078011f8c1221fabe922843354c6aa7ce2

Refcount check in Buffer map async in Wire

If buffer is released and the external refcount reaches 0 while
buffer map state is pending map in Wire, the map async callback
is fired with destroyed-before-callback status from the buffer
destructor.

It is possible to call another MapAsync in the callback. At that
time the pointer is still valid because the internal refcount is
not 0 yet.

The behavior of MapAsync should be undefined if external refcount
is 0. This commit adds an assert to check whether external
refcount is 0 in Buffer::MapAsync() in Wire. Ending up with
assertion error may be reasonable.

This commit also adds protected GetRefcount() method to
ObjectBase to allow derived classes to check the refcount.

bug: dawn:1624
Change-Id: I95411a7be2093ba7bb2bb45b466f17f1ebac0ca9
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/119961
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>

src/dawn/tests/unittests/wire/WireBufferMappingTests.cpp

diff --git a/src/dawn/tests/unittests/wire/WireBufferMappingTests.cpp b/src/dawn/tests/unittests/wire/WireBufferMappingTests.cpp
index dd37ade..e4bf75d 100644
--- a/src/dawn/tests/unittests/wire/WireBufferMappingTests.cpp
+++ b/src/dawn/tests/unittests/wire/WireBufferMappingTests.cpp
@@ -15,6 +15,7 @@
 #include <limits>
 #include <memory>
 
+#include "dawn/common/Assert.h"
 #include "dawn/tests/unittests/wire/WireTest.h"
 #include "dawn/wire/WireClient.h"
 
@@ -818,6 +819,37 @@
     }
 }
 
+#if defined(DAWN_ENABLE_ASSERTS)
+static void ToMockBufferMapCallbackWithAssertErrorRequest(WGPUBufferMapAsyncStatus status,
+                                                          void* userdata) {
+    WGPUBuffer* buffer = reinterpret_cast<WGPUBuffer*>(userdata);
+
+    mockBufferMapCallback->Call(status, buffer);
+    ASSERT_DEATH(
+        {
+            // This map async should cause assertion error because of
+            // refcount == 0.
+            wgpuBufferMapAsync(*buffer, WGPUMapMode_Read, 0, sizeof(uint32_t),
+                               ToMockBufferMapCallback, nullptr);
+        },
+        "");
+}
+
+// Test that request inside user callbacks after object destruction is called
+TEST_F(WireBufferMappingTests, MapInsideCallbackAfterDestruction) {
+    SetupBuffer(WGPUMapMode_Read);
+
+    wgpuBufferMapAsync(buffer, WGPUMapMode_Read, 0, kBufferSize,
+                       ToMockBufferMapCallbackWithAssertErrorRequest, &buffer);
+
+    // By releasing the buffer the refcount reaches zero and pending map async
+    // should fail with destroyed before callback status.
+    EXPECT_CALL(*mockBufferMapCallback, Call(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback, _))
+        .Times(1);
+    wgpuBufferRelease(buffer);
+}
+#endif  // defined(DAWN_ENABLE_ASSERTS)
+
 // Hack to pass in test context into user callback
 struct TestData {
     WireBufferMappingTests* pTest;
@@ -864,6 +896,7 @@
 
 // Test that requests inside user callbacks before object destruction are called
 TEST_F(WireBufferMappingWriteTests, MapInsideCallbackBeforeDestruction) {
+    SetupBuffer(WGPUMapMode_Write);
     TestData testData = {this, &buffer, 10};
     wgpuBufferMapAsync(buffer, WGPUMapMode_Write, 0, kBufferSize,
                        ToMockBufferMapCallbackWithNewRequests, &testData);
@@ -876,12 +909,22 @@
 
     FlushClient();
 
-    // Maybe this should be assert errors, see dawn:1624
+    // The first map async call should succeed
+    EXPECT_CALL(*mockBufferMapCallback, Call(WGPUBufferMapAsyncStatus_Success, this)).Times(1);
+
+    // The second or later map async calls in the map async callback
+    // should immediately fail because of pending map
     EXPECT_CALL(*mockBufferMapCallback, Call(WGPUBufferMapAsyncStatus_Error, this))
         .Times(testData.numRequests - 1);
+
+    // The first map async call in the map async callback should fail
+    // with destroyed before callback status due to buffer release below
     EXPECT_CALL(*mockBufferMapCallback,
                 Call(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback, this))
         .Times(1);
+
+    FlushServer();
+
     wgpuBufferRelease(buffer);
 }
 

src/dawn/wire/client/Buffer.cpp

diff --git a/src/dawn/wire/client/Buffer.cpp b/src/dawn/wire/client/Buffer.cpp
index b9093e4..32e9ef0 100644
--- a/src/dawn/wire/client/Buffer.cpp
+++ b/src/dawn/wire/client/Buffer.cpp
@@ -183,6 +183,8 @@
                       size_t size,
                       WGPUBufferMapCallback callback,
                       void* userdata) {
+    ASSERT(GetRefcount() != 0);
+
     if (mPendingMap) {
         return callback(WGPUBufferMapAsyncStatus_Error, userdata);
     }

src/dawn/wire/client/ObjectBase.h

diff --git a/src/dawn/wire/client/ObjectBase.h b/src/dawn/wire/client/ObjectBase.h
index cf5f8ee..feb0086 100644
--- a/src/dawn/wire/client/ObjectBase.h
+++ b/src/dawn/wire/client/ObjectBase.h
@@ -51,6 +51,9 @@
     // object.
     [[nodiscard]] bool Release();
 
+  protected:
+    uint32_t GetRefcount() const { return mRefcount; }
+
   private:
     Client* const mClient;
     const ObjectHandle mHandle;