[metal]: Fix double-free when re-using layer

Author: madsmtm

wgpu-hal/src/metal/surface.rs

@@ -1,6 +1,7 @@
 #![allow(clippy::let_unit_value)] // `let () =` being used to constrain result type
 
 use std::ffi::c_uint;
+use std::mem::ManuallyDrop;
 use std::ptr::NonNull;
 use std::sync::Once;
 use std::thread;
@@ -14,7 +15,7 @@ use objc::{
     class,
     declare::ClassDecl,
     msg_send,
-    rc::autoreleasepool,
+    rc::{autoreleasepool, StrongPtr},
     runtime::{Class, Object, Sel, BOOL, NO, YES},
     sel, sel_impl,
 };
@@ -81,7 +82,9 @@ impl super::Surface {
         delegate: Option<&HalManagedMetalLayerDelegate>,
     ) -> Self {
         let layer = unsafe { Self::get_metal_layer(view, delegate) };
-        // SAFETY: The layer is an initialized instance of `CAMetalLayer`.
+        let layer = ManuallyDrop::new(layer);
+        // SAFETY: The layer is an initialized instance of `CAMetalLayer`, and
+        // we transfer the retain count to `MetalLayer` using `ManuallyDrop`.
         let layer = unsafe { metal::MetalLayer::from_ptr(layer.cast()) };
         let view: *mut Object = msg_send![view.as_ptr(), retain];
         let view = NonNull::new(view).expect("retain should return the same object");
@@ -108,7 +111,7 @@ impl super::Surface {
     pub(crate) unsafe fn get_metal_layer(
         view: NonNull<Object>,
         delegate: Option<&HalManagedMetalLayerDelegate>,
-    ) -> *mut Object {
+    ) -> StrongPtr {
         let is_main_thread: BOOL = msg_send![class!(NSThread), isMainThread];
         if is_main_thread == NO {
             panic!("get_metal_layer cannot be called in non-ui thread.");
@@ -142,7 +145,7 @@ impl super::Surface {
             // render directly into that; after all, the user passed a view
             // with an explicit Metal layer to us, so this is very likely what
             // they expect us to do.
-            root_layer
+            unsafe { StrongPtr::retain(root_layer) }
         } else {
             // The view does not have a `CAMetalLayer` as the root layer (this
             // is the default for most views).
@@ -247,7 +250,7 @@ impl super::Surface {
             if let Some(delegate) = delegate {
                 let () = msg_send![new_layer, setDelegate: delegate.0];
             }
-            new_layer
+            unsafe { StrongPtr::new(new_layer) }
         }
     }
 

wgpu-hal/src/vulkan/instance.rs

@@ -515,13 +515,16 @@ impl super::Instance {
         }
 
         let layer = unsafe { crate::metal::Surface::get_metal_layer(view.cast(), None) };
+        // NOTE: The layer is retained by Vulkan's `vkCreateMetalSurfaceEXT`,
+        // so no need to retain it beyond the scope of this function.
+        let layer_ptr = (*layer).cast();
 
         let surface = {
             let metal_loader =
                 ext::metal_surface::Instance::new(&self.shared.entry, &self.shared.raw);
             let vk_info = vk::MetalSurfaceCreateInfoEXT::default()
                 .flags(vk::MetalSurfaceCreateFlagsEXT::empty())
-                .layer(layer.cast());
+                .layer(layer_ptr);
 
             unsafe { metal_loader.create_metal_surface(&vk_info, None).unwrap() }
         };