fix(💥): detach swapchains before device destroy

generator/templates/dawn/native/ObjectType.cpp

@@ -44,6 +44,8 @@ namespace {{native_namespace}} {
             {% endfor %}
                 case ObjectType::BindGroupLayoutInternal:
                     return "BindGroupLayoutInternal";
+                case ObjectType::SwapChain:
+                    return "SwapChain";
             default:
                 DAWN_UNREACHABLE();
         }

generator/templates/dawn/native/ObjectType.h

@@ -44,9 +44,10 @@ namespace {{native_namespace}} {
 
         // Additional internal object types. Keep kExtraObjectTypes in sync when updating.
         BindGroupLayoutInternal,
+        SwapChain,
     };
 
-    constexpr size_t kExtraObjectTypes = 1;
+    constexpr size_t kExtraObjectTypes = 2;
 
     template <typename T>
     using PerObjectType = ityp::array<ObjectType, T, {{len(by_category["object"])}} + kExtraObjectTypes>;

src/dawn/native/Device.cpp

@@ -510,7 +510,7 @@ void DeviceBase::DestroyObjects() {
     // can destroy the frontend cache.
 
     // clang-format off
-    static constexpr std::array<ObjectType, 21> kObjectTypeDependencyOrder = {
+    static constexpr std::array<ObjectType, 22> kObjectTypeDependencyOrder = {
         // Encoders first, nothing refers to them.
         ObjectType::ComputePassEncoder,
         ObjectType::RenderPassEncoder,
@@ -533,7 +533,8 @@ void DeviceBase::DestroyObjects() {
         ObjectType::SharedTextureMemory,
         ObjectType::SharedFence,
         ObjectType::ExternalTexture,
-        ObjectType::Texture,  // Note that Textures own the TextureViews.
+        ObjectType::SwapChain,  // Note that SwapChains own their current Texture.
+        ObjectType::Texture,    // Note that Textures own the TextureViews.
         ObjectType::QuerySet,
         ObjectType::Sampler,
         ObjectType::Buffer,

src/dawn/native/Surface.cpp

@@ -513,14 +513,34 @@ MaybeError Surface::Configure(const SurfaceConfiguration* configIn) {
     return {};
 }
 
+void Surface::DetachSwapChain(SwapChainBase* swapChain) {
+    DAWN_ASSERT(swapChain != nullptr);
+    DAWN_ASSERT(swapChain->GetSurface() == this);
+
+    if (mSwapChain.Get() == swapChain) {
+        swapChain->DetachFromSurface();
+        mSwapChain = nullptr;
+        // A failed Configure() with another device leaves the previous swapchain attached, in
+        // which case the surface stays configured with that other device.
+        if (mCurrentDevice.Get() == swapChain->GetDevice()) {
+            mCurrentDevice = nullptr;
+        }
+    } else if (mRecycledSwapChain.Get() == swapChain) {
+        swapChain->DetachFromSurface();
+        mRecycledSwapChain = nullptr;
+    } else {
+        DAWN_UNREACHABLE();
+    }
+}
+
 MaybeError Surface::Unconfigure() {
     if (IsError()) {
         DAWN_CHECK(mSwapChain == nullptr);
         DAWN_CHECK(mCurrentDevice == nullptr);
         return DAWN_VALIDATION_ERROR("%s is invalid.", this);
     }
+    // Unconfiguring an unconfigured surface is a no-op.
     mCurrentDevice = nullptr;
-    DAWN_INVALID_IF(!mSwapChain.Get(), "%s is not configured.", this);
 
     if (mSwapChain != nullptr) {
         if (mRecycledSwapChain != nullptr) {

src/dawn/native/Surface.h

@@ -119,6 +119,12 @@ class Surface final : public ErrorMonad {
 
     const std::string& GetLabel() const;
 
+    // Called by `swapChain` when its device is destroyed while it is still attached to this
+    // surface. The swapchain is detached and the surface drops it. If it was the current
+    // swapchain the surface becomes unconfigured, as if Unconfigure() had been called, except
+    // that the swapchain cannot be recycled.
+    void DetachSwapChain(SwapChainBase* swapChain);
+
     // Dawn API
     void APIConfigure(const SurfaceConfiguration* config);
     wgpu::Status APIGetCapabilities(AdapterBase* adapter, SurfaceCapabilities* capabilities) const;

src/dawn/native/SwapChain.cpp

@@ -57,7 +57,7 @@ TextureDescriptor GetSwapChainBaseTextureDescriptor(SwapChainBase* swapChain) {
 SwapChainBase::SwapChainBase(DeviceBase* device,
                              Surface* surface,
                              const SurfaceConfiguration* config)
-    : mDevice(device),
+    : ApiObjectBase(device, kLabelNotImplemented),
       mWidth(config->width),
       mHeight(config->height),
       mFormat(config->format),
@@ -72,6 +72,7 @@ SwapChainBase::SwapChainBase(DeviceBase* device,
         }
         mViewFormats.push_back(viewFormat);
     }
+    GetObjectTrackingList()->Track(this);
 }
 
 FormatSet SwapChainBase::ComputeViewFormatSet() const {
@@ -90,6 +91,18 @@ SwapChainBase::~SwapChainBase() {
     DAWN_CHECK(!mAttached);
 }
 
+void SwapChainBase::DestroyImpl(DestroyReason reason) {
+    // The surface has a Ref on the swapchains attached to it and detaches them before dropping
+    // that Ref, so a swapchain that is being deleted is already detached. The swapchain can
+    // still be attached when the device is destroyed though: detach it so that its backend
+    // resources are released before the device's, and so that the surface stops using a
+    // swapchain of a destroyed device.
+    if (mAttached) {
+        mSurface->DetachSwapChain(this);
+    }
+    DAWN_ASSERT(!mAttached);
+}
+
 void SwapChainBase::DetachFromSurface() {
     if (mAttached) {
         DetachFromSurfaceImpl();
@@ -99,6 +112,7 @@ void SwapChainBase::DetachFromSurface() {
 }
 
 void SwapChainBase::SetIsAttached() {
+    DAWN_ASSERT(!mAttached);
     mAttached = true;
 }
 
@@ -143,9 +157,10 @@ MaybeError SwapChainBase::Present() {
     return {};
 }
 
-DeviceBase* SwapChainBase::GetDevice() const {
-    return mDevice.Get();
+ObjectType SwapChainBase::GetType() const {
+    return ObjectType::SwapChain;
 }
+
 uint32_t SwapChainBase::GetWidth() const {
     return mWidth;
 }

src/dawn/native/SwapChain.h

@@ -46,40 +46,28 @@ struct SwapChainTextureInfo {
     wgpu::SurfaceGetCurrentTextureStatus status = wgpu::SurfaceGetCurrentTextureStatus::Error;
 };
 
-class SwapChainBase : public RefCounted {
+class SwapChainBase : public ApiObjectBase {
   public:
     SwapChainBase(DeviceBase* device, Surface* surface, const SurfaceConfiguration* config);
 
     // This is called when the swapchain is detached when one of the following happens:
     //
     //  - The surface it is attached to is being destroyed.
     //  - The swapchain is being replaced by another one on the surface.
+    //  - The device that created it is being destroyed (see DestroyImpl).
     //
     // Note that the surface has a Ref on the last swapchain that was used on it so the
     // SwapChain destructor will only be called after one of the things above happens.
     //
     // The call for the detaching previous swapchain should be called inside the backend
     // implementation of SwapChains. This is to allow them to acquire any resources before
     // calling detach to make a seamless transition from the previous swapchain.
-    //
-    // Likewise the call for the swapchain being destroyed must be done in the backend's
-    // swapchain's destructor since C++ says it is UB to call virtual methods in the base class
-    // destructor.
     void DetachFromSurface();
 
     void SetIsAttached();
 
-    // TODO(crbug.com/dawn/831):
-    // APIRelease() can be called without any synchronization guarantees so we need to use a Release
-    // method that will call LockAndDeleteThis() on destruction.
-    // This is because losing the last reference to the SwapChain will detach its surface which
-    // explicitly destroys the current texture. Explicit destruction of textures is not thread safe
-    // yet.
-    // Hot path and intended to be shadowed.
-    // NOLINTNEXTLINE(bugprone-derived-method-shadowing-base-method)
-    void APIRelease() { ReleaseAndLockBeforeDestroy(); }
-
-    DeviceBase* GetDevice() const;
+    ObjectType GetType() const override;
+
     uint32_t GetWidth() const;
     uint32_t GetHeight() const;
     wgpu::TextureFormat GetFormat() const;
@@ -99,13 +87,15 @@ class SwapChainBase : public RefCounted {
     ~SwapChainBase() override;
 
   private:
+    // Detaches the swapchain from its surface if it is still attached, which is the case when the
+    // device is destroyed while the surface is still configured with it.
+    void DestroyImpl(DestroyReason reason) override;
+
     void SetChildLabel(ApiObjectBase* child) const;
     // Get a format set from mViewFormats (equivalent information, but easier to validate the
     // current texture)
     FormatSet ComputeViewFormatSet() const;
 
-    Ref<DeviceBase> mDevice;
-
     bool mAttached = false;
     uint32_t mWidth = 0;
     uint32_t mHeight = 0;

src/dawn/tests/end2end/SurfaceTests.cpp

@@ -574,6 +574,146 @@ TEST_P(SurfaceTests, PresentWithoutGet) {
     ASSERT_EQ(wgpu::Status::Success, presentStatus);
 }
 
+// Releasing a configured surface after its device was destroyed must not crash. Regression test
+// for the Vulkan backend dereferencing the destroyed device's FencedDeleter when the swapchain was
+// detached by the surface destructor.
+TEST_P(SurfaceTests, ReleaseSurfaceAfterDeviceDestroy) {
+    wgpu::Surface surface = CreateTestSurface();
+    wgpu::SurfaceConfiguration config = GetPreferredConfiguration(surface);
+    surface.Configure(&config);
+
+    wgpu::SurfaceTexture surfaceTexture;
+    surface.GetCurrentTexture(&surfaceTexture);
+    ClearTexture(surfaceTexture.texture, {1.0, 0.0, 0.0, 1.0});
+    ASSERT_EQ(wgpu::Status::Success, surface.Present());
+
+    DestroyDevice();
+
+    // The surface is released at the end of the test body, after the device was destroyed.
+}
+
+// Same as ReleaseSurfaceAfterDeviceDestroy but with the swapchain parked as the surface's recycled
+// swapchain by Unconfigure() instead of being the current one.
+TEST_P(SurfaceTests, ReleaseSurfaceAfterUnconfigureThenDeviceDestroy) {
+    // TODO(crbug.com/500766623): Fails due to backend validation errors on
+    // Windows 11/AMD RX 5500 XT w/ D3D12.
+    DAWN_SUPPRESS_TEST_IF(IsWindows11() && IsAMD() && IsD3D12() && IsBackendValidationEnabled());
+
+    wgpu::Surface surface = CreateTestSurface();
+    wgpu::SurfaceConfiguration config = GetPreferredConfiguration(surface);
+    surface.Configure(&config);
+
+    wgpu::SurfaceTexture surfaceTexture;
+    surface.GetCurrentTexture(&surfaceTexture);
+    ClearTexture(surfaceTexture.texture, {1.0, 0.0, 0.0, 1.0});
+    ASSERT_EQ(wgpu::Status::Success, surface.Present());
+
+    surface.Unconfigure();
+    DestroyDevice();
+}
+
+// Releasing a configured surface after the last external reference to its device was dropped
+// (which destroys the device) must not crash.
+TEST_P(SurfaceTests, ReleaseSurfaceAfterDeviceReleased) {
+    // TODO(crbug.com/500766623): Fails due to backend validation errors on
+    // Windows 11/AMD RX 5500 XT w/ D3D12.
+    DAWN_SUPPRESS_TEST_IF(IsWindows11() && IsAMD() && IsD3D12() && IsBackendValidationEnabled());
+
+    wgpu::Device device2 = CreateDevice();
+
+    wgpu::Surface surface = CreateTestSurface();
+    wgpu::SurfaceConfiguration config = GetPreferredConfiguration(surface);
+    config.device = device2;
+    surface.Configure(&config);
+
+    wgpu::SurfaceTexture surfaceTexture;
+    surface.GetCurrentTexture(&surfaceTexture);
+    ClearTexture(surfaceTexture.texture, {1.0, 0.0, 0.0, 1.0}, device2);
+    ASSERT_EQ(wgpu::Status::Success, surface.Present());
+
+    surfaceTexture.texture = nullptr;
+    device2 = nullptr;
+}
+
+// A surface configured with a destroyed device can be configured again with a new device.
+TEST_P(SurfaceTests, ReconfigureWithNewDeviceAfterDestroy) {
+    // TODO(crbug.com/dawn/269): Creating the IDXGISwapChain1 for the new device fails with
+    // E_ACCESSDENIED on D3D11 because the swapchain of the destroyed device is still alive, like
+    // in SwitchingDevice.
+    DAWN_SUPPRESS_TEST_IF(IsD3D11());
+
+    // TODO(crbug.com/500766623): Fails due to backend validation errors on
+    // Windows 11/AMD RX 5500 XT w/ D3D12.
+    DAWN_SUPPRESS_TEST_IF(IsWindows11() && IsAMD() && IsD3D12() && IsBackendValidationEnabled());
+
+    wgpu::Surface surface = CreateTestSurface();
+    wgpu::SurfaceConfiguration config = GetPreferredConfiguration(surface);
+    surface.Configure(&config);
+
+    wgpu::SurfaceTexture surfaceTexture;
+    surface.GetCurrentTexture(&surfaceTexture);
+    ClearTexture(surfaceTexture.texture, {1.0, 0.0, 0.0, 1.0});
+    ASSERT_EQ(wgpu::Status::Success, surface.Present());
+
+    DestroyDevice();
+
+    wgpu::Device device2 = CreateDevice();
+    config.device = device2;
+    surface.Configure(&config);
+
+    surface.GetCurrentTexture(&surfaceTexture);
+    ASSERT_EQ(wgpu::SurfaceGetCurrentTextureStatus::SuccessOptimal, surfaceTexture.status);
+    ClearTexture(surfaceTexture.texture, {0.0, 1.0, 0.0, 1.0}, device2);
+    ASSERT_EQ(wgpu::Status::Success, surface.Present());
+}
+
+// Unconfiguring a surface after its device was destroyed must not crash. Destroying the device
+// already unconfigured the surface, so this is a no-op like unconfiguring an unconfigured surface.
+TEST_P(SurfaceTests, UnconfigureAfterDeviceDestroy) {
+    // TODO(crbug.com/500766623): Fails due to backend validation errors on
+    // Windows 11/AMD RX 5500 XT w/ D3D12.
+    DAWN_SUPPRESS_TEST_IF(IsWindows11() && IsAMD() && IsD3D12() && IsBackendValidationEnabled());
+
+    wgpu::Surface surface = CreateTestSurface();
+    wgpu::SurfaceConfiguration config = GetPreferredConfiguration(surface);
+    surface.Configure(&config);
+
+    wgpu::SurfaceTexture surfaceTexture;
+    surface.GetCurrentTexture(&surfaceTexture);
+    ClearTexture(surfaceTexture.texture, {1.0, 0.0, 0.0, 1.0});
+    ASSERT_EQ(wgpu::Status::Success, surface.Present());
+
+    DestroyDevice();
+
+    surface.Unconfigure();
+}
+
+// Getting the current texture or presenting after the device was destroyed must not crash. The
+// surface behaves as if it was unconfigured.
+TEST_P(SurfaceTests, GetCurrentTextureAfterDeviceDestroy) {
+    // TODO(crbug.com/500766623): Fails due to backend validation errors on
+    // Windows 11/AMD RX 5500 XT w/ D3D12.
+    DAWN_SUPPRESS_TEST_IF(IsWindows11() && IsAMD() && IsD3D12() && IsBackendValidationEnabled());
+
+    wgpu::Surface surface = CreateTestSurface();
+    wgpu::SurfaceConfiguration config = GetPreferredConfiguration(surface);
+    surface.Configure(&config);
+
+    wgpu::SurfaceTexture surfaceTexture;
+    surface.GetCurrentTexture(&surfaceTexture);
+    ClearTexture(surfaceTexture.texture, {1.0, 0.0, 0.0, 1.0});
+    ASSERT_EQ(wgpu::Status::Success, surface.Present());
+
+    DestroyDevice();
+
+    // The surface is unconfigured and has no device anymore, so the validation errors are
+    // reported to the instance.
+    surface.GetCurrentTexture(&surfaceTexture);
+    EXPECT_EQ(surfaceTexture.status, wgpu::SurfaceGetCurrentTextureStatus::Error);
+    EXPECT_EQ(surfaceTexture.texture, nullptr);
+    ASSERT_EQ(wgpu::Status::Error, surface.Present());
+}
+
 // Check that all surfaces must support RenderAttachment.
 TEST_P(SurfaceTests, RenderAttachmentAlwaysSupported) {
     wgpu::Surface surface = CreateTestSurface();