Ensure all wire child objects are destroyed before their device
Destroying a device will implicit destroy all its child objects.
Attempting to use a child object after results in a fatal error.
Bug: dawn:384
Change-Id: I43c27c92cacde759be83cca79ac890f41bac3927
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/37002
Commit-Queue: Austin Eng <enga@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Jiawei Shao <jiawei.shao@intel.com>
diff --git a/src/dawn_wire/WireServer.cpp b/src/dawn_wire/WireServer.cpp
index 0056d5b..723f691 100644
--- a/src/dawn_wire/WireServer.cpp
+++ b/src/dawn_wire/WireServer.cpp
@@ -32,8 +32,12 @@
return mImpl->HandleCommands(commands, size);
}
- bool WireServer::InjectTexture(WGPUTexture texture, uint32_t id, uint32_t generation) {
- return mImpl->InjectTexture(texture, id, generation);
+ bool WireServer::InjectTexture(WGPUTexture texture,
+ uint32_t id,
+ uint32_t generation,
+ uint32_t deviceId,
+ uint32_t deviceGeneration) {
+ return mImpl->InjectTexture(texture, id, generation, deviceId, deviceGeneration);
}
namespace server {
diff --git a/src/dawn_wire/client/Buffer.cpp b/src/dawn_wire/client/Buffer.cpp
index 2244ace..40b5759 100644
--- a/src/dawn_wire/client/Buffer.cpp
+++ b/src/dawn_wire/client/Buffer.cpp
@@ -66,7 +66,7 @@
buffer->mSize = descriptor->size;
DeviceCreateBufferCmd cmd;
- cmd.device = ToAPI(device);
+ cmd.deviceId = device->id;
cmd.descriptor = descriptor;
cmd.result = ObjectHandle{buffer->id, bufferObjectAndSerial->generation};
cmd.handleCreateInfoLength = writeHandleCreateInfoLength;
diff --git a/src/dawn_wire/client/Client.cpp b/src/dawn_wire/client/Client.cpp
index 51af5a6..0ca5f61 100644
--- a/src/dawn_wire/client/Client.cpp
+++ b/src/dawn_wire/client/Client.cpp
@@ -59,6 +59,9 @@
void Client::DestroyAllObjects() {
for (auto& objectList : mObjects) {
ObjectType objectType = static_cast<ObjectType>(&objectList - mObjects.data());
+ if (objectType == ObjectType::Device) {
+ continue;
+ }
while (!objectList.empty()) {
ObjectBase* object = objectList.head()->value();
@@ -69,6 +72,16 @@
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);
+ }
}
WGPUDevice Client::GetDevice() {
@@ -85,6 +98,8 @@
result.texture = ToAPI(allocation->object.get());
result.id = allocation->object->id;
result.generation = allocation->generation;
+ result.deviceId = FromAPI(device)->id;
+ result.deviceGeneration = DeviceAllocator().GetGeneration(FromAPI(device)->id);
return result;
}
diff --git a/src/dawn_wire/client/Device.cpp b/src/dawn_wire/client/Device.cpp
index af4cbfb..08a3e33 100644
--- a/src/dawn_wire/client/Device.cpp
+++ b/src/dawn_wire/client/Device.cpp
@@ -215,7 +215,7 @@
}
DeviceCreateReadyComputePipelineCmd cmd;
- cmd.device = ToAPI(this);
+ cmd.deviceId = this->id;
cmd.descriptor = descriptor;
uint64_t serial = mCreateReadyPipelineRequestSerial++;
@@ -270,7 +270,7 @@
"GPU device disconnected", userdata);
}
DeviceCreateReadyRenderPipelineCmd cmd;
- cmd.device = ToAPI(this);
+ cmd.deviceId = this->id;
cmd.descriptor = descriptor;
uint64_t serial = mCreateReadyPipelineRequestSerial++;
diff --git a/src/dawn_wire/server/ObjectStorage.h b/src/dawn_wire/server/ObjectStorage.h
index 9963158..74cc5a7 100644
--- a/src/dawn_wire/server/ObjectStorage.h
+++ b/src/dawn_wire/server/ObjectStorage.h
@@ -20,6 +20,7 @@
#include <algorithm>
#include <map>
+#include <unordered_set>
namespace dawn_wire { namespace server {
@@ -32,6 +33,8 @@
// Whether this object has been allocated, used by the KnownObjects queries
// TODO(cwallez@chromium.org): make this an internal bit vector in KnownObjects.
bool allocated;
+
+ ObjectDataBase<WGPUDevice>* device = nullptr;
};
// Stores what the backend knows about the type.
@@ -48,6 +51,26 @@
BufferMapWriteState mapWriteState = BufferMapWriteState::Unmapped;
};
+ // Pack the ObjectType and ObjectId as a single value for storage in
+ // an std::unordered_set. This lets us avoid providing our own hash and
+ // equality comparison operators.
+ 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> {
+ std::unordered_set<uint64_t> childObjectTypesAndIds;
+ };
+
// Keeps track of the mapping between client IDs and backend objects.
template <typename T>
class KnownObjects {
diff --git a/src/dawn_wire/server/Server.cpp b/src/dawn_wire/server/Server.cpp
index bb5644b..67a9dd5 100644
--- a/src/dawn_wire/server/Server.cpp
+++ b/src/dawn_wire/server/Server.cpp
@@ -66,15 +66,29 @@
DestroyAllObjects(mProcs);
}
- bool Server::InjectTexture(WGPUTexture texture, uint32_t id, uint32_t generation) {
+ bool Server::InjectTexture(WGPUTexture texture,
+ uint32_t id,
+ uint32_t generation,
+ uint32_t deviceId,
+ uint32_t deviceGeneration) {
+ ObjectData<WGPUDevice>* device = DeviceObjects().Get(deviceId);
+ if (device == nullptr || device->generation != deviceGeneration) {
+ return false;
+ }
+
ObjectData<WGPUTexture>* data = TextureObjects().Allocate(id);
if (data == nullptr) {
return false;
}
+ if (!TrackDeviceChild(device, ObjectType::Texture, id)) {
+ return false;
+ }
+
data->handle = texture;
data->generation = generation;
data->allocated = true;
+ data->device = device;
// 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.
@@ -83,4 +97,25 @@
return true;
}
+ bool TrackDeviceChild(ObjectDataBase<WGPUDevice>* device, ObjectType type, ObjectId id) {
+ auto it = static_cast<ObjectData<WGPUDevice>*>(device)->childObjectTypesAndIds.insert(
+ PackObjectTypeAndId(type, id));
+ if (!it.second) {
+ // An object of this type and id already exists.
+ return false;
+ }
+ return true;
+ }
+
+ bool UntrackDeviceChild(ObjectDataBase<WGPUDevice>* device, ObjectType type, ObjectId id) {
+ auto& children = static_cast<ObjectData<WGPUDevice>*>(device)->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
diff --git a/src/dawn_wire/server/Server.h b/src/dawn_wire/server/Server.h
index 867f29b..f45ed0d 100644
--- a/src/dawn_wire/server/Server.h
+++ b/src/dawn_wire/server/Server.h
@@ -161,7 +161,11 @@
const volatile char* HandleCommandsImpl(const volatile char* commands,
size_t size) override;
- bool InjectTexture(WGPUTexture texture, uint32_t id, uint32_t generation);
+ bool InjectTexture(WGPUTexture texture,
+ uint32_t id,
+ uint32_t generation,
+ uint32_t deviceId,
+ uint32_t deviceGeneration);
template <typename T,
typename Enable = std::enable_if<std::is_base_of<CallbackUserdata, T>::value>>
@@ -215,6 +219,9 @@
std::shared_ptr<bool> mIsAlive;
};
+ bool TrackDeviceChild(ObjectDataBase<WGPUDevice>* device, ObjectType type, ObjectId id);
+ bool UntrackDeviceChild(ObjectDataBase<WGPUDevice>* device, ObjectType type, ObjectId id);
+
std::unique_ptr<MemoryTransferService> CreateInlineMemoryTransferService();
}} // namespace dawn_wire::server
diff --git a/src/dawn_wire/server/ServerBuffer.cpp b/src/dawn_wire/server/ServerBuffer.cpp
index e842bb1..fdc850b 100644
--- a/src/dawn_wire/server/ServerBuffer.cpp
+++ b/src/dawn_wire/server/ServerBuffer.cpp
@@ -120,18 +120,27 @@
return true;
}
- bool Server::DoDeviceCreateBuffer(WGPUDevice device,
+ bool Server::DoDeviceCreateBuffer(ObjectId deviceId,
const WGPUBufferDescriptor* descriptor,
ObjectHandle bufferResult,
uint64_t handleCreateInfoLength,
const uint8_t* handleCreateInfo) {
+ auto* device = DeviceObjects().Get(deviceId);
+ if (device == nullptr) {
+ return false;
+ }
+
// Create and register the buffer object.
auto* resultData = BufferObjects().Allocate(bufferResult.id);
if (resultData == nullptr) {
return false;
}
resultData->generation = bufferResult.generation;
- resultData->handle = mProcs.deviceCreateBuffer(device, descriptor);
+ resultData->handle = mProcs.deviceCreateBuffer(device->handle, descriptor);
+ resultData->device = device;
+ if (!TrackDeviceChild(device, ObjectType::Buffer, bufferResult.id)) {
+ return false;
+ }
// If the buffer isn't mapped at creation, we are done.
if (!descriptor->mappedAtCreation) {
diff --git a/src/dawn_wire/server/ServerDevice.cpp b/src/dawn_wire/server/ServerDevice.cpp
index cd2eba7..8884dd1 100644
--- a/src/dawn_wire/server/ServerDevice.cpp
+++ b/src/dawn_wire/server/ServerDevice.cpp
@@ -59,23 +59,32 @@
}
bool Server::DoDeviceCreateReadyComputePipeline(
- WGPUDevice cDevice,
+ ObjectId deviceId,
uint64_t requestSerial,
ObjectHandle pipelineObjectHandle,
const WGPUComputePipelineDescriptor* descriptor) {
+ auto* device = DeviceObjects().Get(deviceId);
+ if (device == nullptr) {
+ return false;
+ }
+
auto* resultData = ComputePipelineObjects().Allocate(pipelineObjectHandle.id);
if (resultData == nullptr) {
return false;
}
resultData->generation = pipelineObjectHandle.generation;
+ resultData->device = device;
+ if (!TrackDeviceChild(device, ObjectType::ComputePipeline, pipelineObjectHandle.id)) {
+ return false;
+ }
auto userdata = MakeUserdata<CreateReadyPipelineUserData>();
userdata->requestSerial = requestSerial;
userdata->pipelineObjectID = pipelineObjectHandle.id;
mProcs.deviceCreateReadyComputePipeline(
- cDevice, descriptor,
+ device->handle, descriptor,
ForwardToServer<decltype(&Server::OnCreateReadyComputePipelineCallback)>::Func<
&Server::OnCreateReadyComputePipelineCallback>(),
userdata.release());
@@ -116,23 +125,32 @@
SerializeCommand(cmd);
}
- bool Server::DoDeviceCreateReadyRenderPipeline(WGPUDevice cDevice,
+ bool Server::DoDeviceCreateReadyRenderPipeline(ObjectId deviceId,
uint64_t requestSerial,
ObjectHandle pipelineObjectHandle,
const WGPURenderPipelineDescriptor* descriptor) {
+ auto* device = DeviceObjects().Get(deviceId);
+ if (device == nullptr) {
+ return false;
+ }
+
auto* resultData = RenderPipelineObjects().Allocate(pipelineObjectHandle.id);
if (resultData == nullptr) {
return false;
}
resultData->generation = pipelineObjectHandle.generation;
+ resultData->device = device;
+ if (!TrackDeviceChild(device, ObjectType::RenderPipeline, pipelineObjectHandle.id)) {
+ return false;
+ }
auto userdata = MakeUserdata<CreateReadyPipelineUserData>();
userdata->requestSerial = requestSerial;
userdata->pipelineObjectID = pipelineObjectHandle.id;
mProcs.deviceCreateReadyRenderPipeline(
- cDevice, descriptor,
+ device->handle, descriptor,
ForwardToServer<decltype(&Server::OnCreateReadyRenderPipelineCallback)>::Func<
&Server::OnCreateReadyRenderPipelineCallback>(),
userdata.release());
diff --git a/src/include/dawn_wire/WireClient.h b/src/include/dawn_wire/WireClient.h
index 1a22fe5..8af02a97 100644
--- a/src/include/dawn_wire/WireClient.h
+++ b/src/include/dawn_wire/WireClient.h
@@ -34,6 +34,8 @@
WGPUTexture texture;
uint32_t id;
uint32_t generation;
+ uint32_t deviceId;
+ uint32_t deviceGeneration;
};
struct DAWN_WIRE_EXPORT WireClientDescriptor {
diff --git a/src/include/dawn_wire/WireServer.h b/src/include/dawn_wire/WireServer.h
index 7627ab8..ad36f44 100644
--- a/src/include/dawn_wire/WireServer.h
+++ b/src/include/dawn_wire/WireServer.h
@@ -43,7 +43,12 @@
const volatile char* HandleCommands(const volatile char* commands,
size_t size) override final;
- bool InjectTexture(WGPUTexture texture, uint32_t id, uint32_t generation);
+ // TODO(enga): Remove defaults after updating Chrome.
+ bool InjectTexture(WGPUTexture texture,
+ uint32_t id,
+ uint32_t generation,
+ uint32_t deviceId = 1,
+ uint32_t deviceGeneration = 0);
private:
std::unique_ptr<server::Server> mImpl;
diff --git a/src/tests/BUILD.gn b/src/tests/BUILD.gn
index 8fec31f..1c78506 100644
--- a/src/tests/BUILD.gn
+++ b/src/tests/BUILD.gn
@@ -218,6 +218,7 @@
"unittests/wire/WireBasicTests.cpp",
"unittests/wire/WireBufferMappingTests.cpp",
"unittests/wire/WireCreateReadyPipelineTests.cpp",
+ "unittests/wire/WireDestroyObjectTests.cpp",
"unittests/wire/WireDisconnectTests.cpp",
"unittests/wire/WireErrorCallbackTests.cpp",
"unittests/wire/WireExtensionTests.cpp",
diff --git a/src/tests/unittests/wire/WireDestroyObjectTests.cpp b/src/tests/unittests/wire/WireDestroyObjectTests.cpp
new file mode 100644
index 0000000..f5e16b7
--- /dev/null
+++ b/src/tests/unittests/wire/WireDestroyObjectTests.cpp
@@ -0,0 +1,45 @@
+// 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 "tests/unittests/wire/WireTest.h"
+
+using namespace testing;
+using namespace dawn_wire;
+
+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, DeviceRelease(apiDevice)).InSequence(s1, s2);
+
+ FlushClient();
+
+ // Using the command encoder should be an error.
+ wgpuCommandEncoderFinish(encoder, nullptr);
+ FlushClient(false);
+}
diff --git a/src/tests/unittests/wire/WireDisconnectTests.cpp b/src/tests/unittests/wire/WireDisconnectTests.cpp
index 4c99ced..f44df13 100644
--- a/src/tests/unittests/wire/WireDisconnectTests.cpp
+++ b/src/tests/unittests/wire/WireDisconnectTests.cpp
@@ -145,9 +145,10 @@
DeleteClient();
// Expect release on all objects created by the client.
- EXPECT_CALL(api, DeviceRelease(apiDevice)).Times(1);
- EXPECT_CALL(api, QueueRelease(apiQueue)).Times(1);
- EXPECT_CALL(api, CommandEncoderRelease(apiCommandEncoder)).Times(1);
- EXPECT_CALL(api, SamplerRelease(apiSampler)).Times(1);
+ 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, DeviceRelease(apiDevice)).Times(1).InSequence(s1, s2, s3);
FlushClient();
}
diff --git a/src/tests/unittests/wire/WireInjectTextureTests.cpp b/src/tests/unittests/wire/WireInjectTextureTests.cpp
index 9e327cf..c6a1f2c 100644
--- a/src/tests/unittests/wire/WireInjectTextureTests.cpp
+++ b/src/tests/unittests/wire/WireInjectTextureTests.cpp
@@ -34,7 +34,8 @@
WGPUTexture apiTexture = api.GetNewTexture();
EXPECT_CALL(api, TextureReference(apiTexture));
- ASSERT_TRUE(GetWireServer()->InjectTexture(apiTexture, reservation.id, reservation.generation));
+ ASSERT_TRUE(GetWireServer()->InjectTexture(apiTexture, reservation.id, reservation.generation,
+ reservation.deviceId, reservation.deviceGeneration));
wgpuTextureCreateView(reservation.texture, nullptr);
WGPUTextureView apiDummyView = api.GetNewTextureView();
@@ -57,11 +58,13 @@
WGPUTexture apiTexture = api.GetNewTexture();
EXPECT_CALL(api, TextureReference(apiTexture));
- ASSERT_TRUE(GetWireServer()->InjectTexture(apiTexture, reservation.id, reservation.generation));
+ ASSERT_TRUE(GetWireServer()->InjectTexture(apiTexture, reservation.id, reservation.generation,
+ reservation.deviceId, reservation.deviceGeneration));
// ID already in use, call fails.
- ASSERT_FALSE(
- GetWireServer()->InjectTexture(apiTexture, reservation.id, reservation.generation));
+ ASSERT_FALSE(GetWireServer()->InjectTexture(apiTexture, reservation.id, reservation.generation,
+ reservation.deviceId,
+ reservation.deviceGeneration));
}
// Test that the server only borrows the texture and does a single reference-release
@@ -71,7 +74,8 @@
// Injecting the texture adds a reference
WGPUTexture apiTexture = api.GetNewTexture();
EXPECT_CALL(api, TextureReference(apiTexture));
- ASSERT_TRUE(GetWireServer()->InjectTexture(apiTexture, reservation.id, reservation.generation));
+ ASSERT_TRUE(GetWireServer()->InjectTexture(apiTexture, reservation.id, reservation.generation,
+ reservation.deviceId, reservation.deviceGeneration));
// Releasing the texture removes a single reference.
wgpuTextureRelease(reservation.texture);
