Skip to content

fix(vulkan): drop implementation panic for surface texture#9678

Open
hack3rmann wants to merge 9 commits into
gfx-rs:trunkfrom
hack3rmann:fix/destructor-panic-in-surface-texture-drop-when-panicking
Open

fix(vulkan): drop implementation panic for surface texture#9678
hack3rmann wants to merge 9 commits into
gfx-rs:trunkfrom
hack3rmann:fix/destructor-panic-in-surface-texture-drop-when-panicking

Conversation

@hack3rmann

@hack3rmann hack3rmann commented Jun 15, 2026

Copy link
Copy Markdown

Connections
Fixes #8243

Description
During panic unwind, we release the acquired_texture slot so the texture (and its SwapchainAcquireSemaphore Arc) is dropped, without calling HAL discard.
That fixes "Trying to destroy a SwapchainAcquireSemaphore that is still in use by a SurfaceTexture" error that causes a secondary panic during unwind and abort the process.

Testing
The original SurfaceTexture::drop is the same when there's no panic.
When a thread is panicking, SurfaceTexture::drop now drops the second reference Arc<SwapchainAcquireSemaphore>. After that there will be only one reference to the semaphores. So they can be dropped correctly.
I don't think that automated regression test is possible for that fix. But it's pretty niche: only covers that panicking case, where there's already something isn't right. I've tested the panicking case on my machine and the fix is integrated in my engine, that uses wgpu for rendering.

Squash or Rebase?

Squash. The changes are minimal.

Checklist

  • I self-reviewed and fully understand this PR.
  • WebGPU implementations built with wgpu may be affected behaviorally.
  • Validation and feature gates are in place to confine behavioral changes.
  • Tests demonstrate the validation and altered logic works. See Testing section
  • CHANGELOG.md entries for the user-facing effects of this change are present.
  • The PR is minimal, and doesn't make sense to land as multiple PRs.
  • Commits are logically scoped and individually reviewable.
  • The PR description has enough context to understand the motivation and solution implemented.

@hack3rmann hack3rmann marked this pull request as ready for review June 15, 2026 11:17
@hack3rmann

Copy link
Copy Markdown
Author

I've merged the new changes from the trunk branch. There was one minor conflict: Action::DestroyBindGroupLayout was renamed to Action::DropBindGroupLayout and that's it. I'm pretty sure that I've resolved that correctly

I've reviewed the failing CI checks. The failing cases are:

  1. CI / Test Linux x86_64 (pull_request): fail in wgpu_examples::timestamp_queries::tests::timestamps_passes
  2. CI / Test Windows x86_64 (pull_request): wgpu_gpu::clear_texture::clear_texture_compressed_bcn, wgpu_gpu::timestamp_normalization::utils::shift_right_u96, wgpu_gpu::timestamp_normalization::utils::u64_mul_u32 and test_api are timed out
  3. CTS / API Validation Windows x86_64 (pull_request):
Error: CTS failed
error: process didn't exit successfully: `target\debug\wgpu-xtask.exe cts --llvm-cov --backend dx12 --filter '^webgpu:api,validation,'` (exit code: 1)

The issues 1 and 2 are unrelated, because this PR doesn't touch the code in non-panicking code path.
The issue 3 is just weird. I went through the generated report and there was 0 failed cases. I wonder if that test case itself could have issues, because the process is exited with code 1 when none of the test cases failed.

@andyleiserson

Copy link
Copy Markdown
Contributor

Hopefully if you merge trunk again, the CI failures will be resolved.

@hack3rmann hack3rmann force-pushed the fix/destructor-panic-in-surface-texture-drop-when-panicking branch from 935fb20 to 914ebdc Compare June 20, 2026 18:31
@hack3rmann

Copy link
Copy Markdown
Author

Thank you! All checks have passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Panicing while surface texture acquired causes a panic while unwinding

2 participants