Skip to content

Reduce unsafeness in WebGPU module #47342

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Reduce unsafeness in WebGPU module #47342

wants to merge 1 commit into from
+85 −91

Conversation

annevk
Copy link
Contributor

@annevk annevk commented Jun 28, 2025 •
edited by webkit-early-warning-system
Loading

a854904

Reduce unsafeness in WebGPU module
https://bugs.webkit.org/show_bug.cgi?id=295160

Reviewed by NOBODY (OOPS!).

Apply https://github.com/WebKit/WebKit/wiki/Safer-CPP-Guidelines

a854904

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe loading 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 loading 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
loading-orange 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision loading-orange 🧪 mac-AS-debug-wk2 loading 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@annevk annevk self-assigned this Jun 28, 2025
@annevk annevk added the WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). label Jun 28, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 28, 2025 •
edited
Loading

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

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@annevk annevk requested a review from mwyrzykowski June 28, 2025 20:16
@webkit-ews-buildbot
Copy link
Collaborator

Safer C++ Build #41981 (aca9e84)

⚠️ Found 9 fixed files! Please update expectations in Source/[Project]/SaferCPPExpectations by running the following command and update your pull request:

  • Tools/Scripts/update-safer-cpp-expectations -p WebCore --MemoryUnsafeCastChecker Modules/WebGPU/GPUQueue.cpp --UncountedCallArgsChecker Modules/WebGPU/GPUDevice.cpp Modules/WebGPU/GPUQuerySet.cpp Modules/WebGPU/GPUCommandEncoder.cpp Modules/WebGPU/GPUCommandBuffer.cpp Modules/WebGPU/GPUBuffer.cpp Modules/WebGPU/GPUPresentationContext.cpp Modules/WebGPU/GPURenderPassEncoder.cpp Modules/WebGPU/GPUComputePassEncoder.cpp --UncountedLocalVarsChecker Modules/WebGPU/GPUDevice.cpp

cdumez
cdumez reviewed Jun 30, 2025
View reviewed changes
Source/WebCore/Modules/WebGPU/GPUBuffer.h Outdated
@@ -90,7 +90,7 @@ class GPUBuffer : public RefCountedAndCanMakeWeakPtr<GPUBuffer> {
const GPUBufferUsageFlags m_usage { 0 };
GPUBufferMapState m_mapState { GPUBufferMapState::Unmapped };
std::optional<MapAsyncPromise> m_pendingMapPromise;
WeakPtr<GPUDevice, WeakPtrImplWithEventTargetData> m_device;
const WeakPtr<GPUDevice, WeakPtrImplWithEventTargetData> m_device;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not protect anything since the type is WeakPtr and WeakPtr does not keep the object alive. This should not help for the purpose of static analysis and if it does, I believe it would likely be a bug in the static analyzer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, but I saw people adding these in other PRs so I figured I'd do it as drive-by as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what's the purpose?

Copy link
Contributor Author

@annevk annevk Jun 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I misunderstood. I'll revert that change and those like it.

@annevk annevk force-pushed the eng/Reduce-unsafeness-in-WebGPU-module branch from aca9e84 to 38ac9df Compare June 30, 2025 15:45
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 30, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 38ac9df)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

charliewolfe
charliewolfe reviewed Jun 30, 2025
View reviewed changes
Source/WebCore/Modules/WebGPU/GPUPresentationContext.cpp Outdated
Comment on lines 67 to 72
RefPtr currentTexture = m_currentTexture;
if ((!currentTexture || currentTexture->isDestroyed()) && m_device.get()) {
if (auto backingCurrentTexture = m_backing->getCurrentTexture(index)) {
currentTexture = GPUTexture::create(*backingCurrentTexture, m_textureDescriptor, *m_device.get()).ptr();
m_currentTexture = currentTexture.copyRef();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could just inline isDestroyed() and avoid doing all of this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out, I'll make that change!

mwyrzykowski
mwyrzykowski reviewed Jun 30, 2025
View reviewed changes
Source/WebCore/Modules/WebGPU/GPUQueue.cpp
if (!nativeImage)
return callback({ }, 0, 0);
RetainPtr platformImage = nativeImage->platformImage();
if (!platformImage)
return callback({ }, 0, 0);
RetainPtr pixelDataCfData = adoptCF(CGDataProviderCopyData(CGImageGetDataProvider(platformImage.get())));
RetainPtr pixelDataCfData = adoptCF(CGDataProviderCopyData(RetainPtr { CGImageGetDataProvider(platformImage.get()) }.get()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does RetainPtr here extend the lifetime of the result returned from GImageGetDataProvider(platformImage.get())? Wouldn't RetainPtr::~RetainPtr run prior to the invocation of CGDataProviderCopyData?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does RetainPtr here extend the lifetime of the result returned from GImageGetDataProvider(platformImage.get())?

Yes.

Wouldn't RetainPtr::~RetainPtr run prior to the invocation of CGDataProviderCopyData?

I do not think so?

@annevk annevk force-pushed the eng/Reduce-unsafeness-in-WebGPU-module branch from 38ac9df to e1f5964 Compare July 1, 2025 07:05
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jul 1, 2025 •
edited
Loading

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

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ❌ 🛠 ios ❌ 🛠 mac ❌ 🛠 wpe ❌ 🛠 win
✅ 🧪 bindings ❌ 🛠 ios-sim ❌ 🛠 mac-AS-debug ❌ 🧪 wpe-wk2 ❌ 🧪 win-tests
✅ 🧪 webkitperl ❌ 🧪 ios-wk2 ❌ 🧪 api-mac ❌ 🧪 api-wpe
❌ 🧪 ios-wk2-wpt ❌ 🧪 mac-wk1 ❌ 🛠 wpe-cairo
❌ 🧪 api-ios ❌ 🧪 mac-wk2 ❌ 🛠 gtk
❌ 🛠 vision ❌ 🧪 mac-AS-debug-wk2 ❌ 🧪 gtk-wk2
❌ 🛠 vision-sim ❌ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
⏳ 🧪 vision-wk2 ❌ 🧪 mac-intel-wk2 ❌ 🛠 playstation
❌ 🛠 tv ❌ 🛠 mac-safer-cpp
❌ 🛠 tv-sim
❌ 🛠 watch
❌ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jul 1, 2025
@annevk annevk removed the merging-blocked Applied to prevent a change from being merged label Jul 1, 2025
@annevk annevk force-pushed the eng/Reduce-unsafeness-in-WebGPU-module branch from e1f5964 to a854904 Compare July 1, 2025 07:42
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jul 1, 2025 •
edited
Loading

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

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe loading 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 loading 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
loading-orange 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision loading-orange 🧪 mac-AS-debug-wk2 loading 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@annevk annevk added the safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks label Jul 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdumez cdumez cdumez left review comments

@charliewolfe charliewolfe charliewolfe left review comments

@mwyrzykowski mwyrzykowski mwyrzykowski approved these changes

@tadeuzagallo tadeuzagallo Awaiting requested review from tadeuzagallo

Assignees

@annevk annevk

Labels
safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit).
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@annevk @webkit-early-warning-system @webkit-ews-buildbot @cdumez @charliewolfe @mwyrzykowski