Skip to content
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.

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

Fix AnyDevice drop implementation dropping the wrong thing #6052

Merged
merged 1 commit into from
Jul 26, 2024

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Jul 26, 2024

Connections

Post-merge edit:

Description
There is a crash report in egui after wgpu update. Quickly realized that this happens only when the surface is destroyed after the all other resources are dropped. Which leads to this simple repro in hello_triangle (cc: @cwfitzgerald any idea how I can unit-testify this? :/)

diff --git a/examples/src/hello_triangle/mod.rs b/examples/src/hello_triangle/mod.rs
index 7c82d49cf..418bf5c66 100644
--- a/examples/src/hello_triangle/mod.rs
+++ b/examples/src/hello_triangle/mod.rs
@@ -1,4 +1,4 @@
-use std::borrow::Cow;
+use std::{borrow::Cow, sync::Arc};
 use winit::{
     event::{Event, WindowEvent},
     event_loop::EventLoop,
@@ -80,6 +80,8 @@ async fn run(event_loop: EventLoop<()>, window: Window) {
         .get_default_config(&adapter, size.width, size.height)
         .unwrap();
     surface.configure(&device, &config);
+    let surface = Arc::new(surface);
+    let surface_b = surface.clone();
 
     let window = &window;
     event_loop
@@ -143,6 +145,8 @@ async fn run(event_loop: EventLoop<()>, window: Window) {
             }
         })
         .unwrap();
+
+    drop(surface_b);
 }
 
 pub fn main() {

Unknown why this happened after wgpu 22.0.0 update, I strongly suspect the error is older and just surfaces (hah!) now because of improving ownership hygiene in the project.

A shining case of "How did this ever work?"
(A: this still drops the right ref count, so this is ONLY an issue if the last droping of Arc<Device<A>> happens through AnyDevice which is surprisingly rare)

Testing
Above manual test works now

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@Wumpf Wumpf requested a review from a team as a code owner July 26, 2024 07:48
@teoxoy teoxoy added the PR: needs back-porting PR with a fix that needs to land on crates label Jul 26, 2024
@Wumpf Wumpf force-pushed the fix-broken-anydevice-drop branch from 52ae707 to ef6d584 Compare July 26, 2024 08:04
@Wumpf
Copy link
Member Author

Wumpf commented Jul 26, 2024

(force push to fill in the changelog pr numbers, no other change)

Wumpf added a commit to emilk/egui that referenced this pull request Jul 26, 2024
@Wumpf Wumpf merged commit d3c38a4 into gfx-rs:trunk Jul 26, 2024
25 checks passed
@Wumpf Wumpf deleted the fix-broken-anydevice-drop branch July 26, 2024 08:31
@Wumpf Wumpf mentioned this pull request Jul 26, 2024
6 tasks
@sagudev
Copy link
Contributor

sagudev commented Jul 26, 2024

any idea how I can unit-testify this?

Simple test that inits every possible resource (device, buffer, texture, ...) and then runs all permutations of drops.

@Wumpf
Copy link
Member Author

Wumpf commented Jul 26, 2024

that would be a good unit test to have regardless indeed! The main problem specifically here though is that we need to create a Surface and we're still having issues to do so on CI afaik

@sagudev
Copy link
Contributor

sagudev commented Jul 26, 2024

Maybe xvfb-run can help (for linux), that's what we use in servo.

cwfitzgerald pushed a commit to cwfitzgerald/wgpu that referenced this pull request Jul 31, 2024
486c pushed a commit to 486c/egui that referenced this pull request Oct 9, 2024
hacknus pushed a commit to hacknus/egui that referenced this pull request Oct 30, 2024
@Wumpf Wumpf removed the PR: needs back-porting PR with a fix that needs to land on crates label Nov 23, 2024
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.

3 participants