Remove device-child wire hack and enable DeviceLifetimeTests on the wire

Bug: dawn:384, dawn:1164
Change-Id: I88a503513295900975819b56f60738218a1c23ac
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/90920
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Loko Kung <lokokung@google.com>
Commit-Queue: Austin Eng <enga@chromium.org>

generator/templates/dawn/wire/server/ServerDoers.cpp

@@ -77,11 +77,6 @@ namespace dawn::wire::server {
                     if (data == nullptr) {
                         return false;
                     }
-                    if (data->deviceInfo != nullptr) {
-                        if (!UntrackDeviceChild(data->deviceInfo, objectType, objectId)) {
-                            return false;
-                        }
-                    }
                     if (data->state == AllocationState::Allocated) {
                         ASSERT(data->handle != nullptr);
                         {% if type.name.CamelCase() in server_reverse_lookup_objects %}
@@ -89,17 +84,6 @@ namespace dawn::wire::server {
                         {% endif %}
 
                         {% if type.name.get() == "device" %}
-                            //* TODO(crbug.com/dawn/384): This is a hack to make sure that all child objects
-                            //* are destroyed before their device. We should have a solution in
-                            //* Dawn native that makes all child objects internally null if their
-                            //* Device is destroyed.
-                            while (data->info->childObjectTypesAndIds.size() > 0) {
-                                auto [childObjectType, childObjectId] = UnpackObjectTypeAndId(
-                                    *data->info->childObjectTypesAndIds.begin());
-                                if (!DoDestroyObject(childObjectType, childObjectId)) {
-                                    return false;
-                                }
-                            }
                             if (data->handle != nullptr) {
                                 //* Deregisters uncaptured error and device lost callbacks since
                                 //* they should not be forwarded if the device no longer exists on the wire.

generator/templates/dawn/wire/server/ServerHandlers.cpp

@@ -52,24 +52,6 @@ namespace dawn::wire::server {
                     return false;
                 }
                 {{name}}Data->generation = cmd.{{name}}.generation;
-
-                //* TODO(crbug.com/dawn/384): This is a hack to make sure that all child objects
-                //* are destroyed before their device. The dawn_native device needs to track all child objects so
-                //* it can destroy them if the device is destroyed first.
-                {% if command.derived_object %}
-                    {% set type = command.derived_object %}
-                    {% if type.name.get() == "device" %}
-                        {{name}}Data->deviceInfo = DeviceObjects().Get(cmd.selfId)->info.get();
-                    {% else %}
-                        auto* selfData = {{type.name.CamelCase()}}Objects().Get(cmd.selfId);
-                        {{name}}Data->deviceInfo = selfData->deviceInfo;
-                    {% endif %}
-                    if ({{name}}Data->deviceInfo != nullptr) {
-                        if (!TrackDeviceChild({{name}}Data->deviceInfo, ObjectType::{{Type}}, cmd.{{name}}.id)) {
-                            return false;
-                        }
-                    }
-                {% endif %}
             {% endfor %}
 
             //* Do command

src/dawn/tests/BUILD.gn

@@ -307,7 +307,6 @@ dawn_test("dawn_unittests") {
     "unittests/wire/WireBasicTests.cpp",
     "unittests/wire/WireBufferMappingTests.cpp",
     "unittests/wire/WireCreatePipelineAsyncTests.cpp",
-    "unittests/wire/WireDestroyObjectTests.cpp",
     "unittests/wire/WireDisconnectTests.cpp",
     "unittests/wire/WireErrorCallbackTests.cpp",
     "unittests/wire/WireExtensionTests.cpp",

src/dawn/tests/DawnTest.cpp

@@ -716,7 +716,6 @@ DawnTestBase::DawnTestBase(const AdapterTestParam& param)
       mWireHelper(utils::CreateWireHelper(gTestEnv->UsesWire(), gTestEnv->GetWireTraceDir())) {}
 
 DawnTestBase::~DawnTestBase() {
-    // We need to destroy child objects before the Device
     mReadbackSlots.clear();
     queue = wgpu::Queue();
     device = wgpu::Device();

src/dawn/tests/end2end/DeviceLifetimeTests.cpp

@@ -17,14 +17,7 @@
 #include "dawn/tests/DawnTest.h"
 #include "dawn/utils/WGPUHelpers.h"
 
-class DeviceLifetimeTests : public DawnTest {
+class DeviceLifetimeTests : public DawnTest {};
-    void SetUp() override {
-        DawnTest::SetUp();
-        // The wire currently has a different device / device-child lifetime mechanism
-        // which will be removed soon and these tests enabled.
-        DAWN_TEST_UNSUPPORTED_IF(UsesWire());
-    }
-};
 
 // Test that the device can be dropped before its queue.
 TEST_P(DeviceLifetimeTests, DroppedBeforeQueue) {
@@ -86,9 +79,16 @@ TEST_P(DeviceLifetimeTests, DroppedInsideQueueOnSubmittedWorkDone) {
 // Test that the device can be dropped while a popErrorScope callback is in flight.
 TEST_P(DeviceLifetimeTests, DroppedWhilePopErrorScope) {
     device.PushErrorScope(wgpu::ErrorFilter::Validation);
+    bool wire = UsesWire();
     device.PopErrorScope(
-        [](WGPUErrorType type, const char*, void*) { EXPECT_EQ(type, WGPUErrorType_NoError); },
+        [](WGPUErrorType type, const char*, void* userdata) {
-        nullptr);
+            const bool wire = *static_cast<bool*>(userdata);
+            // On the wire, all callbacks get rejected immediately with once the device is deleted.
+            // In native, popErrorScope is called synchronously.
+            // TODO(crbug.com/dawn/1122): These callbacks should be made consistent.
+            EXPECT_EQ(type, wire ? WGPUErrorType_Unknown : WGPUErrorType_NoError);
+        },
+        &wire);
     device = nullptr;
 }
 
@@ -215,11 +215,12 @@ TEST_P(DeviceLifetimeTests, DroppedInsideBufferMapCallback) {
     struct Userdata {
         wgpu::Device device;
         wgpu::Buffer buffer;
+        bool wire;
         bool done;
     };
 
     // Ask for a mapAsync callback and drop the device inside the callback.
-    Userdata data = Userdata{std::move(device), buffer, false};
+    Userdata data = Userdata{std::move(device), buffer, UsesWire(), false};
     buffer.MapAsync(
         wgpu::MapMode::Read, 0, wgpu::kWholeMapSize,
         [](WGPUBufferMapAsyncStatus status, void* userdata) {
@@ -229,7 +230,11 @@ TEST_P(DeviceLifetimeTests, DroppedInsideBufferMapCallback) {
             data->done = true;
 
             // Mapped data should be null since the buffer is implicitly destroyed.
+            // TODO(crbug.com/dawn/1424): On the wire client, we don't track device child objects so
+            // the mapped data is still available when the device is destroyed.
+            if (!data->wire) {
                 EXPECT_EQ(data->buffer.GetConstMappedRange(), nullptr);
+            }
         },
         &data);
 
@@ -243,8 +248,12 @@ TEST_P(DeviceLifetimeTests, DroppedInsideBufferMapCallback) {
     }
 
     // Mapped data should be null since the buffer is implicitly destroyed.
+    // TODO(crbug.com/dawn/1424): On the wire client, we don't track device child objects so the
+    // mapped data is still available when the device is destroyed.
+    if (!UsesWire()) {
         EXPECT_EQ(buffer.GetConstMappedRange(), nullptr);
     }
+}
 
 // Test that the device can be dropped while a write buffer operation is enqueued.
 TEST_P(DeviceLifetimeTests, DroppedWhileWriteBuffer) {
@@ -341,16 +350,20 @@ TEST_P(DeviceLifetimeTests, DroppedWhileCreatePipelineAsyncAlreadyCached) {
     // Create a pipeline ahead of time so it's in the cache.
     wgpu::ComputePipeline p = device.CreateComputePipeline(&desc);
 
+    bool wire = UsesWire();
     device.CreateComputePipelineAsync(
         &desc,
-        [](WGPUCreatePipelineAsyncStatus status, WGPUComputePipeline cPipeline, const char* message,
+        [](WGPUCreatePipelineAsyncStatus status, WGPUComputePipeline cPipeline, const char*,
            void* userdata) {
+            const bool wire = *static_cast<bool*>(userdata);
             wgpu::ComputePipeline::Acquire(cPipeline);
-            // Success because it hits the frontend cache immediately.
+            // On the wire, all callbacks get rejected immediately with once the device is deleted.
-            EXPECT_EQ(status, WGPUCreatePipelineAsyncStatus_Success);
+            // In native, expect success since the compilation hits the frontend cache immediately.
+            // TODO(crbug.com/dawn/1122): These callbacks should be made consistent.
+            EXPECT_EQ(status, wire ? WGPUCreatePipelineAsyncStatus_DeviceDestroyed
+                                   : WGPUCreatePipelineAsyncStatus_Success);
         },
-        nullptr);
+        &wire);
-
     device = nullptr;
 }
 

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

@@ -363,20 +363,10 @@ TEST_F(WireCreatePipelineAsyncTest, DeviceDeletedBeforeCallback) {
 
     wgpuDeviceRelease(device);
 
-    // Expect release on all objects created by the client.
+    EXPECT_CALL(api, OnDeviceSetUncapturedErrorCallback(apiDevice, nullptr, nullptr)).Times(1);
-    Sequence s1, s2;
+    EXPECT_CALL(api, OnDeviceSetLoggingCallback(apiDevice, nullptr, nullptr)).Times(1);
-    EXPECT_CALL(api, QueueRelease(apiQueue)).Times(1).InSequence(s1);
+    EXPECT_CALL(api, OnDeviceSetDeviceLostCallback(apiDevice, nullptr, nullptr)).Times(1);
-    EXPECT_CALL(api, ShaderModuleRelease(apiModule)).Times(1).InSequence(s2);
+    EXPECT_CALL(api, DeviceRelease(apiDevice)).Times(1);
-    EXPECT_CALL(api, OnDeviceSetUncapturedErrorCallback(apiDevice, nullptr, nullptr))
-        .Times(1)
-        .InSequence(s1, s2);
-    EXPECT_CALL(api, OnDeviceSetLoggingCallback(apiDevice, nullptr, nullptr))
-        .Times(1)
-        .InSequence(s1, s2);
-    EXPECT_CALL(api, OnDeviceSetDeviceLostCallback(apiDevice, nullptr, nullptr))
-        .Times(1)
-        .InSequence(s1, s2);
-    EXPECT_CALL(api, DeviceRelease(apiDevice)).Times(1).InSequence(s1, s2);
 
     FlushClient();
     DefaultApiDeviceWasReleased();

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

@@ -1,62 +0,0 @@
-// Copyright 2021 The Dawn Authors
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-#include "dawn/tests/MockCallback.h"
-#include "dawn/tests/unittests/wire/WireTest.h"
-
-namespace dawn::wire {
-
-using testing::Return;
-using testing::Sequence;
-
-class WireDestroyObjectTests : public WireTest {};
-
-// Test that destroying the device also destroys child objects.
-TEST_F(WireDestroyObjectTests, DestroyDeviceDestroysChildren) {
-    WGPUCommandEncoder encoder = wgpuDeviceCreateCommandEncoder(device, nullptr);
-
-    WGPUCommandEncoder apiEncoder = api.GetNewCommandEncoder();
-    EXPECT_CALL(api, DeviceCreateCommandEncoder(apiDevice, nullptr)).WillOnce(Return(apiEncoder));
-
-    FlushClient();
-
-    // Release the device. It should cause the command encoder to be destroyed.
-    wgpuDeviceRelease(device);
-
-    Sequence s1, s2;
-    // The device and child objects should be released.
-    EXPECT_CALL(api, CommandEncoderRelease(apiEncoder)).InSequence(s1);
-    EXPECT_CALL(api, QueueRelease(apiQueue)).InSequence(s2);
-    EXPECT_CALL(api, OnDeviceSetUncapturedErrorCallback(apiDevice, nullptr, nullptr))
-        .Times(1)
-        .InSequence(s1, s2);
-    EXPECT_CALL(api, OnDeviceSetLoggingCallback(apiDevice, nullptr, nullptr))
-        .Times(1)
-        .InSequence(s1, s2);
-    EXPECT_CALL(api, OnDeviceSetDeviceLostCallback(apiDevice, nullptr, nullptr))
-        .Times(1)
-        .InSequence(s1, s2);
-    EXPECT_CALL(api, DeviceRelease(apiDevice)).InSequence(s1, s2);
-
-    FlushClient();
-
-    // Signal that we already released and cleared callbacks for |apiDevice|
-    DefaultApiDeviceWasReleased();
-
-    // Using the command encoder should be an error.
-    wgpuCommandEncoderFinish(encoder, nullptr);
-    FlushClient(false);
-}
-
-}  // namespace dawn::wire

src/dawn/wire/server/ObjectStorage.h

@@ -27,12 +27,6 @@
 
 namespace dawn::wire::server {
 
-struct DeviceInfo {
-    std::unordered_set<uint64_t> childObjectTypesAndIds;
-    Server* server;
-    ObjectHandle self;
-};
-
 // Whether this object has been allocated, or reserved for async object creation.
 // Used by the KnownObjects queries
 enum class AllocationState : uint32_t {
@@ -48,9 +42,6 @@ struct ObjectDataBase {
     uint32_t generation = 0;
 
     AllocationState state;
-
-    // This points to an allocation that is owned by the device.
-    DeviceInfo* deviceInfo = nullptr;
 };
 
 // Stores what the backend knows about the type.
@@ -70,28 +61,21 @@ struct ObjectData<WGPUBuffer> : public ObjectDataBase<WGPUBuffer> {
     bool mappedAtCreation = false;
 };
 
-// Pack the ObjectType and ObjectId as a single value for storage in
+struct DeviceInfo {
-// an std::unordered_set. This lets us avoid providing our own hash and
+    Server* server;
-// equality comparison operators.
+    ObjectHandle self;
-inline uint64_t PackObjectTypeAndId(ObjectType type, ObjectId id) {
+};
-    static_assert(sizeof(ObjectType) * 8 <= 32);
-    static_assert(sizeof(ObjectId) * 8 <= 32);
-    return (static_cast<uint64_t>(type) << 32) + id;
-}
-
-inline std::pair<ObjectType, ObjectId> UnpackObjectTypeAndId(uint64_t payload) {
-    ObjectType type = static_cast<ObjectType>(payload >> 32);
-    ObjectId id = payload & 0xFFFFFFFF;
-    return std::make_pair(type, id);
-}
 
 template <>
 struct ObjectData<WGPUDevice> : public ObjectDataBase<WGPUDevice> {
     // Store |info| as a separate allocation so that its address does not move.
-    // The pointer to |info| is stored in device child objects.
+    // The pointer to |info| is used as the userdata to device callback.
     std::unique_ptr<DeviceInfo> info = std::make_unique<DeviceInfo>();
 };
 
+template <>
+struct ObjectData<WGPUQueue> : public ObjectDataBase<WGPUQueue> {};
+
 // Keeps track of the mapping between client IDs and backend objects.
 template <typename T>
 class KnownObjectsBase {

src/dawn/wire/server/Server.cpp

@@ -62,11 +62,6 @@ bool Server::InjectTexture(WGPUTexture texture,
     data->handle = texture;
     data->generation = generation;
     data->state = AllocationState::Allocated;
-    data->deviceInfo = device->info.get();
-
-    if (!TrackDeviceChild(data->deviceInfo, ObjectType::Texture, id)) {
-        return false;
-    }
 
     // The texture is externally owned so it shouldn't be destroyed when we receive a destroy
     // message from the client. Add a reference to counterbalance the eventual release.
@@ -94,11 +89,6 @@ bool Server::InjectSwapChain(WGPUSwapChain swapchain,
     data->handle = swapchain;
     data->generation = generation;
     data->state = AllocationState::Allocated;
-    data->deviceInfo = device->info.get();
-
-    if (!TrackDeviceChild(data->deviceInfo, ObjectType::SwapChain, id)) {
-        return false;
-    }
 
     // The texture is externally owned so it shouldn't be destroyed when we receive a destroy
     // message from the client. Add a reference to counterbalance the eventual release.
@@ -164,6 +154,9 @@ void Server::SetForwardingDeviceCallbacks(ObjectData<WGPUDevice>* deviceObject)
     // free their userdata. Also unlike other callbacks, these are cleared and unset when
     // the server is destroyed, so we don't need to check if the server is still alive
     // inside them.
+    // Also, the device is special-cased in Server::DoDestroyObject to call
+    // ClearDeviceCallbacks. This ensures that callbacks will not fire after |deviceObject|
+    // is freed.
     mProcs.deviceSetUncapturedErrorCallback(
         deviceObject->handle,
         [](WGPUErrorType type, const char* message, void* userdata) {
@@ -197,24 +190,4 @@ void Server::ClearDeviceCallbacks(WGPUDevice device) {
     mProcs.deviceSetDeviceLostCallback(device, nullptr, nullptr);
 }
 
-bool TrackDeviceChild(DeviceInfo* info, ObjectType type, ObjectId id) {
-    auto [_, inserted] = info->childObjectTypesAndIds.insert(PackObjectTypeAndId(type, id));
-    if (!inserted) {
-        // An object of this type and id already exists.
-        return false;
-    }
-    return true;
-}
-
-bool UntrackDeviceChild(DeviceInfo* info, ObjectType type, ObjectId id) {
-    auto& children = info->childObjectTypesAndIds;
-    auto it = children.find(PackObjectTypeAndId(type, id));
-    if (it == children.end()) {
-        // An object of this type and id was already deleted.
-        return false;
-    }
-    children.erase(it);
-    return true;
-}
-
 }  // namespace dawn::wire::server

src/dawn/wire/server/Server.h

@@ -234,9 +234,6 @@ class Server : public ServerBase {
     std::shared_ptr<bool> mIsAlive;
 };
 
-bool TrackDeviceChild(DeviceInfo* device, ObjectType type, ObjectId id);
-bool UntrackDeviceChild(DeviceInfo* device, ObjectType type, ObjectId id);
-
 std::unique_ptr<MemoryTransferService> CreateInlineMemoryTransferService();
 
 }  // namespace dawn::wire::server

src/dawn/wire/server/ServerBuffer.cpp

@@ -116,12 +116,8 @@ bool Server::DoDeviceCreateBuffer(ObjectId deviceId,
     }
     resultData->generation = bufferResult.generation;
     resultData->handle = mProcs.deviceCreateBuffer(device->handle, descriptor);
-    resultData->deviceInfo = device->info.get();
     resultData->usage = descriptor->usage;
     resultData->mappedAtCreation = descriptor->mappedAtCreation;
-    if (!TrackDeviceChild(resultData->deviceInfo, ObjectType::Buffer, bufferResult.id)) {
-        return false;
-    }
 
     // isReadMode and isWriteMode could be true at the same time if usage contains
     // WGPUMapMode_Read and buffer is mappedAtCreation

src/dawn/wire/server/ServerDevice.cpp

@@ -26,12 +26,6 @@ void HandleCreatePipelineAsyncCallback(KnownObjects<Pipeline>* knownObjects,
     if (status == WGPUCreatePipelineAsyncStatus_Success) {
         auto* pipelineObject = knownObjects->FillReservation(data->pipelineObjectID, pipeline);
         ASSERT(pipelineObject != nullptr);
-
-        // This should be impossible to fail. It would require a command to be sent that
-        // creates a duplicate ObjectId, which would fail validation.
-        bool success =
-            TrackDeviceChild(pipelineObject->deviceInfo, objectType, data->pipelineObjectID);
-        ASSERT(success);
     } else {
         // Otherwise, free the ObjectId which will make it unusable.
         knownObjects->Free(data->pipelineObjectID);
@@ -111,7 +105,6 @@ bool Server::DoDeviceCreateComputePipelineAsync(ObjectId deviceId,
     }
 
     resultData->generation = pipelineObjectHandle.generation;
-    resultData->deviceInfo = device->info.get();
 
     auto userdata = MakeUserdata<CreatePipelineAsyncUserData>();
     userdata->device = ObjectHandle{deviceId, device->generation};
@@ -156,7 +149,6 @@ bool Server::DoDeviceCreateRenderPipelineAsync(ObjectId deviceId,
     }
 
     resultData->generation = pipelineObjectHandle.generation;
-    resultData->deviceInfo = device->info.get();
 
     auto userdata = MakeUserdata<CreatePipelineAsyncUserData>();
     userdata->device = ObjectHandle{deviceId, device->generation};

src/dawn/wire/server/ServerQueue.cpp

@@ -59,13 +59,8 @@ bool Server::DoQueueWriteBuffer(ObjectId queueId,
     }
 
     if (size > std::numeric_limits<size_t>::max()) {
-        auto* device = DeviceObjects().Get(queue->deviceInfo->self.id);
-        if (device == nullptr) {
         return false;
     }
-        return DoDeviceInjectError(reinterpret_cast<WGPUDevice>(device), WGPUErrorType_OutOfMemory,
-                                   "Data size too large for write texture.");
-    }
 
     mProcs.queueWriteBuffer(queue->handle, buffer->handle, bufferOffset, data,
                             static_cast<size_t>(size));
@@ -86,13 +81,8 @@ bool Server::DoQueueWriteTexture(ObjectId queueId,
     }
 
     if (dataSize > std::numeric_limits<size_t>::max()) {
-        auto* device = DeviceObjects().Get(queue->deviceInfo->self.id);
-        if (device == nullptr) {
         return false;
     }
-        return DoDeviceInjectError(reinterpret_cast<WGPUDevice>(device), WGPUErrorType_OutOfMemory,
-                                   "Data size too large for write texture.");
-    }
 
     mProcs.queueWriteTexture(queue->handle, destination, data, static_cast<size_t>(dataSize),
                              dataLayout, writeSize);