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
Merged
merged 1 commit into from
Jul 1, 2025
Merged

Reduce unsafeness in WebGPU module #47342

merged 1 commit into from
Jul 1, 2025
+85 −91

Conversation

annevk
Copy link
Contributor

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

24972cf

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

Reviewed by Mike Wyrzykowski.

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

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

a854904

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
✅ 🛠 🧪 merge ✅ 🧪 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
@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 ✅ 🛠 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
✅ 🛠 🧪 merge ✅ 🧪 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
@webkit-ews-buildbot webkit-ews-buildbot added merge-queue Applied to send a pull request to merge-queue and removed safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks labels Jul 1, 2025
@webkit-ews-buildbot
Copy link
Collaborator

Safe-Merge-Queue: Build #61938.

@webkit-commit-queue webkit-commit-queue force-pushed the eng/Reduce-unsafeness-in-WebGPU-module branch from a854904 to 24972cf Compare July 1, 2025 11:07
@webkit-commit-queue
Copy link
Collaborator

Committed 296859@main (24972cf): https://commits.webkit.org/296859@main

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

@webkit-commit-queue webkit-commit-queue merged commit 24972cf into WebKit:main Jul 1, 2025
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jul 1, 2025
@annevk annevk deleted the eng/Reduce-unsafeness-in-WebGPU-module branch July 1, 2025 12:15
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
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.
7 participants
@annevk @webkit-early-warning-system @webkit-ews-buildbot @webkit-commit-queue @cdumez @charliewolfe @mwyrzykowski