Fix leak of wire client default queue

Previously, deleting the device in the wire implicitly released all
child objects. This is no longer the case, so a leak of the client
default queue caused the service-side queue to leak.

Fixed: chromium:1332926
Change-Id: I1efa02e79246f985e99e1bc814d87f292ddc22bd
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/92743
Reviewed-by: Jiawei Shao <jiawei.shao@intel.com>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Austin Eng <enga@chromium.org>

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

@@ -160,11 +160,10 @@ TEST_F(WireDisconnectTests, DeleteClientDestroysObjects) {
 
     DeleteClient();
 
-    // Expect release on all objects created by the client.
+    // Expect release on all objects created by the client. Note: the device
+    // should be deleted first because it may free its reference to the default queue
+    // on deletion.
     Sequence s1, s2, s3;
-    EXPECT_CALL(api, QueueRelease(apiQueue)).Times(1).InSequence(s1);
-    EXPECT_CALL(api, CommandEncoderRelease(apiCommandEncoder)).Times(1).InSequence(s2);
-    EXPECT_CALL(api, SamplerRelease(apiSampler)).Times(1).InSequence(s3);
     EXPECT_CALL(api, OnDeviceSetUncapturedErrorCallback(apiDevice, nullptr, nullptr))
         .Times(1)
         .InSequence(s1, s2);
@@ -175,6 +174,9 @@ TEST_F(WireDisconnectTests, DeleteClientDestroysObjects) {
         .Times(1)
         .InSequence(s1, s2);
     EXPECT_CALL(api, DeviceRelease(apiDevice)).Times(1).InSequence(s1, s2, s3);
+    EXPECT_CALL(api, QueueRelease(apiQueue)).Times(1).InSequence(s1);
+    EXPECT_CALL(api, CommandEncoderRelease(apiCommandEncoder)).Times(1).InSequence(s2);
+    EXPECT_CALL(api, SamplerRelease(apiSampler)).Times(1).InSequence(s3);
     FlushClient();
 
     // Signal that we already released and cleared callbacks for |apiDevice|

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

@@ -139,6 +139,60 @@ TEST_F(WireQueueTests, OnSubmittedWorkDoneInsideCallbackBeforeDisconnect) {
     GetWireClient()->Disconnect();
 }
 
+// Test releasing the default queue, then its device. Both should be
+// released when the device is released since the device holds a reference
+// to the queue. Regresssion test for crbug.com/1332926.
+TEST_F(WireQueueTests, DefaultQueueThenDeviceReleased) {
+    // Note: The test fixture gets the default queue.
+
+    // Release the queue which is the last external client reference.
+    // The device still holds a reference.
+    wgpuQueueRelease(queue);
+    FlushClient();
+
+    // Release the device which holds an internal reference to the queue.
+    // Now, the queue and device should be released on the server.
+    wgpuDeviceRelease(device);
+
+    EXPECT_CALL(api, QueueRelease(apiQueue));
+    EXPECT_CALL(api, DeviceRelease(apiDevice));
+    // These set X callback methods are called before the device is released.
+    EXPECT_CALL(api, OnDeviceSetUncapturedErrorCallback(apiDevice, nullptr, nullptr)).Times(1);
+    EXPECT_CALL(api, OnDeviceSetLoggingCallback(apiDevice, nullptr, nullptr)).Times(1);
+    EXPECT_CALL(api, OnDeviceSetDeviceLostCallback(apiDevice, nullptr, nullptr)).Times(1);
+    FlushClient();
+
+    // Indicate to the fixture that the device was already released.
+    DefaultApiDeviceWasReleased();
+}
+
+// Test the device, then its default queue. The default queue should be
+// released when its external reference is dropped since releasing the device
+// drops the internal reference. Regresssion test for crbug.com/1332926.
+TEST_F(WireQueueTests, DeviceThenDefaultQueueReleased) {
+    // Note: The test fixture gets the default queue.
+
+    // Release the device which holds an internal reference to the queue.
+    // Now, the should be released on the server, but not the queue since
+    // the default queue still has one external reference.
+    wgpuDeviceRelease(device);
+
+    EXPECT_CALL(api, DeviceRelease(apiDevice));
+    // These set X callback methods are called before the device is released.
+    EXPECT_CALL(api, OnDeviceSetUncapturedErrorCallback(apiDevice, nullptr, nullptr)).Times(1);
+    EXPECT_CALL(api, OnDeviceSetLoggingCallback(apiDevice, nullptr, nullptr)).Times(1);
+    EXPECT_CALL(api, OnDeviceSetDeviceLostCallback(apiDevice, nullptr, nullptr)).Times(1);
+    FlushClient();
+
+    // Release the external queue reference. The queue should be released.
+    wgpuQueueRelease(queue);
+    EXPECT_CALL(api, QueueRelease(apiQueue));
+    FlushClient();
+
+    // Indicate to the fixture that the device was already released.
+    DefaultApiDeviceWasReleased();
+}
+
 // Only one default queue is supported now so we cannot test ~Queue triggering ClearAllCallbacks
 // since it is always destructed after the test TearDown, and we cannot create a new queue obj
 // with wgpuDeviceGetQueue

src/dawn/wire/client/Client.cpp

@@ -51,6 +51,19 @@ Client::~Client() {
 }
 
 void Client::DestroyAllObjects() {
+    // Free all devices first since they may hold references to other objects
+    // like the default queue. The Device destructor releases the default queue,
+    // which would be invalid if the queue was already freed.
+    while (!mObjects[ObjectType::Device].empty()) {
+        ObjectBase* object = mObjects[ObjectType::Device].head()->value();
+
+        DestroyObjectCmd cmd;
+        cmd.objectType = ObjectType::Device;
+        cmd.objectId = object->id;
+        SerializeCommand(cmd);
+        FreeObject(ObjectType::Device, object);
+    }
+
     for (auto& objectList : mObjects) {
         ObjectType objectType = static_cast<ObjectType>(&objectList - mObjects.data());
         if (objectType == ObjectType::Device) {
@@ -66,16 +79,6 @@ void Client::DestroyAllObjects() {
             FreeObject(objectType, object);
         }
     }
-
-    while (!mObjects[ObjectType::Device].empty()) {
-        ObjectBase* object = mObjects[ObjectType::Device].head()->value();
-
-        DestroyObjectCmd cmd;
-        cmd.objectType = ObjectType::Device;
-        cmd.objectId = object->id;
-        SerializeCommand(cmd);
-        FreeObject(ObjectType::Device, object);
-    }
 }
 
 ReservedTexture Client::ReserveTexture(WGPUDevice device) {

src/dawn/wire/client/Device.cpp

@@ -67,6 +67,10 @@ Device::~Device() {
                 "Device destroyed before callback", request->userdata);
         }
     });
+
+    if (mQueue != nullptr) {
+        GetProcs().queueRelease(ToAPI(mQueue));
+    }
 }
 
 bool Device::GetLimits(WGPUSupportedLimits* limits) const {