Remove deadlock potential from UncapturedErrorHandler.

CHANGELOG.md

@@ -72,6 +72,9 @@ By @Vecvec in [#7913](https://github.com/gfx-rs/wgpu/pull/7913).
   - Copies of depth/stencil formats must be 4B aligned.
 - 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
 

examples/standalone/custom_backend/src/custom.rs

@@ -240,7 +240,7 @@ impl DeviceInterface for CustomDevice {
         unimplemented!()
     }
 
-    fn on_uncaptured_error(&self, _handler: Box<dyn wgpu::UncapturedErrorHandler>) {
+    fn on_uncaptured_error(&self, _handler: Arc<dyn wgpu::UncapturedErrorHandler>) {
         unimplemented!()
     }
 

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/api/device.rs

@@ -407,8 +407,8 @@ impl Device {
         QuerySet { inner: query_set }
     }
 
-    /// Set a callback for errors that are not handled in error scopes.
-    pub fn on_uncaptured_error(&self, handler: Box<dyn UncapturedErrorHandler>) {
+    /// Set a callback which will be called for all errors that are not handled in error scopes.
+    pub fn on_uncaptured_error(&self, handler: Arc<dyn UncapturedErrorHandler>) {
         self.inner.on_uncaptured_error(handler)
     }
 
@@ -714,9 +714,11 @@ impl From<wgc::instance::RequestDeviceError> for RequestDeviceError {
     }
 }
 
-/// Type for the callback of uncaptured error handler
-pub trait UncapturedErrorHandler: Fn(Error) + Send + 'static {}
-impl<T> UncapturedErrorHandler for T where T: Fn(Error) + Send + 'static {}
+/// The callback of [`Device::on_uncaptured_error()`].
+///
+/// It must be a function with this signature.
+pub trait UncapturedErrorHandler: Fn(Error) + Send + Sync + 'static {}
+impl<T> UncapturedErrorHandler for T where T: Fn(Error) + Send + Sync + 'static {}
 
 /// Kinds of [`Error`]s a [`Device::push_error_scope()`] may be configured to catch.
 #[derive(Clone, Copy, Debug, Eq, PartialEq, PartialOrd)]

wgpu/src/backend/webgpu.rs

@@ -10,6 +10,7 @@ use alloc::{
     format,
     rc::Rc,
     string::{String, ToString as _},
+    sync::Arc,
     vec,
     vec::Vec,
 };
@@ -2409,7 +2410,7 @@ impl dispatch::DeviceInterface for WebDevice {
         closure.forget();
     }
 
-    fn on_uncaptured_error(&self, handler: Box<dyn crate::UncapturedErrorHandler>) {
+    fn on_uncaptured_error(&self, handler: Arc<dyn crate::UncapturedErrorHandler>) {
         let f = Closure::wrap(Box::new(move |event: webgpu_sys::GpuUncapturedErrorEvent| {
             let error = crate::Error::from_js(event.error().value_of());
             handler(error);

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]
@@ -617,7 +626,7 @@ struct ErrorScope {
 
 struct ErrorSinkRaw {
     scopes: Vec<ErrorScope>,
-    uncaptured_handler: Option<Box<dyn crate::UncapturedErrorHandler>>,
+    uncaptured_handler: Option<Arc<dyn crate::UncapturedErrorHandler>>,
 }
 
 impl ErrorSinkRaw {
@@ -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");
 }
@@ -1757,7 +1778,7 @@ impl dispatch::DeviceInterface for CoreDevice {
             .device_set_device_lost_closure(self.id, device_lost_callback);
     }
 
-    fn on_uncaptured_error(&self, handler: Box<dyn crate::UncapturedErrorHandler>) {
+    fn on_uncaptured_error(&self, handler: Arc<dyn crate::UncapturedErrorHandler>) {
         let mut error_sink = self.error_sink.lock();
         error_sink.uncaptured_handler = Some(handler);
     }

wgpu/src/dispatch.rs

@@ -185,7 +185,7 @@ pub trait DeviceInterface: CommonTraits {
 
     fn set_device_lost_callback(&self, device_lost_callback: BoxDeviceLostCallback);
 
-    fn on_uncaptured_error(&self, handler: Box<dyn crate::UncapturedErrorHandler>);
+    fn on_uncaptured_error(&self, handler: Arc<dyn crate::UncapturedErrorHandler>);
     fn push_error_scope(&self, filter: crate::ErrorFilter);
     fn pop_error_scope(&self) -> Pin<Box<dyn PopErrorScopeFuture>>;