Dropping a failed QuerySet within an error scope panics

[jimblandy]

The following test fails if added to tests/tests:

use wgpu_test::{initialize_test, TestParameters};

#[test]
fn drop_failed_timestamp_query_set() {
    initialize_test(TestParameters::default(), |ctx| {
        // Enter an error scope, so the validation catch-all doesn't
        // report the error too early.
        ctx.device.push_error_scope(wgpu::ErrorFilter::Validation);

        // Creating this query set should fail, since we didn't include
        // TIMESTAMP_QUERY in our required features.
        let bad_query_set = ctx.device.create_query_set(&wgpu::QuerySetDescriptor {
            label: Some("doomed query set"),
            ty: wgpu::QueryType::Timestamp,
            count: 1,
        });

        // Dropping this should not panic.
        drop(bad_query_set);

        assert!(pollster::block_on(ctx.device.pop_error_scope()).is_some());
    });
}

Instead of simply reporting the failure to create the QuerySet when we exit the error scope, drop(bad_query_set) causes a panic trying to access the registry:

thread 'query_set::drop_failed_timestamp_query_set' panicked at 'called `Result::unwrap()` on an `Err` value: InvalidId', /home/jimb/rust/wgpu/wgpu-core/src/device/global.rs:1702:67
stack backtrace:
...
   2: core::result::unwrap_failed
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/result.rs:1785:5
   3: core::result::Result<T,E>::unwrap
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/result.rs:1107:23
   4: wgpu_core::device::global::<impl wgpu_core::global::Global<G>>::query_set_drop
             at /home/jimb/rust/wgpu/wgpu-core/src/device/global.rs:1702:29
   5: <wgpu::backend::direct::Context as wgpu::context::Context>::query_set_drop
             at /home/jimb/rust/wgpu/wgpu/src/backend/direct.rs:1630:9
   6: <T as wgpu::context::DynContext>::query_set_drop
             at /home/jimb/rust/wgpu/wgpu/src/context.rs:2578:9
   7: <wgpu::QuerySet as core::ops::drop::Drop>::drop
             at /home/jimb/rust/wgpu/wgpu/src/lib.rs:890:13
   8: core::ptr::drop_in_place<wgpu::QuerySet>
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/ptr/mod.rs:487:1
   9: core::mem::drop
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/mem/mod.rs:988:24
  10: wgpu_tests::query_set::drop_failed_timestamp_query_set::{{closure}}
             at ./tests/query_set.rs:19:9
  11: wgpu_test::initialize_test::{{closure}}
             at ./src/lib.rs:410:53
... catch-unwind frames
  17: wgpu_test::initialize_test
             at ./src/lib.rs:410:20
  18: wgpu_tests::query_set::drop_failed_timestamp_query_set
             at ./tests/query_set.rs:5:5
  19: wgpu_tests::query_set::drop_failed_timestamp_query_set::{{closure}}
             at ./tests/query_set.rs:4:1
  20: core::ops::function::FnOnce::call_once
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/ops/function.rs:248:5
  21: core::ops::function::FnOnce::call_once
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/ops/function.rs:248:5

[jimblandy]

It's typical for a function to call pop_error_scope, note the failure, and pass the wgpu::Error along to its caller. This will often cause the QuerySet to be dropped, so the panic ends up preventing us from reporting the error in a helpful way.

[jimblandy]

I've published a branch containing the test case to https://github.com/jimblandy/wgpu/tree/issue-4139-test. Check out that branch and reproduce with:

$ RUST_BACKTRACE=1 cargo nextest run -p wgpu-test --test wgpu-tests query_set

[cwfitzgerald]

Could you PR the test with it marked as should fail? In this case it is easy to copy in, but I think this is a good habit to get into.

[gents83]

This is not panicking anymore with #3626 👍