Commit 45b2af30 authored by Corentin Wallez's avatar Corentin Wallez Committed by Commit Bot

WebGPUDecoder: Fix using multiple SharedImages at the same time.

The issue when using multiple shared images was that a `ScopedAccess`
was destroyed after it's `SharedImageRepresentation`.
The code was similar to the following:

  struct Pair {
      unique_ptr<Representation> representation;
      unique_ptr<Access> access;
  };

  base::flat_map<Key, Pair> map;
  map.erase(some_iterator);

In the Pair destructor C++ guarantees that `access` is destroyed before
`representation` but `erase` can move one element over another, causing
the move-assignment operator to be called. In this case the defaulted
move-assignment would first move `representation` then `access`. Causing
incorrect member destruction order for the move-to object.

Thanks to senorblanco@ and amaiorano@ for figuring out this wasn't a
compiler bug by jumping in the flat_map internals!

Bug: chromium:1038210
Change-Id: Ida90804a9840d1acba96a37c7cad17cf6bcc46ea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2014927Reviewed-by: default avatarGeoff Lang <geofflang@chromium.org>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734956}
parent 86dcd0d4
...@@ -436,7 +436,7 @@ class WebGPUDecoderImpl final : public WebGPUDecoder { ...@@ -436,7 +436,7 @@ class WebGPUDecoderImpl final : public WebGPUDecoder {
// Map from the <ID, generation> pair for a wire texture to the shared image // Map from the <ID, generation> pair for a wire texture to the shared image
// representation and access for it. // representation and access for it.
base::flat_map<std::tuple<uint32_t, uint32_t>, base::flat_map<std::tuple<uint32_t, uint32_t>,
SharedImageRepresentationAndAccess> std::unique_ptr<SharedImageRepresentationAndAccess>>
associated_shared_image_map_; associated_shared_image_map_;
std::unique_ptr<dawn_platform::Platform> dawn_platform_; std::unique_ptr<dawn_platform::Platform> dawn_platform_;
...@@ -855,11 +855,15 @@ error::Error WebGPUDecoderImpl::HandleAssociateMailboxImmediate( ...@@ -855,11 +855,15 @@ error::Error WebGPUDecoderImpl::HandleAssociateMailboxImmediate(
return error::kInvalidArguments; return error::kInvalidArguments;
} }
std::unique_ptr<SharedImageRepresentationAndAccess>
representation_and_access =
std::make_unique<SharedImageRepresentationAndAccess>();
representation_and_access->representation = std::move(shared_image);
representation_and_access->access = std::move(shared_image_access);
std::tuple<uint32_t, uint32_t> id_and_generation{id, generation}; std::tuple<uint32_t, uint32_t> id_and_generation{id, generation};
SharedImageRepresentationAndAccess shared_image_representation_and_access{
std::move(shared_image), std::move(shared_image_access)};
auto insertion = associated_shared_image_map_.emplace( auto insertion = associated_shared_image_map_.emplace(
id_and_generation, std::move(shared_image_representation_and_access)); id_and_generation, std::move(representation_and_access));
// InjectTexture already validated that the (ID, generation) can't have been // InjectTexture already validated that the (ID, generation) can't have been
// registered before. // registered before.
......
...@@ -235,4 +235,67 @@ TEST_F(WebGPUMailboxTest, ErrorWhenUsingTextureAfterDissociate) { ...@@ -235,4 +235,67 @@ TEST_F(WebGPUMailboxTest, ErrorWhenUsingTextureAfterDissociate) {
WaitForCompletion(device); WaitForCompletion(device);
} }
// This is a regression test for an issue when using multiple shared images
// where a `ScopedAccess` was destroyed after it's `SharedImageRepresentation`.
// The code was similar to the following.
//
// struct Pair {
// unique_ptr<Representation> representation;
// unique_ptr<Access> access;
// };
//
// base::flat_map<Key, Pair> map;
// map.erase(some_iterator);
//
// In the Pair destructor C++ guarantees that `access` is destroyed before
// `representation` but `erase` can move one element over another, causing
// the move-assignment operator to be called. In this case the defaulted
// move-assignment would first move `representation` then `access`. Causing
// incorrect member destruction order for the move-to object.
TEST_F(WebGPUMailboxTest, UseA_UseB_DestroyA_DestroyB) {
if (!WebGPUSupported()) {
LOG(ERROR) << "Test skipped because WebGPU isn't supported";
return;
}
if (!WebGPUSharedImageSupported()) {
LOG(ERROR) << "Test skipped because WebGPUSharedImage isn't supported";
return;
}
// Create a the shared images.
SharedImageInterface* sii = GetSharedImageInterface();
Mailbox mailboxA = sii->CreateSharedImage(
viz::ResourceFormat::RGBA_8888, {1, 1}, gfx::ColorSpace::CreateSRGB(),
SHARED_IMAGE_USAGE_WEBGPU);
Mailbox mailboxB = sii->CreateSharedImage(
viz::ResourceFormat::RGBA_8888, {1, 1}, gfx::ColorSpace::CreateSRGB(),
SHARED_IMAGE_USAGE_WEBGPU);
// Get a WebGPU device to associate the shared images to.
constexpr uint32_t kAdapterID = 0;
webgpu()->RequestDeviceAsync(kAdapterID, nullptr,
base::BindOnce(&OnRequestDeviceCallback));
wgpu::Device device = wgpu::Device::Acquire(webgpu()->GetDefaultDevice());
// Associate both mailboxes
gpu::webgpu::ReservedTexture reservationA =
webgpu()->ReserveTexture(device.Get());
webgpu()->AssociateMailbox(0, 0, reservationA.id, reservationA.generation,
WGPUTextureUsage_OutputAttachment,
reinterpret_cast<GLbyte*>(&mailboxA));
gpu::webgpu::ReservedTexture reservationB =
webgpu()->ReserveTexture(device.Get());
webgpu()->AssociateMailbox(0, 0, reservationB.id, reservationB.generation,
WGPUTextureUsage_OutputAttachment,
reinterpret_cast<GLbyte*>(&mailboxB));
// Dissociate both mailboxes in the same order.
webgpu()->DissociateMailbox(reservationA.id, reservationA.generation);
webgpu()->DissociateMailbox(reservationB.id, reservationB.generation);
// Send all the previous commands to the WebGPU decoder.
webgpu()->FlushCommands();
}
} // namespace gpu } // namespace gpu
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment