Ensure glow::Context is current when dropped (#6114)

Cleanup code in glow needs the context to be current (mainly for debug
message callbacks).

See https://docs.rs/glow/0.14.0/glow/trait.HasContext.html#safety

Author: Imberflur

CHANGELOG.md

@@ -69,7 +69,7 @@ By @teoxoy [#6134](https://github.com/gfx-rs/wgpu/pull/6134).
 
 #### Naga
 
-* Support constant evaluation for `firstLeadingBit` and `firstTrailingBit` numeric built-ins in WGSL. Front-ends that translate to these built-ins also benefit from constant evaluation. By @ErichDonGubler in [#5101](https://github.com/gfx-rs/wgpu/pull/5101).
+- Support constant evaluation for `firstLeadingBit` and `firstTrailingBit` numeric built-ins in WGSL. Front-ends that translate to these built-ins also benefit from constant evaluation. By @ErichDonGubler in [#5101](https://github.com/gfx-rs/wgpu/pull/5101).
 
 #### Vulkan
 
@@ -91,8 +91,13 @@ By @teoxoy [#6134](https://github.com/gfx-rs/wgpu/pull/6134).
 - Fix crash when dropping the surface after the device. By @wumpf in [#6052](https://github.com/gfx-rs/wgpu/pull/6052)
 - Fix error message that is thrown in create_render_pass to no longer say `compute_pass`. By @matthew-wong1 [#6041](https://github.com/gfx-rs/wgpu/pull/6041)
 
+#### GLES / OpenGL
+
+- Fix GL debug message callbacks not being properly cleaned up (causing UB). By @Imberflur in [#6114](https://github.com/gfx-rs/wgpu/pull/6114)
+
 ### Changes
 
+- `wgpu_hal::gles::Adapter::new_external` now requires the context to be current when dropping the adapter and related objects. By @Imberflur in [#6114](https://github.com/gfx-rs/wgpu/pull/6114).
 - Reduce the amount of debug and trace logs emitted by wgpu-core and wgpu-hal. By @nical in [#6065](https://github.com/gfx-rs/wgpu/issues/6065)
 - `Rg11b10Float` is renamed to `Rg11b10UFloat`. By @sagudev in [#6108](https://github.com/gfx-rs/wgpu/pull/6108)
 

wgpu-hal/src/gles/egl.rs

@@ -1,8 +1,10 @@
 use glow::HasContext;
 use once_cell::sync::Lazy;
-use parking_lot::{Mutex, MutexGuard, RwLock};
+use parking_lot::{MappedMutexGuard, Mutex, MutexGuard, RwLock};
 
-use std::{collections::HashMap, ffi, os::raw, ptr, rc::Rc, sync::Arc, time::Duration};
+use std::{
+    collections::HashMap, ffi, mem::ManuallyDrop, os::raw, ptr, rc::Rc, sync::Arc, time::Duration,
+};
 
 /// The amount of time to wait while trying to obtain a lock to the adapter context
 const CONTEXT_LOCK_TIMEOUT_SECS: u64 = 1;
@@ -295,6 +297,7 @@ impl EglContext {
             .make_current(self.display, self.pbuffer, self.pbuffer, Some(self.raw))
             .unwrap();
     }
+
     fn unmake_current(&self) {
         self.instance
             .make_current(self.display, None, None, None)
@@ -305,7 +308,7 @@ impl EglContext {
 /// A wrapper around a [`glow::Context`] and the required EGL context that uses locking to guarantee
 /// exclusive access when shared with multiple threads.
 pub struct AdapterContext {
-    glow: Mutex<glow::Context>,
+    glow: Mutex<ManuallyDrop<glow::Context>>,
     egl: Option<EglContext>,
 }
 
@@ -346,14 +349,39 @@ impl AdapterContext {
     }
 }
 
+impl Drop for AdapterContext {
+    fn drop(&mut self) {
+        struct CurrentGuard<'a>(&'a EglContext);
+        impl Drop for CurrentGuard<'_> {
+            fn drop(&mut self) {
+                self.0.unmake_current();
+            }
+        }
+
+        // Context must be current when dropped. See safety docs on
+        // `glow::HasContext`.
+        //
+        // NOTE: This is only set to `None` by `Adapter::new_external` which
+        // requires the context to be current when anything that may be holding
+        // the `Arc<AdapterShared>` is dropped.
+        let _guard = self.egl.as_ref().map(|egl| {
+            egl.make_current();
+            CurrentGuard(egl)
+        });
+        let glow = self.glow.get_mut();
+        // SAFETY: Field not used after this.
+        unsafe { ManuallyDrop::drop(glow) };
+    }
+}
+
 struct EglContextLock<'a> {
     instance: &'a Arc<EglInstance>,
     display: khronos_egl::Display,
 }
 
 /// A guard containing a lock to an [`AdapterContext`]
 pub struct AdapterContextLock<'a> {
-    glow: MutexGuard<'a, glow::Context>,
+    glow: MutexGuard<'a, ManuallyDrop<glow::Context>>,
     egl: Option<EglContextLock<'a>>,
 }
 
@@ -387,10 +415,12 @@ impl AdapterContext {
     ///
     /// > **Note:** Calling this function **will** still lock the [`glow::Context`] which adds an
     /// > extra safe-guard against accidental concurrent access to the context.
-    pub unsafe fn get_without_egl_lock(&self) -> MutexGuard<glow::Context> {
-        self.glow
+    pub unsafe fn get_without_egl_lock(&self) -> MappedMutexGuard<glow::Context> {
+        let guard = self
+            .glow
             .try_lock_for(Duration::from_secs(CONTEXT_LOCK_TIMEOUT_SECS))
-            .expect("Could not lock adapter context. This is most-likely a deadlock.")
+            .expect("Could not lock adapter context. This is most-likely a deadlock.");
+        MutexGuard::map(guard, |glow| &mut **glow)
     }
 
     /// Obtain a lock to the EGL context and get handle to the [`glow::Context`] that can be used to
@@ -1052,6 +1082,8 @@ impl crate::Instance for Instance {
             unsafe { gl.debug_message_callback(super::gl_debug_message_callback) };
         }
 
+        // Avoid accidental drop when the context is not current.
+        let gl = ManuallyDrop::new(gl);
         inner.egl.unmake_current();
 
         unsafe {
@@ -1073,13 +1105,15 @@ impl super::Adapter {
     /// - The underlying OpenGL ES context must be current.
     /// - The underlying OpenGL ES context must be current when interfacing with any objects returned by
     ///   wgpu-hal from this adapter.
+    /// - The underlying OpenGL ES context must be current when dropping this adapter and when
+    ///   dropping any objects returned from this adapter.
     pub unsafe fn new_external(
         fun: impl FnMut(&str) -> *const ffi::c_void,
     ) -> Option<crate::ExposedAdapter<super::Api>> {
         let context = unsafe { glow::Context::from_loader_function(fun) };
         unsafe {
             Self::expose(AdapterContext {
-                glow: Mutex::new(context),
+                glow: Mutex::new(ManuallyDrop::new(context)),
                 egl: None,
             })
         }

wgpu-hal/src/gles/wgl.rs

@@ -9,7 +9,7 @@ use raw_window_handle::{RawDisplayHandle, RawWindowHandle};
 use std::{
     collections::HashSet,
     ffi::{c_void, CStr, CString},
-    mem,
+    mem::{self, ManuallyDrop},
     os::raw::c_int,
     ptr,
     sync::{
@@ -134,11 +134,29 @@ unsafe impl Send for WglContext {}
 unsafe impl Sync for WglContext {}
 
 struct Inner {
-    gl: glow::Context,
+    gl: ManuallyDrop<glow::Context>,
     device: InstanceDevice,
     context: WglContext,
 }
 
+impl Drop for Inner {
+    fn drop(&mut self) {
+        struct CurrentGuard<'a>(&'a WglContext);
+        impl Drop for CurrentGuard<'_> {
+            fn drop(&mut self) {
+                self.0.unmake_current().unwrap();
+            }
+        }
+
+        // Context must be current when dropped. See safety docs on
+        // `glow::HasContext`.
+        self.context.make_current(self.device.dc).unwrap();
+        let _guard = CurrentGuard(&self.context);
+        // SAFETY: Field not used after this.
+        unsafe { ManuallyDrop::drop(&mut self.gl) };
+    }
+}
+
 unsafe impl Send for Inner {}
 unsafe impl Sync for Inner {}
 
@@ -497,6 +515,8 @@ impl crate::Instance for Instance {
             unsafe { gl.debug_message_callback(super::gl_debug_message_callback) };
         }
 
+        // Avoid accidental drop when the context is not current.
+        let gl = ManuallyDrop::new(gl);
         context.unmake_current().map_err(|e| {
             crate::InstanceError::with_source(
                 String::from("unable to unset the current WGL context"),