[WebGPU] Extra deref of SwapChainImpl::m_backing #8941

mwyrzykowski · 2023-01-22T07:19:31Z

`83826f3`

[WebGPU] Extra deref of SwapChainImpl::m_backing
https://bugs.webkit.org/show_bug.cgi?id=250963
<rdar://104522929>

Reviewed by Myles C. Maxfield.

WebGPUSurfaceImpl and WebGPUSwapChainImpl called release in both
destroy and their destructor, resulting in a double release.

Fix this issue by setting the backing to null when the object is released.

* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUSurfaceImpl.cpp:
(PAL::WebGPU::SurfaceImpl::destroy):
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUSwapChainImpl.cpp:
(PAL::WebGPU::SwapChainImpl::~SwapChainImpl):
(PAL::WebGPU::SwapChainImpl::destroy):

Canonical link: https://commits.webkit.org/259318@main

c6f2805

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-01-22T07:25:34Z

EWS run on previous version of this PR (hash 9236988)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	❌ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

litherum · 2023-01-22T07:38:39Z

Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUSwapChainImpl.cpp

-    wgpuSwapChainRelease(m_backing);
+    destroy();
 }

 void SwapChainImpl::destroy()
 {
-    wgpuSwapChainRelease(m_backing);


I think it's wrong to delete all calls to wgpuSwapChainRelease(). I think the fact that there's only a single object under the hood shouldn't be visible from outside WebGPU.framework.

Good idea, I'll increment the ref count instead on m_backing

litherum

requesting changes due to missing wgpuSwapChainRelease()

webkit-early-warning-system · 2023-01-24T01:55:12Z

EWS run on previous version of this PR (hash de93d28)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

litherum

I think this bumps the ref by 2 instead of by 1

litherum · 2023-01-24T02:16:00Z

Source/WebGPU/WebGPU/PresentationContext.mm

@@ -36,6 +36,7 @@
 Ref<PresentationContext> Device::createSwapChain(PresentationContext& presentationContext, const WGPUSwapChainDescriptor& descriptor)
 {
    presentationContext.configure(*this, descriptor);
+    presentationContext.ref();


Why is this necessary? The input to this function is a PresentationContext& and the output is a Ref. So the next line return statement will invoke the constructor of Ref that will increment the ref count. So I think this change will actually bump the ref count by 2, not 1.

Also, suspicious that every ref needs to be matched with a deref but this one doesn't seem to be.

Good observation, I will take a closer look.

Oh I think I found the root cause now. The destructor and destroy were both calling release. wgpuSurfaceRelease doesn't actually release the backing, it just decrements the ref count. So after calling this I now set the backing to null so it can't be released again.

webkit-early-warning-system · 2023-01-24T07:50:25Z

EWS run on current version of this PR (hash c6f2805)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge	✅ 🛠 watch-sim

litherum

OK. I'm curious how this will end up, regarding #8856 and #8940

https://bugs.webkit.org/show_bug.cgi?id=250963 <rdar://104522929> Reviewed by Myles C. Maxfield. WebGPUSurfaceImpl and WebGPUSwapChainImpl called release in both destroy and their destructor, resulting in a double release. Fix this issue by setting the backing to null when the object is released. * Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUSurfaceImpl.cpp: (PAL::WebGPU::SurfaceImpl::destroy): * Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUSwapChainImpl.cpp: (PAL::WebGPU::SwapChainImpl::~SwapChainImpl): (PAL::WebGPU::SwapChainImpl::destroy): Canonical link: https://commits.webkit.org/259318@main

webkit-commit-queue · 2023-01-25T00:44:46Z

Committed 259318@main (83826f3): https://commits.webkit.org/259318@main

Reviewed commits have been landed. Closing PR #8941 and removing active labels.

mwyrzykowski self-assigned this Jan 22, 2023

mwyrzykowski added the WebGPU For bugs in WebGPU label Jan 22, 2023

mwyrzykowski requested a review from litherum January 22, 2023 07:25

litherum reviewed Jan 22, 2023

View reviewed changes

litherum requested changes Jan 22, 2023

View reviewed changes

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 22, 2023

mwyrzykowski removed the merging-blocked Applied to prevent a change from being merged label Jan 24, 2023

mwyrzykowski force-pushed the eng/WebGPU-Extra-deref-of-SwapChainImplm_backing branch from 9236988 to de93d28 Compare January 24, 2023 01:53

mwyrzykowski requested review from kkinnunen-apple, tadeuzagallo, djg, grorg and litherum January 24, 2023 01:55

litherum requested changes Jan 24, 2023

View reviewed changes

mwyrzykowski force-pushed the eng/WebGPU-Extra-deref-of-SwapChainImplm_backing branch from de93d28 to c6f2805 Compare January 24, 2023 07:49

litherum approved these changes Jan 24, 2023

View reviewed changes

mwyrzykowski added the merge-queue Applied to send a pull request to merge-queue label Jan 24, 2023

mwyrzykowski mentioned this pull request Jan 24, 2023

[WebGPU] Implement resource deallocation #8856

Closed

webkit-early-warning-system force-pushed the eng/WebGPU-Extra-deref-of-SwapChainImplm_backing branch from c6f2805 to 83826f3 Compare January 25, 2023 00:44

webkit-early-warning-system merged commit 83826f3 into WebKit:main Jan 25, 2023

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jan 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WebGPU] Extra deref of SwapChainImpl::m_backing #8941

[WebGPU] Extra deref of SwapChainImpl::m_backing #8941

mwyrzykowski commented Jan 22, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Jan 22, 2023 •

edited

litherum Jan 22, 2023

mwyrzykowski Jan 22, 2023

litherum left a comment

webkit-early-warning-system commented Jan 24, 2023 •

edited

litherum left a comment

litherum Jan 24, 2023

mwyrzykowski Jan 24, 2023

mwyrzykowski Jan 24, 2023

webkit-early-warning-system commented Jan 24, 2023 •

edited

litherum left a comment

webkit-commit-queue commented Jan 25, 2023

[WebGPU] Extra deref of SwapChainImpl::m_backing #8941

[WebGPU] Extra deref of SwapChainImpl::m_backing #8941

Conversation

mwyrzykowski commented Jan 22, 2023 • edited by webkit-early-warning-system

83826f3

webkit-early-warning-system commented Jan 22, 2023 • edited

litherum Jan 22, 2023

Choose a reason for hiding this comment

mwyrzykowski Jan 22, 2023

Choose a reason for hiding this comment

litherum left a comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Jan 24, 2023 • edited

litherum left a comment

Choose a reason for hiding this comment

litherum Jan 24, 2023

Choose a reason for hiding this comment

mwyrzykowski Jan 24, 2023

Choose a reason for hiding this comment

mwyrzykowski Jan 24, 2023

Choose a reason for hiding this comment

webkit-early-warning-system commented Jan 24, 2023 • edited

litherum left a comment

Choose a reason for hiding this comment

webkit-commit-queue commented Jan 25, 2023

mwyrzykowski commented Jan 22, 2023 •

edited by webkit-early-warning-system

`83826f3`

webkit-early-warning-system commented Jan 22, 2023 •

edited

webkit-early-warning-system commented Jan 24, 2023 •

edited

webkit-early-warning-system commented Jan 24, 2023 •

edited