Prevent Metal from crashing when a still-open encoder is deallocated, resolve issue #2077.

CHANGELOG.md

@@ -40,6 +40,20 @@ Bottom level categories:
 
 ## Unreleased
 
+### Bug Fixes
+
+#### DX12
+
+- Ensure that CommandAllocator closes the command list before it is reset.
+
+By @bradwerth in [#4203](https://github.com/gfx-rs/wgpu/pull/4023)
+
+#### Metal
+
+- Ensure that MTLCommandEncoder calls endEncoding before it is deallocated.
+
+By @bradwerth in [#4203](https://github.com/gfx-rs/wgpu/pull/4023)
+
 ### Major changes
 
 #### Pass timestamp queries
@@ -111,7 +125,7 @@ This release was fairly minor as breaking changes go.
 
 #### `wgpu` types now `!Send` `!Sync` on wasm
 
-Up until this point, wgpu has made the assumption that threads do not exist on wasm. With the rise of libraries like [`wasm_thread`](https://crates.io/crates/wasm_thread) making it easier and easier to do wasm multithreading this assumption is no longer sound. As all wgpu objects contain references into the JS heap, they cannot leave the thread they started on. 
+Up until this point, wgpu has made the assumption that threads do not exist on wasm. With the rise of libraries like [`wasm_thread`](https://crates.io/crates/wasm_thread) making it easier and easier to do wasm multithreading this assumption is no longer sound. As all wgpu objects contain references into the JS heap, they cannot leave the thread they started on.
 
 As we understand that this change might be very inconvenient for users who don't care about wasm threading, there is a crate feature which re-enables the old behavior: `fragile-send-sync-non-atomic-wasm`. So long as you don't compile your code with `-Ctarget-feature=+atomics`, `Send` and `Sync` will be implemented again on wgpu types on wasm. As the name implies, especially for libraries, this is very fragile, as you don't know if a user will want to compile with atomics (and therefore threads) or not.
 

tests/tests/encoder.rs

@@ -1,5 +1,6 @@
 use wasm_bindgen_test::*;
-use wgpu_test::{initialize_test, TestParameters};
+use wgpu::RenderPassDescriptor;
+use wgpu_test::{fail, initialize_test, TestParameters};
 
 #[test]
 #[wasm_bindgen_test]
@@ -11,3 +12,51 @@ fn drop_encoder() {
         drop(encoder);
     })
 }
+
+#[test]
+fn drop_encoder_after_error() {
+    initialize_test(TestParameters::default(), |ctx| {
+        let mut encoder = ctx
+            .device
+            .create_command_encoder(&wgpu::CommandEncoderDescriptor::default());
+
+        let target_tex = ctx.device.create_texture(&wgpu::TextureDescriptor {
+            label: None,
+            size: wgpu::Extent3d {
+                width: 100,
+                height: 100,
+                depth_or_array_layers: 1,
+            },
+            mip_level_count: 1,
+            sample_count: 1,
+            dimension: wgpu::TextureDimension::D2,
+            format: wgpu::TextureFormat::R8Unorm,
+            usage: wgpu::TextureUsages::RENDER_ATTACHMENT,
+            view_formats: &[],
+        });
+        let target_view = target_tex.create_view(&wgpu::TextureViewDescriptor::default());
+
+        let mut renderpass = encoder.begin_render_pass(&RenderPassDescriptor {
+            label: Some("renderpass"),
+            color_attachments: &[Some(wgpu::RenderPassColorAttachment {
+                ops: wgpu::Operations::default(),
+                resolve_target: None,
+                view: &target_view,
+            })],
+            depth_stencil_attachment: None,
+            timestamp_writes: None,
+            occlusion_query_set: None,
+        });
+
+        // Set a bad viewport on renderpass, triggering an error.
+        fail(&ctx.device, || {
+            renderpass.set_viewport(0.0, 0.0, -1.0, -1.0, 0.0, 1.0);
+            drop(renderpass);
+        });
+
+        // This is the actual interesting error condition. We've created
+        // a CommandEncoder which errored out when processing a command.
+        // The encoder is still open!
+        drop(encoder);
+    })
+}

wgpu-hal/src/dx12/command.rs

@@ -1,4 +1,5 @@
 use crate::auxil::{self, dxgi::result::HResult as _};
+use crate::CommandEncoder as _;
 
 use super::conv;
 use std::{mem, ops::Range, ptr};
@@ -1192,3 +1193,13 @@ impl crate::CommandEncoder<super::Api> for super::CommandEncoder {
         };
     }
 }
+
+impl Drop for super::CommandEncoder {
+    fn drop(&mut self) {
+        // DX12 throws an exception when an encoder is dropped without being closed.
+        // To prevent this, we explicitiy call discard_encoding.
+        unsafe {
+            self.discard_encoding();
+        }
+    }
+}

wgpu-hal/src/metal/command.rs

@@ -1,4 +1,5 @@
 use super::{conv, AsNative};
+use crate::CommandEncoder as _;
 use std::{borrow::Cow, mem, ops::Range};
 
 // has to match `Temp::binding_sizes`
@@ -1054,3 +1055,17 @@ impl crate::CommandEncoder<super::Api> for super::CommandEncoder {
         encoder.dispatch_thread_groups_indirect(&buffer.raw, offset, self.state.raw_wg_size);
     }
 }
+
+impl Drop for super::CommandEncoder {
+    fn drop(&mut self) {
+        // Metal raises an assert when a MTLCommandEncoder is deallocated without a call
+        // to endEncoding. This isn't documented at
+        // https://developer.apple.com/documentation/metal/mtlcommandencoder?language=objc
+        // but it manifests as a crash with the message 'Command encoder released without
+        // endEncoding'. To prevent this, we explicitiy call discard_encoding, which
+        // calls end_encoding on any still-held metal::CommandEncoders.
+        unsafe {
+            self.discard_encoding();
+        }
+    }
+}