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

CHANGELOG.md

@@ -99,6 +99,10 @@ By @Valaphee in [#3402](https://github.com/gfx-rs/wgpu/pull/3402)
 - DX12 doesn't support `Features::POLYGON_MODE_POINT``. By @teoxoy in [#4032](https://github.com/gfx-rs/wgpu/pull/4032).
 - Set `Features::VERTEX_WRITABLE_STORAGE` based on the right feature level. By @teoxoy in [#4033](https://github.com/gfx-rs/wgpu/pull/4033).
 
+#### Metal
+
+- Ensure that MTLCommandEncoder calls endEncoding before it is deallocated. By @bradwerth in [#4023](https://github.com/gfx-rs/wgpu/pull/4023)
+
 ### Documentation
 
 - Add an overview of `RenderPass` and how render state works. By @kpreid in [#4055](https://github.com/gfx-rs/wgpu/pull/4055)
@@ -113,7 +117,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,59 @@ fn drop_encoder() {
         drop(encoder);
     })
 }
+
+#[test]
+fn drop_encoder_after_error() {
+    // This test crashes on DX12 with the exception:
+    //
+    // ID3D12CommandAllocator::Reset: The command allocator cannot be reset because a
+    // command list is currently being recorded with the allocator. [ EXECUTION ERROR
+    // #543: COMMAND_ALLOCATOR_CANNOT_RESET]
+    //
+    // For now, we mark the test as failing on DX12.
+    let parameters = TestParameters::default().backend_failure(wgpu::Backends::DX12);
+    initialize_test(parameters, |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/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,20 @@ 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 in the general case at
+        // https://developer.apple.com/documentation/metal/mtlcommandencoder, but for the
+        // more-specific MTLComputeCommandEncoder it is stated as a requirement at
+        // https://developer.apple.com/documentation/metal/mtlcomputecommandencoder. It
+        // appears to be a requirement for all MTLCommandEncoder objects. Failing to call
+        // endEncoding causes 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();
+        }
+    }
+}