Call uncaptured error handler without the lock held.

This prevents a possible deadlock if the callback itself calls wgpu
operations.

Author: kpreid

CHANGELOG.md

@@ -73,6 +73,7 @@ By @Vecvec in [#7913](https://github.com/gfx-rs/wgpu/pull/7913).
 - The offset for `set_vertex_buffer` and `set_index_buffer` must be 4B aligned. By @andyleiserson in [#7929](https://github.com/gfx-rs/wgpu/pull/7929).
 - The offset and size of bindings are validated as fitting within the underlying buffer in more cases. By @andyleiserson in [#7911](https://github.com/gfx-rs/wgpu/pull/7911).
 - The function you pass to `Device::on_uncaptured_error()` must now implement `Sync` in addition to `Send`, and be wrapped in `Arc` instead of `Box`.
+  In exchange for this, it is no longer possible for calling `wgpu` functions while in that callback to cause a deadlock (not that we encourage you to actually do that).
   By @kpreid in [#8011](https://github.com/gfx-rs/wgpu/pull/8011).
 
 #### Naga

tests/tests/wgpu-validation/api/device.rs

@@ -0,0 +1,44 @@
+/// Test that if another error occurs reentrantly in the [`wgpu::Device::on_uncaptured_error`]
+/// handler, this does not result in a deadlock (as a previous implementation would have had).
+#[cfg(not(target_family = "wasm"))] // test needs wgpu::Device: Send + Sync to achieve reentrance
+#[test]
+fn recursive_uncaptured_error() {
+    use std::sync::atomic::{AtomicU32, Ordering::Relaxed};
+    use std::sync::Arc;
+
+    const ERRONEOUS_TEXTURE_DESC: wgpu::TextureDescriptor = wgpu::TextureDescriptor {
+        label: None,
+        size: wgpu::Extent3d {
+            width: 1,
+            height: 1,
+            depth_or_array_layers: 10,
+        },
+        mip_level_count: 0,
+        sample_count: 0,
+        dimension: wgpu::TextureDimension::D2,
+        format: wgpu::TextureFormat::Rgba8UnormSrgb,
+        usage: wgpu::TextureUsages::COPY_SRC,
+        view_formats: &[],
+    };
+
+    let (device, _queue) = wgpu::Device::noop(&wgpu::DeviceDescriptor::default());
+
+    let errors_seen: Arc<AtomicU32> = Arc::default();
+    let handler = Arc::new({
+        let errors_seen = errors_seen.clone();
+        let device = device.clone();
+        move |_error| {
+            let previous_count = errors_seen.fetch_add(1, Relaxed);
+            if previous_count == 0 {
+                // Trigger another error recursively
+                _ = device.create_texture(&ERRONEOUS_TEXTURE_DESC);
+            }
+        }
+    });
+
+    // Trigger one error which will call the handler
+    device.on_uncaptured_error(handler);
+    _ = device.create_texture(&ERRONEOUS_TEXTURE_DESC);
+
+    assert_eq!(errors_seen.load(Relaxed), 2);
+}

tests/tests/wgpu-validation/api/mod.rs

@@ -1,6 +1,7 @@
 mod binding_arrays;
 mod buffer;
 mod buffer_slice;
+mod device;
 mod external_texture;
 mod instance;
 mod texture;

wgpu/src/backend/wgpu_core.rs

@@ -281,27 +281,36 @@ impl ContextWgpuCore {
             source,
             label: label.unwrap_or_default().to_string(),
         });
-        let mut sink = sink_mutex.lock();
-        let description = || self.format_error(&*source);
-        let error = match error_type {
-            ErrorType::Internal => {
-                let description = description();
-                crate::Error::Internal {
-                    source,
-                    description,
+        let final_error_handling = {
+            let mut sink = sink_mutex.lock();
+            let description = || self.format_error(&*source);
+            let error = match error_type {
+                ErrorType::Internal => {
+                    let description = description();
+                    crate::Error::Internal {
+                        source,
+                        description,
+                    }
                 }
-            }
-            ErrorType::OutOfMemory => crate::Error::OutOfMemory { source },
-            ErrorType::Validation => {
-                let description = description();
-                crate::Error::Validation {
-                    source,
-                    description,
+                ErrorType::OutOfMemory => crate::Error::OutOfMemory { source },
+                ErrorType::Validation => {
+                    let description = description();
+                    crate::Error::Validation {
+                        source,
+                        description,
+                    }
                 }
-            }
-            ErrorType::DeviceLost => return, // will be surfaced via callback
+                ErrorType::DeviceLost => return, // will be surfaced via callback
+            };
+            sink.handle_error_or_return_handler(error)
         };
-        sink.handle_error(error);
+
+        if let Some(f) = final_error_handling {
+            // If the user has provided their own `uncaptured_handler` callback, invoke it now,
+            // having released our lock on `sink_mutex`. See the comments on
+            // `handle_error_or_return_handler` for details.
+            f();
+        }
     }
 
     #[inline]
@@ -628,8 +637,18 @@ impl ErrorSinkRaw {
         }
     }
 
+    /// Deliver the error to
+    ///
+    /// * the innermost error scope, if any, or
+    /// * the uncaptured error handler, if there is one, or
+    /// * [`default_error_handler()`].
+    ///
+    /// If a closure is returned, the caller should call it immediately after dropping the
+    /// [`ErrorSink`] mutex guard. This makes sure that the user callback is not called with
+    /// a wgpu mutex held.
     #[track_caller]
-    fn handle_error(&mut self, err: crate::Error) {
+    #[must_use]
+    fn handle_error_or_return_handler(&mut self, err: crate::Error) -> Option<impl FnOnce()> {
         let filter = match err {
             crate::Error::OutOfMemory { .. } => crate::ErrorFilter::OutOfMemory,
             crate::Error::Validation { .. } => crate::ErrorFilter::Validation,
@@ -645,13 +664,15 @@ impl ErrorSinkRaw {
                 if scope.error.is_none() {
                     scope.error = Some(err);
                 }
+                None
             }
             None => {
-                if let Some(custom_handler) = self.uncaptured_handler.as_ref() {
-                    (custom_handler)(err);
+                if let Some(custom_handler) = &self.uncaptured_handler {
+                    let custom_handler = Arc::clone(custom_handler);
+                    Some(move || (custom_handler)(err))
                 } else {
                     // direct call preserves #[track_caller] where dyn can't
-                    default_error_handler(err);
+                    default_error_handler(err)
                 }
             }
         }
@@ -665,7 +686,7 @@ impl fmt::Debug for ErrorSinkRaw {
 }
 
 #[track_caller]
-fn default_error_handler(err: crate::Error) {
+fn default_error_handler(err: crate::Error) -> ! {
     log::error!("Handling wgpu errors as fatal by default");
     panic!("wgpu error: {err}\n");
 }