From b594374ba84da986c5ebc93984961040338465d0 Mon Sep 17 00:00:00 2001 From: Laszlo Agocs Date: Tue, 27 Jul 2021 19:42:27 +0200 Subject: rhi: Be more graceful when one destroys a resource after the QRhi One is a bad application or library in this case, but nonetheless we should handle this more gracefully then just crashing due to the QRhi already having been destroyed. Mainly because in Qt 5 one could get away with the same: releasing OpenGL objects underneath, for example, a QSGPlainTexture with no (or wrong) GL context did not generate any user visible fatal errors. So we should not crash in Qt 6 either with these code bases. In debug builds or when QT_RHI_LEAK_CHECK is set, one will get the unreleased resources warning printed in Qt 6, which is a step forward compared to Qt 5. So there is still some indication that something is badly designed, even if the application survives. Task-number: QTBUG-95394 Pick-to: 6.2 Change-Id: I944f4f425ff126e7363a82aff926b280ccf1dfc3 Reviewed-by: Qt CI Bot Reviewed-by: Andy Nichols --- src/gui/rhi/qrhi.cpp | 16 ++++-- src/gui/rhi/qrhid3d11.cpp | 54 +++++++++++--------- src/gui/rhi/qrhigles2.cpp | 76 +++++++++++++++++++---------- src/gui/rhi/qrhimetal.mm | 60 +++++++++++++---------- src/gui/rhi/qrhinull.cpp | 79 +++++++++++++++++++++++++++--- src/gui/rhi/qrhinull_p_p.h | 4 ++ src/gui/rhi/qrhivulkan.cpp | 95 ++++++++++++++++++++---------------- tests/auto/gui/rhi/qrhi/tst_qrhi.cpp | 49 +++++++++++++++++++ 8 files changed, 306 insertions(+), 127 deletions(-) diff --git a/src/gui/rhi/qrhi.cpp b/src/gui/rhi/qrhi.cpp index 4ae75b5df5..88fb19e5f3 100644 --- a/src/gui/rhi/qrhi.cpp +++ b/src/gui/rhi/qrhi.cpp @@ -4439,11 +4439,19 @@ QRhiImplementation::~QRhiImplementation() // release builds: opt-in static bool leakCheck = qEnvironmentVariableIntValue("QT_RHI_LEAK_CHECK"); #endif - if (leakCheck && !resources.isEmpty()) { - qWarning("QRhi %p going down with %d unreleased resources that own native graphics objects. This is not nice.", - q, int(resources.count())); + if (!resources.isEmpty()) { + if (leakCheck) { + qWarning("QRhi %p going down with %d unreleased resources that own native graphics objects. This is not nice.", + q, int(resources.count())); + } for (QRhiResource *res : qAsConst(resources)) { - qWarning(" %s resource %p (%s)", resourceTypeStr(res), res, res->m_objectName.constData()); + if (leakCheck) + qWarning(" %s resource %p (%s)", resourceTypeStr(res), res, res->m_objectName.constData()); + + // Null out the resource's rhi pointer. This is why it makes sense to do null + // checks in the destroy() implementations of the various resource types. It + // allows to survive in bad applications that somehow manage to destroy a + // resource of a QRhi after the QRhi itself. res->m_rhi = nullptr; } } diff --git a/src/gui/rhi/qrhid3d11.cpp b/src/gui/rhi/qrhid3d11.cpp index 93d3e30943..c02197b151 100644 --- a/src/gui/rhi/qrhid3d11.cpp +++ b/src/gui/rhi/qrhid3d11.cpp @@ -2664,9 +2664,11 @@ void QD3D11Buffer::destroy() } QRHI_RES_RHI(QRhiD3D11); - QRHI_PROF; - QRHI_PROF_F(releaseBuffer(this)); - rhiD->unregisterResource(this); + if (rhiD) { + QRHI_PROF; + QRHI_PROF_F(releaseBuffer(this)); + rhiD->unregisterResource(this); + } } static inline uint toD3DBufferUsage(QRhiBuffer::UsageFlags usage) @@ -2821,9 +2823,11 @@ void QD3D11RenderBuffer::destroy() tex = nullptr; QRHI_RES_RHI(QRhiD3D11); - QRHI_PROF; - QRHI_PROF_F(releaseRenderBuffer(this)); - rhiD->unregisterResource(this); + if (rhiD) { + QRHI_PROF; + QRHI_PROF_F(releaseRenderBuffer(this)); + rhiD->unregisterResource(this); + } } bool QD3D11RenderBuffer::create() @@ -2948,9 +2952,11 @@ void QD3D11Texture::destroy() tex3D = nullptr; QRHI_RES_RHI(QRhiD3D11); - QRHI_PROF; - QRHI_PROF_F(releaseTexture(this)); - rhiD->unregisterResource(this); + if (rhiD) { + QRHI_PROF; + QRHI_PROF_F(releaseTexture(this)); + rhiD->unregisterResource(this); + } } static inline DXGI_FORMAT toD3DDepthTextureSRVFormat(QRhiTexture::Format format) @@ -3238,7 +3244,8 @@ void QD3D11Sampler::destroy() samplerState = nullptr; QRHI_RES_RHI(QRhiD3D11); - rhiD->unregisterResource(this); + if (rhiD) + rhiD->unregisterResource(this); } static inline D3D11_FILTER toD3DFilter(QRhiSampler::Filter minFilter, QRhiSampler::Filter magFilter, QRhiSampler::Filter mipFilter) @@ -3419,8 +3426,6 @@ QD3D11TextureRenderTarget::~QD3D11TextureRenderTarget() void QD3D11TextureRenderTarget::destroy() { - QRHI_RES_RHI(QRhiD3D11); - if (!rtv[0] && !dsv) return; @@ -3438,7 +3443,9 @@ void QD3D11TextureRenderTarget::destroy() } } - rhiD->unregisterResource(this); + QRHI_RES_RHI(QRhiD3D11); + if (rhiD) + rhiD->unregisterResource(this); } QRhiRenderPassDescriptor *QD3D11TextureRenderTarget::newCompatibleRenderPassDescriptor() @@ -3632,8 +3639,6 @@ QD3D11GraphicsPipeline::~QD3D11GraphicsPipeline() void QD3D11GraphicsPipeline::destroy() { - QRHI_RES_RHI(QRhiD3D11); - if (!dsState) return; @@ -3667,7 +3672,9 @@ void QD3D11GraphicsPipeline::destroy() } fs.nativeResourceBindingMap.clear(); - rhiD->unregisterResource(this); + QRHI_RES_RHI(QRhiD3D11); + if (rhiD) + rhiD->unregisterResource(this); } static inline D3D11_CULL_MODE toD3DCullMode(QRhiGraphicsPipeline::CullMode c) @@ -4174,8 +4181,6 @@ QD3D11ComputePipeline::~QD3D11ComputePipeline() void QD3D11ComputePipeline::destroy() { - QRHI_RES_RHI(QRhiD3D11); - if (!cs.shader) return; @@ -4183,7 +4188,9 @@ void QD3D11ComputePipeline::destroy() cs.shader = nullptr; cs.nativeResourceBindingMap.clear(); - rhiD->unregisterResource(this); + QRHI_RES_RHI(QRhiD3D11); + if (rhiD) + rhiD->unregisterResource(this); } bool QD3D11ComputePipeline::create() @@ -4317,11 +4324,12 @@ void QD3D11SwapChain::destroy() swapChain->Release(); swapChain = nullptr; - QRHI_PROF; - QRHI_PROF_F(releaseSwapChain(this)); - QRHI_RES_RHI(QRhiD3D11); - rhiD->unregisterResource(this); + if (rhiD) { + QRHI_PROF; + QRHI_PROF_F(releaseSwapChain(this)); + rhiD->unregisterResource(this); + } } QRhiCommandBuffer *QD3D11SwapChain::currentFrameCommandBuffer() diff --git a/src/gui/rhi/qrhigles2.cpp b/src/gui/rhi/qrhigles2.cpp index 05465e40cc..7650e852ff 100644 --- a/src/gui/rhi/qrhigles2.cpp +++ b/src/gui/rhi/qrhigles2.cpp @@ -4256,10 +4256,12 @@ void QGles2Buffer::destroy() buffer = 0; QRHI_RES_RHI(QRhiGles2); - rhiD->releaseQueue.append(e); - QRHI_PROF; - QRHI_PROF_F(releaseBuffer(this)); - rhiD->unregisterResource(this); + if (rhiD) { + rhiD->releaseQueue.append(e); + QRHI_PROF; + QRHI_PROF_F(releaseBuffer(this)); + rhiD->unregisterResource(this); + } } bool QGles2Buffer::create() @@ -4366,11 +4368,13 @@ void QGles2RenderBuffer::destroy() stencilRenderbuffer = 0; QRHI_RES_RHI(QRhiGles2); - if (owns) - rhiD->releaseQueue.append(e); - QRHI_PROF; - QRHI_PROF_F(releaseRenderBuffer(this)); - rhiD->unregisterResource(this); + if (rhiD) { + if (owns) + rhiD->releaseQueue.append(e); + QRHI_PROF; + QRHI_PROF_F(releaseRenderBuffer(this)); + rhiD->unregisterResource(this); + } } bool QGles2RenderBuffer::create() @@ -4521,11 +4525,13 @@ void QGles2Texture::destroy() zeroInitialized = false; QRHI_RES_RHI(QRhiGles2); - if (owns) - rhiD->releaseQueue.append(e); - QRHI_PROF; - QRHI_PROF_F(releaseTexture(this)); - rhiD->unregisterResource(this); + if (rhiD) { + if (owns) + rhiD->releaseQueue.append(e); + QRHI_PROF; + QRHI_PROF_F(releaseTexture(this)); + rhiD->unregisterResource(this); + } } bool QGles2Texture::prepareCreate(QSize *adjustedSize) @@ -4803,9 +4809,10 @@ void QGles2TextureRenderTarget::destroy() framebuffer = 0; QRHI_RES_RHI(QRhiGles2); - rhiD->releaseQueue.append(e); - - rhiD->unregisterResource(this); + if (rhiD) { + rhiD->releaseQueue.append(e); + rhiD->unregisterResource(this); + } } QRhiRenderPassDescriptor *QGles2TextureRenderTarget::newCompatibleRenderPassDescriptor() @@ -4999,9 +5006,10 @@ void QGles2GraphicsPipeline::destroy() samplers.clear(); QRHI_RES_RHI(QRhiGles2); - rhiD->releaseQueue.append(e); - - rhiD->unregisterResource(this); + if (rhiD) { + rhiD->releaseQueue.append(e); + rhiD->unregisterResource(this); + } } bool QGles2GraphicsPipeline::create() @@ -5135,9 +5143,10 @@ void QGles2ComputePipeline::destroy() samplers.clear(); QRHI_RES_RHI(QRhiGles2); - rhiD->releaseQueue.append(e); - - rhiD->unregisterResource(this); + if (rhiD) { + rhiD->releaseQueue.append(e); + rhiD->unregisterResource(this); + } } bool QGles2ComputePipeline::create() @@ -5231,8 +5240,12 @@ QGles2SwapChain::~QGles2SwapChain() void QGles2SwapChain::destroy() { - QRHI_PROF; - QRHI_PROF_F(releaseSwapChain(this)); + QRHI_RES_RHI(QRhiGles2); + if (rhiD) { + QRHI_PROF; + QRHI_PROF_F(releaseSwapChain(this)); + rhiD->unregisterResource(this); + } } QRhiCommandBuffer *QGles2SwapChain::currentFrameCommandBuffer() @@ -5258,6 +5271,11 @@ QRhiRenderPassDescriptor *QGles2SwapChain::newCompatibleRenderPassDescriptor() bool QGles2SwapChain::createOrResize() { + // can be called multiple times due to window resizes + const bool needsRegistration = !surface || surface != m_window; + if (surface && surface != m_window) + destroy(); + surface = m_window; m_currentPixelSize = surfacePixelSize(); pixelSize = m_currentPixelSize; @@ -5283,6 +5301,14 @@ bool QGles2SwapChain::createOrResize() // make something up QRHI_PROF_F(resizeSwapChain(this, 2, m_sampleCount > 1 ? 2 : 0, m_sampleCount)); + // The only reason to register this fairly fake gl swapchain + // object with no native resources underneath is to be able to + // implement a safe destroy(). + if (needsRegistration) { + QRHI_RES_RHI(QRhiGles2); + rhiD->registerResource(this); + } + return true; } diff --git a/src/gui/rhi/qrhimetal.mm b/src/gui/rhi/qrhimetal.mm index cf11f12ead..3da25b7ac4 100644 --- a/src/gui/rhi/qrhimetal.mm +++ b/src/gui/rhi/qrhimetal.mm @@ -2237,10 +2237,12 @@ void QMetalBuffer::destroy() } QRHI_RES_RHI(QRhiMetal); - rhiD->d->releaseQueue.append(e); - QRHI_PROF; - QRHI_PROF_F(releaseBuffer(this)); - rhiD->unregisterResource(this); + if (rhiD) { + rhiD->d->releaseQueue.append(e); + QRHI_PROF; + QRHI_PROF_F(releaseBuffer(this)); + rhiD->unregisterResource(this); + } } bool QMetalBuffer::create() @@ -2516,10 +2518,12 @@ void QMetalRenderBuffer::destroy() d->tex = nil; QRHI_RES_RHI(QRhiMetal); - rhiD->d->releaseQueue.append(e); - QRHI_PROF; - QRHI_PROF_F(releaseRenderBuffer(this)); - rhiD->unregisterResource(this); + if (rhiD) { + rhiD->d->releaseQueue.append(e); + QRHI_PROF; + QRHI_PROF_F(releaseRenderBuffer(this)); + rhiD->unregisterResource(this); + } } bool QMetalRenderBuffer::create() @@ -2633,10 +2637,12 @@ void QMetalTexture::destroy() } QRHI_RES_RHI(QRhiMetal); - rhiD->d->releaseQueue.append(e); - QRHI_PROF; - QRHI_PROF_F(releaseTexture(this)); - rhiD->unregisterResource(this); + if (rhiD) { + rhiD->d->releaseQueue.append(e); + QRHI_PROF; + QRHI_PROF_F(releaseTexture(this)); + rhiD->unregisterResource(this); + } } bool QMetalTexture::prepareCreate(QSize *adjustedSize) @@ -2801,8 +2807,10 @@ void QMetalSampler::destroy() d->samplerState = nil; QRHI_RES_RHI(QRhiMetal); - rhiD->d->releaseQueue.append(e); - rhiD->unregisterResource(this); + if (rhiD) { + rhiD->d->releaseQueue.append(e); + rhiD->unregisterResource(this); + } } static inline MTLSamplerMinMagFilter toMetalFilter(QRhiSampler::Filter f) @@ -3171,8 +3179,6 @@ QMetalGraphicsPipeline::~QMetalGraphicsPipeline() void QMetalGraphicsPipeline::destroy() { - QRHI_RES_RHI(QRhiMetal); - d->vs.destroy(); d->fs.destroy(); @@ -3185,7 +3191,9 @@ void QMetalGraphicsPipeline::destroy() [d->ps release]; d->ps = nil; - rhiD->unregisterResource(this); + QRHI_RES_RHI(QRhiMetal); + if (rhiD) + rhiD->unregisterResource(this); } static inline MTLVertexFormat toMetalAttributeFormat(QRhiVertexInputAttribute::Format format) @@ -3672,8 +3680,6 @@ QMetalComputePipeline::~QMetalComputePipeline() void QMetalComputePipeline::destroy() { - QRHI_RES_RHI(QRhiMetal); - d->cs.destroy(); if (!d->ps) @@ -3682,7 +3688,9 @@ void QMetalComputePipeline::destroy() [d->ps release]; d->ps = nil; - rhiD->unregisterResource(this); + QRHI_RES_RHI(QRhiMetal); + if (rhiD) + rhiD->unregisterResource(this); } bool QMetalComputePipeline::create() @@ -3851,12 +3859,12 @@ void QMetalSwapChain::destroy() d->curDrawable = nil; QRHI_RES_RHI(QRhiMetal); - rhiD->swapchains.remove(this); - - QRHI_PROF; - QRHI_PROF_F(releaseSwapChain(this)); - - rhiD->unregisterResource(this); + if (rhiD) { + rhiD->swapchains.remove(this); + QRHI_PROF; + QRHI_PROF_F(releaseSwapChain(this)); + rhiD->unregisterResource(this); + } } QRhiCommandBuffer *QMetalSwapChain::currentFrameCommandBuffer() diff --git a/src/gui/rhi/qrhinull.cpp b/src/gui/rhi/qrhinull.cpp index d8dd77a790..cfcbf09d91 100644 --- a/src/gui/rhi/qrhinull.cpp +++ b/src/gui/rhi/qrhinull.cpp @@ -593,17 +593,30 @@ void QNullBuffer::destroy() delete[] data; data = nullptr; - QRHI_PROF; - QRHI_PROF_F(releaseBuffer(this)); + QRHI_RES_RHI(QRhiNull); + if (rhiD) { + QRHI_PROF; + QRHI_PROF_F(releaseBuffer(this)); + rhiD->unregisterResource(this); + } } bool QNullBuffer::create() { + if (data) + destroy(); + data = new char[m_size]; memset(data, 0, m_size); QRHI_PROF; QRHI_PROF_F(newBuffer(this, uint(m_size), 1, 0)); + // If we register the buffer to the profiler, then it needs to be registered to the + // QRhi too (even though we normally do that for native-resource-owning objects only), + // in order to be able to implement destroy() in a robust manner. + QRHI_RES_RHI(QRhiNull); + rhiD->registerResource(this); + return true; } @@ -627,14 +640,28 @@ QNullRenderBuffer::~QNullRenderBuffer() void QNullRenderBuffer::destroy() { - QRHI_PROF; - QRHI_PROF_F(releaseRenderBuffer(this)); + valid = false; + + QRHI_RES_RHI(QRhiNull); + if (rhiD) { + QRHI_PROF; + QRHI_PROF_F(releaseRenderBuffer(this)); + rhiD->unregisterResource(this); + } } bool QNullRenderBuffer::create() { + if (valid) + destroy(); + + valid = true; + QRHI_PROF; QRHI_PROF_F(newRenderBuffer(this, false, false, 1)); + QRHI_RES_RHI(QRhiNull); + rhiD->registerResource(this); + return true; } @@ -656,12 +683,23 @@ QNullTexture::~QNullTexture() void QNullTexture::destroy() { - QRHI_PROF; - QRHI_PROF_F(releaseTexture(this)); + valid = false; + + QRHI_RES_RHI(QRhiNull); + if (rhiD) { + QRHI_PROF; + QRHI_PROF_F(releaseTexture(this)); + rhiD->unregisterResource(this); + } } bool QNullTexture::create() { + if (valid) + destroy(); + + valid = true; + QRHI_RES_RHI(QRhiNull); const bool isCube = m_flags.testFlag(CubeMap); const bool is3D = m_flags.testFlag(ThreeDimensional); @@ -684,19 +722,29 @@ bool QNullTexture::create() QRHI_PROF; QRHI_PROF_F(newTexture(this, true, mipLevelCount, layerCount, 1)); + rhiD->registerResource(this); + return true; } bool QNullTexture::createFrom(QRhiTexture::NativeTexture src) { Q_UNUSED(src); + if (valid) + destroy(); + + valid = true; + QRHI_RES_RHI(QRhiNull); const bool isCube = m_flags.testFlag(CubeMap); const bool hasMipMaps = m_flags.testFlag(MipMapped); QSize size = m_pixelSize.isEmpty() ? QSize(1, 1) : m_pixelSize; const int mipLevelCount = hasMipMaps ? rhiD->q->mipLevelsForSize(size) : 1; + QRHI_PROF; QRHI_PROF_F(newTexture(this, false, mipLevelCount, isCube ? 6 : 1, 1)); + rhiD->registerResource(this); + return true; } @@ -925,8 +973,12 @@ QNullSwapChain::~QNullSwapChain() void QNullSwapChain::destroy() { - QRHI_PROF; - QRHI_PROF_F(releaseSwapChain(this)); + QRHI_RES_RHI(QRhiNull); + if (rhiD) { + QRHI_PROF; + QRHI_PROF_F(releaseSwapChain(this)); + rhiD->unregisterResource(this); + } } QRhiCommandBuffer *QNullSwapChain::currentFrameCommandBuffer() @@ -951,12 +1003,23 @@ QRhiRenderPassDescriptor *QNullSwapChain::newCompatibleRenderPassDescriptor() bool QNullSwapChain::createOrResize() { + const bool needsRegistration = !window || window != m_window; + if (window && window != m_window) + destroy(); + + window = m_window; m_currentPixelSize = surfacePixelSize(); rt.d.rp = QRHI_RES(QNullRenderPassDescriptor, m_renderPassDesc); rt.d.pixelSize = m_currentPixelSize; frameCount = 0; + QRHI_PROF; QRHI_PROF_F(resizeSwapChain(this, 1, 0, 1)); + if (needsRegistration) { + QRHI_RES_RHI(QRhiNull); + rhiD->registerResource(this); + } + return true; } diff --git a/src/gui/rhi/qrhinull_p_p.h b/src/gui/rhi/qrhinull_p_p.h index b064e7ad37..ddc75d7716 100644 --- a/src/gui/rhi/qrhinull_p_p.h +++ b/src/gui/rhi/qrhinull_p_p.h @@ -76,6 +76,8 @@ struct QNullRenderBuffer : public QRhiRenderBuffer void destroy() override; bool create() override; QRhiTexture::Format backingFormat() const override; + + bool valid = false; }; struct QNullTexture : public QRhiTexture @@ -87,6 +89,7 @@ struct QNullTexture : public QRhiTexture bool create() override; bool createFrom(NativeTexture src) override; + bool valid = false; QVarLengthArray, 6> image; }; @@ -191,6 +194,7 @@ struct QNullSwapChain : public QRhiSwapChain QRhiRenderPassDescriptor *newCompatibleRenderPassDescriptor() override; bool createOrResize() override; + QWindow *window = nullptr; QNullReferenceRenderTarget rt; QNullCommandBuffer cb; int frameCount = 0; diff --git a/src/gui/rhi/qrhivulkan.cpp b/src/gui/rhi/qrhivulkan.cpp index 64b979b8c7..20e7c2f626 100644 --- a/src/gui/rhi/qrhivulkan.cpp +++ b/src/gui/rhi/qrhivulkan.cpp @@ -5523,12 +5523,15 @@ void QVkBuffer::destroy() } QRHI_RES_RHI(QRhiVulkan); - rhiD->releaseQueue.append(e); - - QRHI_PROF; - QRHI_PROF_F(releaseBuffer(this)); - - rhiD->unregisterResource(this); + // destroy() implementations, unlike other functions, are expected to test + // for m_rhi being null, to allow surviving in case one attempts to destroy + // a (leaked) resource after the QRhi. + if (rhiD) { + rhiD->releaseQueue.append(e); + QRHI_PROF; + QRHI_PROF_F(releaseBuffer(this)); + rhiD->unregisterResource(this); + } } bool QVkBuffer::create() @@ -5680,12 +5683,12 @@ void QVkRenderBuffer::destroy() } QRHI_RES_RHI(QRhiVulkan); - rhiD->releaseQueue.append(e); - - QRHI_PROF; - QRHI_PROF_F(releaseRenderBuffer(this)); - - rhiD->unregisterResource(this); + if (rhiD) { + rhiD->releaseQueue.append(e); + QRHI_PROF; + QRHI_PROF_F(releaseRenderBuffer(this)); + rhiD->unregisterResource(this); + } } bool QVkRenderBuffer::create() @@ -5803,12 +5806,12 @@ void QVkTexture::destroy() imageAlloc = nullptr; QRHI_RES_RHI(QRhiVulkan); - rhiD->releaseQueue.append(e); - - QRHI_PROF; - QRHI_PROF_F(releaseTexture(this)); - - rhiD->unregisterResource(this); + if (rhiD) { + rhiD->releaseQueue.append(e); + QRHI_PROF; + QRHI_PROF_F(releaseTexture(this)); + rhiD->unregisterResource(this); + } } bool QVkTexture::prepareCreate(QSize *adjustedSize) @@ -6087,8 +6090,10 @@ void QVkSampler::destroy() sampler = VK_NULL_HANDLE; QRHI_RES_RHI(QRhiVulkan); - rhiD->releaseQueue.append(e); - rhiD->unregisterResource(this); + if (rhiD) { + rhiD->releaseQueue.append(e); + rhiD->unregisterResource(this); + } } bool QVkSampler::create() @@ -6152,9 +6157,10 @@ void QVkRenderPassDescriptor::destroy() rp = VK_NULL_HANDLE; QRHI_RES_RHI(QRhiVulkan); - rhiD->releaseQueue.append(e); - - rhiD->unregisterResource(this); + if (rhiD) { + rhiD->releaseQueue.append(e); + rhiD->unregisterResource(this); + } } static inline bool attachmentDescriptionEquals(const VkAttachmentDescription &a, const VkAttachmentDescription &b) @@ -6317,9 +6323,10 @@ void QVkTextureRenderTarget::destroy() } QRHI_RES_RHI(QRhiVulkan); - rhiD->releaseQueue.append(e); - - rhiD->unregisterResource(this); + if (rhiD) { + rhiD->releaseQueue.append(e); + rhiD->unregisterResource(this); + } } QRhiRenderPassDescriptor *QVkTextureRenderTarget::newCompatibleRenderPassDescriptor() @@ -6528,9 +6535,10 @@ void QVkShaderResourceBindings::destroy() descSets[i] = VK_NULL_HANDLE; QRHI_RES_RHI(QRhiVulkan); - rhiD->releaseQueue.append(e); - - rhiD->unregisterResource(this); + if (rhiD) { + rhiD->releaseQueue.append(e); + rhiD->unregisterResource(this); + } } bool QVkShaderResourceBindings::create() @@ -6643,9 +6651,10 @@ void QVkGraphicsPipeline::destroy() layout = VK_NULL_HANDLE; QRHI_RES_RHI(QRhiVulkan); - rhiD->releaseQueue.append(e); - - rhiD->unregisterResource(this); + if (rhiD) { + rhiD->releaseQueue.append(e); + rhiD->unregisterResource(this); + } } bool QVkGraphicsPipeline::create() @@ -6892,9 +6901,10 @@ void QVkComputePipeline::destroy() layout = VK_NULL_HANDLE; QRHI_RES_RHI(QRhiVulkan); - rhiD->releaseQueue.append(e); - - rhiD->unregisterResource(this); + if (rhiD) { + rhiD->releaseQueue.append(e); + rhiD->unregisterResource(this); + } } bool QVkComputePipeline::create() @@ -7014,8 +7024,10 @@ void QVkSwapChain::destroy() return; QRHI_RES_RHI(QRhiVulkan); - rhiD->swapchains.remove(this); - rhiD->releaseSwapChainResources(this); + if (rhiD) { + rhiD->swapchains.remove(this); + rhiD->releaseSwapChainResources(this); + } for (int i = 0; i < QVK_FRAMES_IN_FLIGHT; ++i) { QVkSwapChain::FrameResources &frame(frameRes[i]); @@ -7025,10 +7037,11 @@ void QVkSwapChain::destroy() surface = lastConnectedSurface = VK_NULL_HANDLE; - QRHI_PROF; - QRHI_PROF_F(releaseSwapChain(this)); - - rhiD->unregisterResource(this); + if (rhiD) { + QRHI_PROF; + QRHI_PROF_F(releaseSwapChain(this)); + rhiD->unregisterResource(this); + } } QRhiCommandBuffer *QVkSwapChain::currentFrameCommandBuffer() diff --git a/tests/auto/gui/rhi/qrhi/tst_qrhi.cpp b/tests/auto/gui/rhi/qrhi/tst_qrhi.cpp index 8b6f55b3b6..be825c8ac2 100644 --- a/tests/auto/gui/rhi/qrhi/tst_qrhi.cpp +++ b/tests/auto/gui/rhi/qrhi/tst_qrhi.cpp @@ -137,6 +137,8 @@ private slots: void renderbufferImportOpenGL(); void threeDimTexture_data(); void threeDimTexture(); + void leakedResourceDestroy_data(); + void leakedResourceDestroy(); private: void setWindowType(QWindow *window, QRhi::Implementation impl); @@ -3985,5 +3987,52 @@ void tst_QRhi::threeDimTexture() } } +void tst_QRhi::leakedResourceDestroy_data() +{ + rhiTestData(); +} + +void tst_QRhi::leakedResourceDestroy() +{ + QFETCH(QRhi::Implementation, impl); + QFETCH(QRhiInitParams *, initParams); + + QScopedPointer rhi(QRhi::create(impl, initParams)); + if (!rhi) + QSKIP("QRhi could not be created, skipping"); + + // Incorrectly destroy the QRhi before the resources created from it. Attempting to + // destroy the resources afterwards is pointless, the native resources are leaked. + // Nonetheless, it should not crash, which is what we are testing here. + // + // We do not however have control over other, native and 3rd party components: a + // validation or debug layer, or a memory allocator may warn, assert, or abort when + // not releasing all native resources correctly. +#ifndef QT_NO_DEBUG + // don't want asserts from vkmemalloc, skip the test in debug builds + if (impl == QRhi::Vulkan) + QSKIP("Skipping leaked resource destroy test due to Vulkan and debug build"); +#endif + + QScopedPointer buffer(rhi->newBuffer(QRhiBuffer::Immutable, QRhiBuffer::VertexBuffer, 256)); + QVERIFY(buffer->create()); + + QScopedPointer texture(rhi->newTexture(QRhiTexture::RGBA8, QSize(512, 512), 1, QRhiTexture::RenderTarget)); + QVERIFY(texture->create()); + + QScopedPointer rt(rhi->newTextureRenderTarget({ texture.data() })); + QScopedPointer rpDesc(rt->newCompatibleRenderPassDescriptor()); + QVERIFY(rpDesc); + rt->setRenderPassDescriptor(rpDesc.data()); + QVERIFY(rt->create()); + + if (impl == QRhi::Vulkan) + qDebug("Vulkan validation layer warnings may be printed below - this is expected"); + + rhi.reset(); + + // let the scoped ptr do its job with the resources +} + #include QTEST_MAIN(tst_QRhi) -- cgit v1.2.3