Update GetMappedRange to not produce validation errors

GetMappedRange never produces errors and instead returns nullptr when it
is disallowed. When in a correct state, should return a valid pointer as
much as possible, even if the buffer is an error or if the device is
lost.

Adds tests for error buffers and device loss, and modify existing tests
to not expect a device error.

Also removes some dead code in the Vulkan backend and adds a fix for
missing deallocation of VkMemory on device shutdown.

Bug: dawn:445

Change-Id: Ia844ee3493cdaf75083424743dd194fa94faf591
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/24160
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Stephen White <senorblanco@chromium.org>
diff --git a/src/dawn_native/Buffer.cpp b/src/dawn_native/Buffer.cpp
index 0414159..cc1b6e7 100644
--- a/src/dawn_native/Buffer.cpp
+++ b/src/dawn_native/Buffer.cpp
@@ -311,22 +311,26 @@
     }
 
     void* BufferBase::GetMappedRange() {
-        if (GetDevice()->ConsumedError(ValidateGetMappedRange(true))) {
-            return nullptr;
-        }
-        if (mStagingBuffer != nullptr) {
-            return mStagingBuffer->GetMappedPointer();
-        }
-        return GetMappedPointerImpl();
+        return GetMappedRangeInternal(true);
     }
 
     const void* BufferBase::GetConstMappedRange() {
-        if (GetDevice()->ConsumedError(ValidateGetMappedRange(false))) {
+        return GetMappedRangeInternal(false);
+    }
+
+    // TODO(dawn:445): When CreateBufferMapped is removed, make GetMappedRangeInternal also take
+    // care of the validation of GetMappedRange.
+    void* BufferBase::GetMappedRangeInternal(bool writable) {
+        if (!CanGetMappedRange(writable)) {
             return nullptr;
         }
+
         if (mStagingBuffer != nullptr) {
             return mStagingBuffer->GetMappedPointer();
         }
+        if (mSize == 0) {
+            return reinterpret_cast<uint8_t*>(intptr_t(0xCAFED00D));
+        }
         return GetMappedPointerImpl();
     }
 
@@ -431,24 +435,29 @@
         return {};
     }
 
-    MaybeError BufferBase::ValidateGetMappedRange(bool writable) const {
-        DAWN_TRY(GetDevice()->ValidateIsAlive());
-        DAWN_TRY(GetDevice()->ValidateObject(this));
+    bool BufferBase::CanGetMappedRange(bool writable) const {
+        // Note that:
+        //
+        //   - We don't check that the device is alive because the application can ask for the
+        //     mapped pointer before it knows, and even Dawn knows, that the device was lost, and
+        //     still needs to work properly.
+        //   - We don't check that the object is alive because we need to return mapped pointers
+        //     for error buffers too.
 
         switch (mState) {
             // Writeable Buffer::GetMappedRange is always allowed when mapped at creation.
             case BufferState::MappedAtCreation:
-                return {};
+                return true;
 
             case BufferState::Mapped:
-                if (writable && !(mUsage & wgpu::BufferUsage::MapWrite)) {
-                    return DAWN_VALIDATION_ERROR("GetMappedRange requires the MapWrite usage");
-                }
-                return {};
+                ASSERT(bool(mUsage & wgpu::BufferUsage::MapRead) ^
+                       bool(mUsage & wgpu::BufferUsage::MapWrite));
+                return !writable || (mUsage & wgpu::BufferUsage::MapWrite);
 
             case BufferState::Unmapped:
             case BufferState::Destroyed:
-                return DAWN_VALIDATION_ERROR("Buffer is not mapped");
+                return false;
+
             default:
                 UNREACHABLE();
         }
@@ -493,7 +502,7 @@
     }
 
     void BufferBase::OnMapCommandSerialFinished(uint32_t mapSerial, bool isWrite) {
-        void* data = GetMappedPointerImpl();
+        void* data = GetMappedRangeInternal(isWrite);
         if (isWrite) {
             CallMapWriteCallback(mapSerial, WGPUBufferMapAsyncStatus_Success, data, GetSize());
         } else {
diff --git a/src/dawn_native/Buffer.h b/src/dawn_native/Buffer.h
index ea67fab..3dfaa2a 100644
--- a/src/dawn_native/Buffer.h
+++ b/src/dawn_native/Buffer.h
@@ -72,15 +72,6 @@
         BufferBase(DeviceBase* device, ObjectBase::ErrorTag tag);
         ~BufferBase() override;
 
-        void CallMapReadCallback(uint32_t serial,
-                                 WGPUBufferMapAsyncStatus status,
-                                 const void* pointer,
-                                 uint64_t dataLength);
-        void CallMapWriteCallback(uint32_t serial,
-                                  WGPUBufferMapAsyncStatus status,
-                                  void* pointer,
-                                  uint64_t dataLength);
-
         void DestroyInternal();
 
         bool IsMapped() const;
@@ -95,12 +86,21 @@
 
         virtual bool IsMapWritable() const = 0;
         MaybeError CopyFromStagingBuffer();
+        void* GetMappedRangeInternal(bool writable);
+        void CallMapReadCallback(uint32_t serial,
+                                 WGPUBufferMapAsyncStatus status,
+                                 const void* pointer,
+                                 uint64_t dataLength);
+        void CallMapWriteCallback(uint32_t serial,
+                                  WGPUBufferMapAsyncStatus status,
+                                  void* pointer,
+                                  uint64_t dataLength);
 
         MaybeError ValidateMap(wgpu::BufferUsage requiredUsage,
                                WGPUBufferMapAsyncStatus* status) const;
-        MaybeError ValidateGetMappedRange(bool writable) const;
         MaybeError ValidateUnmap() const;
         MaybeError ValidateDestroy() const;
+        bool CanGetMappedRange(bool writable) const;
 
         uint64_t mSize = 0;
         wgpu::BufferUsage mUsage = wgpu::BufferUsage::None;
diff --git a/src/dawn_native/vulkan/BufferVk.cpp b/src/dawn_native/vulkan/BufferVk.cpp
index 74e8469..37758cf 100644
--- a/src/dawn_native/vulkan/BufferVk.cpp
+++ b/src/dawn_native/vulkan/BufferVk.cpp
@@ -176,14 +176,6 @@
         DestroyInternal();
     }
 
-    void Buffer::OnMapReadCommandSerialFinished(uint32_t mapSerial, const void* data) {
-        CallMapReadCallback(mapSerial, WGPUBufferMapAsyncStatus_Success, data, GetSize());
-    }
-
-    void Buffer::OnMapWriteCommandSerialFinished(uint32_t mapSerial, void* data) {
-        CallMapWriteCallback(mapSerial, WGPUBufferMapAsyncStatus_Success, data, GetSize());
-    }
-
     VkBuffer Buffer::GetHandle() const {
         return mHandle;
     }
diff --git a/src/dawn_native/vulkan/BufferVk.h b/src/dawn_native/vulkan/BufferVk.h
index ace13a9..feb64b4 100644
--- a/src/dawn_native/vulkan/BufferVk.h
+++ b/src/dawn_native/vulkan/BufferVk.h
@@ -30,9 +30,6 @@
       public:
         static ResultOrError<Buffer*> Create(Device* device, const BufferDescriptor* descriptor);
 
-        void OnMapReadCommandSerialFinished(uint32_t mapSerial, const void* data);
-        void OnMapWriteCommandSerialFinished(uint32_t mapSerial, void* data);
-
         VkBuffer GetHandle() const;
 
         // Transitions the buffer to be used as `usage`, recording any necessary barrier in
diff --git a/src/dawn_native/vulkan/DeviceVk.cpp b/src/dawn_native/vulkan/DeviceVk.cpp
index 5a2a905..da8c592 100644
--- a/src/dawn_native/vulkan/DeviceVk.cpp
+++ b/src/dawn_native/vulkan/DeviceVk.cpp
@@ -853,6 +853,7 @@
 
         // Releasing the uploader enqueues buffers to be released.
         // Call Tick() again to clear them before releasing the deleter.
+        mResourceMemoryAllocator->Tick(GetCompletedCommandSerial());
         mDeleter->Tick(GetCompletedCommandSerial());
 
         // The VkRenderPasses in the cache can be destroyed immediately since all commands referring
diff --git a/src/dawn_wire/client/Buffer.cpp b/src/dawn_wire/client/Buffer.cpp
index 993b2b2..eeb5e49 100644
--- a/src/dawn_wire/client/Buffer.cpp
+++ b/src/dawn_wire/client/Buffer.cpp
@@ -367,7 +367,6 @@
 
     void* Buffer::GetMappedRange() {
         if (!IsMappedForWriting()) {
-            device->InjectError(WGPUErrorType_Validation, "Buffer needs to be mapped for writing");
             return nullptr;
         }
         return mMappedData;
@@ -375,7 +374,6 @@
 
     const void* Buffer::GetConstMappedRange() {
         if (!IsMappedForWriting() && !IsMappedForReading()) {
-            device->InjectError(WGPUErrorType_Validation, "Buffer needs to be mapped");
             return nullptr;
         }
         return mMappedData;
diff --git a/src/tests/end2end/BufferTests.cpp b/src/tests/end2end/BufferTests.cpp
index b0ff2b8..7163051 100644
--- a/src/tests/end2end/BufferTests.cpp
+++ b/src/tests/end2end/BufferTests.cpp
@@ -127,7 +127,21 @@
     wgpu::Buffer buffer = device.CreateBuffer(&descriptor);
 
     const void* mappedData = MapReadAsyncAndWait(buffer);
-    ASSERT_EQ(mappedData, buffer.GetConstMappedRange());
+    ASSERT_EQ(buffer.GetConstMappedRange(), mappedData);
+    ASSERT_NE(buffer.GetConstMappedRange(), nullptr);
+    UnmapBuffer(buffer);
+}
+
+// Test the result of GetMappedRange when mapped for reading for a zero-sized buffer.
+TEST_P(BufferMapReadTests, GetMappedRangeZeroSized) {
+    wgpu::BufferDescriptor descriptor;
+    descriptor.size = 0;
+    descriptor.usage = wgpu::BufferUsage::MapRead | wgpu::BufferUsage::CopyDst;
+    wgpu::Buffer buffer = device.CreateBuffer(&descriptor);
+
+    const void* mappedData = MapReadAsyncAndWait(buffer);
+    ASSERT_EQ(buffer.GetConstMappedRange(), mappedData);
+    ASSERT_NE(buffer.GetConstMappedRange(), nullptr);
     UnmapBuffer(buffer);
 }
 
@@ -273,8 +287,23 @@
     wgpu::Buffer buffer = device.CreateBuffer(&descriptor);
 
     void* mappedData = MapWriteAsyncAndWait(buffer);
-    ASSERT_EQ(mappedData, buffer.GetMappedRange());
-    ASSERT_EQ(mappedData, buffer.GetConstMappedRange());
+    ASSERT_EQ(buffer.GetMappedRange(), mappedData);
+    ASSERT_EQ(buffer.GetMappedRange(), buffer.GetConstMappedRange());
+    ASSERT_NE(buffer.GetMappedRange(), nullptr);
+    UnmapBuffer(buffer);
+}
+
+// Test the result of GetMappedRange when mapped for writing for a zero-sized buffer.
+TEST_P(BufferMapWriteTests, GetMappedRangeZeroSized) {
+    wgpu::BufferDescriptor descriptor;
+    descriptor.size = 0;
+    descriptor.usage = wgpu::BufferUsage::MapWrite | wgpu::BufferUsage::CopySrc;
+    wgpu::Buffer buffer = device.CreateBuffer(&descriptor);
+
+    void* mappedData = MapWriteAsyncAndWait(buffer);
+    ASSERT_EQ(buffer.GetMappedRange(), mappedData);
+    ASSERT_EQ(buffer.GetMappedRange(), buffer.GetConstMappedRange());
+    ASSERT_NE(buffer.GetMappedRange(), nullptr);
     UnmapBuffer(buffer);
 }
 
@@ -538,8 +567,23 @@
     wgpu::CreateBufferMappedResult result;
     result = device.CreateBufferMapped(&descriptor);
 
-    ASSERT_EQ(result.data, result.buffer.GetMappedRange());
-    ASSERT_EQ(result.data, result.buffer.GetConstMappedRange());
+    ASSERT_EQ(result.buffer.GetMappedRange(), result.data);
+    ASSERT_EQ(result.buffer.GetMappedRange(), result.buffer.GetConstMappedRange());
+    ASSERT_NE(result.buffer.GetMappedRange(), nullptr);
+    result.buffer.Unmap();
+}
+
+// Test the result of GetMappedRange when mapped at creation for a zero-sized buffer.
+TEST_P(CreateBufferMappedTests, GetMappedRangeZeroSized) {
+    wgpu::BufferDescriptor descriptor;
+    descriptor.size = 0;
+    descriptor.usage = wgpu::BufferUsage::CopyDst;
+    wgpu::CreateBufferMappedResult result;
+    result = device.CreateBufferMapped(&descriptor);
+
+    ASSERT_EQ(result.buffer.GetMappedRange(), result.data);
+    ASSERT_EQ(result.buffer.GetMappedRange(), result.buffer.GetConstMappedRange());
+    ASSERT_NE(result.buffer.GetMappedRange(), nullptr);
     result.buffer.Unmap();
 }
 
diff --git a/src/tests/end2end/DeviceLostTests.cpp b/src/tests/end2end/DeviceLostTests.cpp
index fbae566..40d8fe4 100644
--- a/src/tests/end2end/DeviceLostTests.cpp
+++ b/src/tests/end2end/DeviceLostTests.cpp
@@ -332,6 +332,73 @@
     ASSERT_DEVICE_ERROR(queue.WriteBuffer(buffer, 0, &data, sizeof(data)));
 }
 
+// Test it's possible to GetMappedRange on a buffer created mapped after device loss
+// TODO(cwallez@chromium.org): enable after CreateBufferMapped is implemented in terms of
+// mappedAtCreation.
+TEST_P(DeviceLostTest, DISABLED_GetMappedRange_CreateBufferMappedAfterLoss) {
+    SetCallbackAndLoseForTesting();
+
+    wgpu::BufferDescriptor desc;
+    desc.size = 4;
+    desc.usage = wgpu::BufferUsage::CopySrc;
+    ASSERT_DEVICE_ERROR(wgpu::CreateBufferMappedResult result = device.CreateBufferMapped(&desc));
+
+    ASSERT_NE(result.buffer.GetMappedRange(), nullptr);
+    ASSERT_EQ(result.buffer.GetMappedRange(), result.data);
+}
+
+// Test that device loss doesn't change the result of GetMappedRange, createBufferMapped version.
+TEST_P(DeviceLostTest, GetMappedRange_CreateBufferMappedBeforeLoss) {
+    wgpu::BufferDescriptor desc;
+    desc.size = 4;
+    desc.usage = wgpu::BufferUsage::CopySrc;
+    wgpu::CreateBufferMappedResult result = device.CreateBufferMapped(&desc);
+
+    void* rangeBeforeLoss = result.buffer.GetMappedRange();
+    SetCallbackAndLoseForTesting();
+
+    ASSERT_NE(result.buffer.GetMappedRange(), nullptr);
+    ASSERT_EQ(result.buffer.GetMappedRange(), rangeBeforeLoss);
+    ASSERT_EQ(result.buffer.GetMappedRange(), result.data);
+}
+
+// Test that device loss doesn't change the result of GetMappedRange, mapReadAsync version.
+TEST_P(DeviceLostTest, GetMappedRange_MapReadAsync) {
+    wgpu::BufferDescriptor desc;
+    desc.size = 4;
+    desc.usage = wgpu::BufferUsage::MapRead | wgpu::BufferUsage::CopyDst;
+    wgpu::Buffer buffer = device.CreateBuffer(&desc);
+
+    buffer.MapReadAsync(nullptr, nullptr);
+    queue.Submit(0, nullptr);
+
+    const void* rangeBeforeLoss = buffer.GetConstMappedRange();
+    SetCallbackAndLoseForTesting();
+
+    ASSERT_NE(buffer.GetConstMappedRange(), nullptr);
+    ASSERT_EQ(buffer.GetConstMappedRange(), rangeBeforeLoss);
+}
+
+// Test that device loss doesn't change the result of GetMappedRange, mapReadAsync version.
+TEST_P(DeviceLostTest, GetMappedRange_MapWriteAsync) {
+    wgpu::BufferDescriptor desc;
+    desc.size = 4;
+    desc.usage = wgpu::BufferUsage::MapWrite | wgpu::BufferUsage::CopySrc;
+    wgpu::Buffer buffer = device.CreateBuffer(&desc);
+
+    buffer.MapWriteAsync(nullptr, nullptr);
+    queue.Submit(0, nullptr);
+
+    const void* rangeBeforeLoss = buffer.GetConstMappedRange();
+    SetCallbackAndLoseForTesting();
+
+    ASSERT_NE(buffer.GetConstMappedRange(), nullptr);
+    ASSERT_EQ(buffer.GetConstMappedRange(), rangeBeforeLoss);
+}
+
+// mapreadasync + resolve + loss getmappedrange != nullptr.
+// mapwriteasync + resolve + loss getmappedrange != nullptr.
+
 // Test that Command Encoder Finish fails when device lost
 TEST_P(DeviceLostTest, CommandEncoderFinishFails) {
     wgpu::CommandBuffer commands;
diff --git a/src/tests/unittests/validation/BufferValidationTests.cpp b/src/tests/unittests/validation/BufferValidationTests.cpp
index d1ececf..eedd18b 100644
--- a/src/tests/unittests/validation/BufferValidationTests.cpp
+++ b/src/tests/unittests/validation/BufferValidationTests.cpp
@@ -672,8 +672,8 @@
         desc.usage = wgpu::BufferUsage::CopySrc;
         wgpu::Buffer buf = device.CreateBuffer(&desc);
 
-        ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetMappedRange()));
-        ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetConstMappedRange()));
+        ASSERT_EQ(nullptr, buf.GetMappedRange());
+        ASSERT_EQ(nullptr, buf.GetConstMappedRange());
     }
 
     // Unmapped after CreateBufferMapped case.
@@ -681,8 +681,8 @@
         wgpu::Buffer buf = CreateBufferMapped(4, wgpu::BufferUsage::CopySrc).buffer;
         buf.Unmap();
 
-        ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetMappedRange()));
-        ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetConstMappedRange()));
+        ASSERT_EQ(nullptr, buf.GetMappedRange());
+        ASSERT_EQ(nullptr, buf.GetConstMappedRange());
     }
 
     // Unmapped after MapReadAsync case.
@@ -696,8 +696,8 @@
         queue.Submit(0, nullptr);
         buf.Unmap();
 
-        ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetMappedRange()));
-        ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetConstMappedRange()));
+        ASSERT_EQ(nullptr, buf.GetMappedRange());
+        ASSERT_EQ(nullptr, buf.GetConstMappedRange());
     }
 
     // Unmapped after MapWriteAsync case.
@@ -710,8 +710,8 @@
         queue.Submit(0, nullptr);
         buf.Unmap();
 
-        ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetMappedRange()));
-        ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetConstMappedRange()));
+        ASSERT_EQ(nullptr, buf.GetMappedRange());
+        ASSERT_EQ(nullptr, buf.GetConstMappedRange());
     }
 }
 
@@ -725,8 +725,8 @@
         wgpu::Buffer buf = device.CreateBuffer(&desc);
         buf.Destroy();
 
-        ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetMappedRange()));
-        ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetConstMappedRange()));
+        ASSERT_EQ(nullptr, buf.GetMappedRange());
+        ASSERT_EQ(nullptr, buf.GetConstMappedRange());
     }
 
     // Destroyed after CreateBufferMapped case.
@@ -734,8 +734,8 @@
         wgpu::Buffer buf = CreateBufferMapped(4, wgpu::BufferUsage::CopySrc).buffer;
         buf.Destroy();
 
-        ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetMappedRange()));
-        ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetConstMappedRange()));
+        ASSERT_EQ(nullptr, buf.GetMappedRange());
+        ASSERT_EQ(nullptr, buf.GetConstMappedRange());
     }
 
     // Destroyed after MapReadAsync case.
@@ -749,8 +749,8 @@
         queue.Submit(0, nullptr);
         buf.Destroy();
 
-        ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetMappedRange()));
-        ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetConstMappedRange()));
+        ASSERT_EQ(nullptr, buf.GetMappedRange());
+        ASSERT_EQ(nullptr, buf.GetConstMappedRange());
     }
 
     // Destroyed after MapWriteAsync case.
@@ -763,8 +763,8 @@
         queue.Submit(0, nullptr);
         buf.Destroy();
 
-        ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetMappedRange()));
-        ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetConstMappedRange()));
+        ASSERT_EQ(nullptr, buf.GetMappedRange());
+        ASSERT_EQ(nullptr, buf.GetConstMappedRange());
     }
 }
 
@@ -778,7 +778,7 @@
         .Times(1);
     queue.Submit(0, nullptr);
 
-    ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetMappedRange()));
+    ASSERT_EQ(nullptr, buf.GetMappedRange());
 }
 
 // Test valid cases to call GetMappedRange on a buffer.
@@ -825,3 +825,34 @@
         ASSERT_EQ(buf.GetConstMappedRange(), mappedPointer);
     }
 }
+
+// Test valid cases to call GetMappedRange on an error buffer.
+// TODO(cwallez@chromium.org): enable after CreateBufferMapped is implemented in terms of
+// mappedAtCreation.
+TEST_F(BufferValidationTest, DISABLED_GetMappedRangeOnErrorBuffer) {
+    wgpu::BufferDescriptor desc;
+    desc.size = 4;
+    desc.usage = wgpu::BufferUsage::Storage | wgpu::BufferUsage::MapRead;
+
+    // GetMappedRange after CreateBufferMapped non-OOM returns a non-nullptr.
+    {
+        wgpu::CreateBufferMappedResult result;
+        ASSERT_DEVICE_ERROR(result = CreateBufferMapped(
+                                4, wgpu::BufferUsage::Storage | wgpu::BufferUsage::MapRead));
+
+        ASSERT_NE(result.buffer.GetConstMappedRange(), nullptr);
+        ASSERT_EQ(result.buffer.GetConstMappedRange(), result.buffer.GetMappedRange());
+        ASSERT_EQ(result.buffer.GetConstMappedRange(), result.data);
+    }
+
+    // GetMappedRange after CreateBufferMapped OOM case returns nullptr.
+    {
+        wgpu::CreateBufferMappedResult result;
+        ASSERT_DEVICE_ERROR(result = CreateBufferMapped(
+                                1 << 31, wgpu::BufferUsage::Storage | wgpu::BufferUsage::MapRead));
+
+        ASSERT_EQ(result.buffer.GetConstMappedRange(), nullptr);
+        ASSERT_EQ(result.buffer.GetConstMappedRange(), result.buffer.GetMappedRange());
+        ASSERT_EQ(result.buffer.GetConstMappedRange(), result.data);
+    }
+}