Fix double MapReadCallback trigger when Unmapping in it.

When calling unmap on a mapped buffer for which the callback hasn't
fired yet, the callback should be called with UNKNOWN. The code marked
the callback as called only after calling it, causing problems with
re-entrancy where the callback would be called twice.

This could also get triggered by destroying the buffer inside the
callback.

Fix this in backend::Buffer and the WireClient and add test for both.

generator/templates/wire/WireClient.cpp

@@ -532,6 +532,8 @@ namespace wire {
                     }
 
                     auto request = requestIt->second;
+                    // Delete the request before calling the callback otherwise the callback could be fired a second time if for example buffer.Unmap() is called inside the callback.
+                    buffer->readRequests.erase(requestIt);
 
                     //* On success, we copy the data locally because the IPC buffer isn't valid outside of this function
                     if (cmd->status == NXT_BUFFER_MAP_READ_STATUS_SUCCESS) {
@@ -553,7 +555,6 @@ namespace wire {
                         request.callback(static_cast<nxtBufferMapReadStatus>(cmd->status), nullptr, request.userdata);
                     }
 
-                    buffer->readRequests.erase(requestIt);
                     return true;
                 }
         };

src/backend/Buffer.cpp

@@ -61,8 +61,11 @@ namespace backend {
                                          nxtBufferMapReadStatus status,
                                          const void* pointer) {
         if (mMapReadCallback && serial == mMapReadSerial) {
-            mMapReadCallback(status, pointer, mMapReadUserdata);
+            // Tag the callback as fired before firing it, otherwise it could fire a second time if
+            // for example buffer.Unmap() is called inside the application-provided callback.
+            nxtBufferMapReadCallback callback = mMapReadCallback;
             mMapReadCallback = nullptr;
+            callback(status, pointer, mMapReadUserdata);
         }
     }
 

src/tests/unittests/WireTests.cpp

@@ -724,3 +724,55 @@ TEST_F(WireBufferMappingTests, MappingErrorWhileAlreadyMappedGetsNullptr) {
 
     FlushServer();
 }
+
+// Test that the MapReadCallback isn't fired twice when unmap() is called inside the callback
+TEST_F(WireBufferMappingTests, UnmapInsideMapReadCallback) {
+    nxtCallbackUserdata userdata = 2039;
+    nxtBufferMapReadAsync(buffer, 40, sizeof(uint32_t), ToMockBufferMapReadCallback, userdata);
+
+    uint32_t bufferContent = 31337;
+    EXPECT_CALL(api, OnBufferMapReadAsyncCallback(apiBuffer, 40, sizeof(uint32_t), _, _))
+        .WillOnce(InvokeWithoutArgs([&]() {
+            api.CallMapReadCallback(apiBuffer, NXT_BUFFER_MAP_READ_STATUS_SUCCESS, &bufferContent);
+        }));
+
+    FlushClient();
+
+    EXPECT_CALL(*mockBufferMapReadCallback, Call(NXT_BUFFER_MAP_READ_STATUS_SUCCESS, Pointee(Eq(bufferContent)), userdata))
+        .WillOnce(InvokeWithoutArgs([&]() {
+            nxtBufferUnmap(buffer);
+        }));
+
+    FlushServer();
+
+    EXPECT_CALL(api, BufferUnmap(apiBuffer))
+        .Times(1);
+
+    FlushClient();
+}
+
+// Test that the MapReadCallback isn't fired twice the buffer external refcount reaches 0 in the callback
+TEST_F(WireBufferMappingTests, DestroyInsideMapReadCallback) {
+    nxtCallbackUserdata userdata = 2039;
+    nxtBufferMapReadAsync(buffer, 40, sizeof(uint32_t), ToMockBufferMapReadCallback, userdata);
+
+    uint32_t bufferContent = 31337;
+    EXPECT_CALL(api, OnBufferMapReadAsyncCallback(apiBuffer, 40, sizeof(uint32_t), _, _))
+        .WillOnce(InvokeWithoutArgs([&]() {
+            api.CallMapReadCallback(apiBuffer, NXT_BUFFER_MAP_READ_STATUS_SUCCESS, &bufferContent);
+        }));
+
+    FlushClient();
+
+    EXPECT_CALL(*mockBufferMapReadCallback, Call(NXT_BUFFER_MAP_READ_STATUS_SUCCESS, Pointee(Eq(bufferContent)), userdata))
+        .WillOnce(InvokeWithoutArgs([&]() {
+            nxtBufferRelease(buffer);
+        }));
+
+    FlushServer();
+
+    EXPECT_CALL(api, BufferRelease(apiBuffer))
+        .Times(1);
+
+    FlushClient();
+}

src/tests/unittests/validation/BufferValidationTests.cpp

@@ -290,6 +290,36 @@ TEST_F(BufferValidationTest, MapReadUnmapBeforeResultThenMapAgain) {
     queue.Submit(0, nullptr);
 }
 
+// Test that the MapReadCallback isn't fired twice when unmap() is called inside the callback
+TEST_F(BufferValidationTest, UnmapInsideMapReadCallback) {
+    nxt::Buffer buf = CreateMapReadBuffer(4);
+
+    nxt::CallbackUserdata userdata = 40678;
+    buf.MapReadAsync(0, 4, ToMockBufferMapReadCallback, userdata);
+
+    EXPECT_CALL(*mockBufferMapReadCallback, Call(NXT_BUFFER_MAP_READ_STATUS_SUCCESS, Ne(nullptr), userdata))
+        .WillOnce(InvokeWithoutArgs([&]() {
+            buf.Unmap();
+        }));
+
+    queue.Submit(0, nullptr);
+}
+
+// Test that the MapReadCallback isn't fired twice the buffer external refcount reaches 0 in the callback
+TEST_F(BufferValidationTest, DestroyInsideMapReadCallback) {
+    nxt::Buffer buf = CreateMapReadBuffer(4);
+
+    nxt::CallbackUserdata userdata = 40679;
+    buf.MapReadAsync(0, 4, ToMockBufferMapReadCallback, userdata);
+
+    EXPECT_CALL(*mockBufferMapReadCallback, Call(NXT_BUFFER_MAP_READ_STATUS_SUCCESS, Ne(nullptr), userdata))
+        .WillOnce(InvokeWithoutArgs([&]() {
+            buf = nxt::Buffer();
+        }));
+
+    queue.Submit(0, nullptr);
+}
+
 // Test the success case for Buffer::SetSubData
 TEST_F(BufferValidationTest, SetSubDataSuccess) {
     nxt::Buffer buf = CreateSetSubDataBuffer(4);