`LoadOpDontCare` should not implement `Default`

[jimblandy]

The point of LoadOpDontCare is that you can't obtain an instance of this value without invoking the unsafe function LoadOpDontCare::enable, so that you cannot use LoadOp::DontCare without using an unsafe block, to acknowledge the additional obligations you accept by using this feature.

But if LoadOpDontCare implements Default, then you can just call LoadOpDontCare::default and go on your merry way, no unsafe block required. For example, this test still compiles fine changed like so:

modified   tests/tests/wgpu-gpu/pass_ops/mod.rs
@@ -89,7 +89,7 @@ async fn run_test(ctx: TestingContext) {
             depth_slice: None,
             resolve_target: None,
             ops: wgpu::Operations {
-                load: wgpu::LoadOp::DontCare(unsafe { wgpu::LoadOpDontCare::enabled() }),
+                load: wgpu::LoadOp::DontCare(wgpu::LoadOpDontCare::default()),
                 store: wgpu::StoreOp::Store,
             },
         })],

There's probably some detail I'm missing, but I suspect that someone slapped Default in there because otherwise serde was unhappy trying to derive deserialization for LoadOp: the DontCare variant is defined like this:

    DontCare(#[cfg_attr(feature = "serde", serde(skip))] LoadOpDontCare) = 2,

and the serde(skip) attribute requires that the field's type derive Default.

For Firefox's sake, since LoadOp::DontCare is not part of WebGPU and is unlikely to become so, it would be better to simply not allow deserialization of DontCare at all.

For record and replay, I don't know what to do about this. As long as there is any way to deserialize this, then one can produce LoadOpDontCare tokens in safe code just by deserializing some JSON. I don't care about this if someone just does it deliberately because they hate unsafe blocks. But it does trouble me that permitting deserialization means that anything that consumes serialized wgpu stuff needs to be prepared for undefined behavior.

cc @cwfitzgerald

[jimblandy]

Marking as P1 for Firefox just because we have to include special code to block it in the GPU process, and we just shouldn't have to think about it at all.

[cwfitzgerald]

I wonder if we need to implement deserialize to deserialize DontCare to Clear. This will limit replay from capturing the exact behavior of the original code, which could mask issues.

I'm torn on what to do about this

[cwfitzgerald]

Well actually if we consider this unsafe in the first place, then yes we must remove the default impl and avoid deserializing this.

Separately, I do have some worries that this token pattern might have issues when extended to others potential future unsafe operations. I'm not sure what better options there are though.

[jimblandy]

One option which would work for Firefox and simplify serialization would be:

• Remove the LoadOpDontCare token; let people just say LoadOp::DontCare without further fanfare.
• Make don't-care attachments a native-only feature (that perhaps requires an unsafe block to enable).
• Have wgpu-core validate out render passes that try to use LoadOp::DontCare unless the feature is enabled.

Firefox already has to filter out non-standard features in what adapters offer and when creating devices. This would let us rely on wgpu-core to catch a corrupt content process trying to use DontCare.

[cwfitzgerald]

Took a shower and got an idea. Our goals are

1. Try to keep unsafe as local as possible in most situations.
2. Allow Firefox to deserialize confidently
3. Allow the player to deserialize unsafely and faithfully
4. Remove the soundness hole of deserialization.

---

1. Make the LoadOpDontCare token contain a Boolean. Defaulting/deserializing sets this false. Unsafe assertion method sets this true.
2. Add a DeserializeUnsafe Boolean token to the device descriptor.
3. When validating the DontCare, check that either the DontCare token or the Deserialize token is in the activated state. If not, raise a validation error.

This means that natively, the current local unsafe works. Firefox will set DeserializeUnsafe to false, meaning deserialized DontCares are validated against. The player can set it to true, allowing deserialized unsafe operations.

This should get us all of our goals.

cc @kpreid as you might have some thoughts.

[jimblandy]

That does meet everyone's needs. Agreed that it's good to have the unsafe near the code that ensures its requirements are met. And I guess the code that creates a render pass descriptor would indeed usually be near the code that builds the render pass, which is what ensures that the attachment gets drawn to.

But it seems pretty elaborate. I'd feel a little bad for the next person who has to figure out why it's set up like this.