fix: parse dawnToggles properly#379
Merged
wcandillon merged 4 commits intoJun 3, 2026
Merged
Conversation
wcandillon
reviewed
Jun 2, 2026
Owner
wcandillon
left a comment
There was a problem hiding this comment.
Thanks for doing this. Now we didn't catch this ealier because we didn't try any valid toggles. Could you add the following test if sensible:
diff --git a/packages/webgpu/src/__tests__/DawnToggles.spec.ts b/packages/webgpu/src/__tests__/DawnToggles.spec.ts
index 574a985..96900ac 100644
--- a/packages/webgpu/src/__tests__/DawnToggles.spec.ts
+++ b/packages/webgpu/src/__tests__/DawnToggles.spec.ts
@@ -38,4 +38,59 @@ describe("Dawn toggles", () => {
});
expect(result).toBe(true);
});
+
+ // The tests above only assert that requestDevice resolves, which is true
+ // whether or not the toggle is parsed and chained onto the device descriptor.
+ // This test instead activates a real toggle (skip_validation) and observes a
+ // behavioral difference, so it fails if dawnToggles ever stops being applied.
+ it("applies skip_validation so a normally-invalid buffer creates without error", async () => {
+ // dawnToggles is a non-standard Dawn extension; browsers ignore it, so the
+ // behavioral difference only exists on the native (Dawn) backends.
+ if (client.OS === "web") {
+ return;
+ }
+ const result = await client.eval(({ gpu }) => {
+ // MAP_READ may only be combined with COPY_DST and MAP_WRITE only with
+ // COPY_SRC, so MAP_READ | MAP_WRITE is a validation error by default. The
+ // buffer is never used on the GPU, so creating it with validation skipped
+ // is harmless on every backend.
+ const invalid = {
+ size: 16,
+ usage: GPUBufferUsage.MAP_READ | GPUBufferUsage.MAP_WRITE,
+ };
+ // A fresh adapter per device: an adapter only vends a single device.
+ return gpu
+ .requestAdapter()
+ .then((adapter) => adapter!.requestDevice())
+ .then((control) => {
+ control.pushErrorScope("validation");
+ control.createBuffer(invalid);
+ return control.popErrorScope();
+ })
+ .then((controlError) =>
+ gpu
+ .requestAdapter()
+ .then((adapter) =>
+ adapter!.requestDevice({
+ dawnToggles: { enabledToggles: ["skip_validation"] },
+ }),
+ )
+ .then((toggled) => {
+ toggled.pushErrorScope("validation");
+ toggled.createBuffer(invalid);
+ return toggled.popErrorScope();
+ })
+ .then((toggledError) => ({
+ controlHadError: controlError !== null,
+ toggledHadError: toggledError !== null,
+ })),
+ );
+ });
+ // The operation is genuinely invalid on this build (guards against the test
+ // silently passing because the buffer became valid).
+ expect(result.controlHadError).toBe(true);
+ // skip_validation took effect, which is only possible if dawnToggles was
+ // parsed and chained onto the device descriptor.
+ expect(result.toggledHadError).toBe(false);
+ });
});
Contributor
Author
|
Done, thank you! |
wcandillon
approved these changes
Jun 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a follow-up to #374. I seem to have forgotten to push this commit, which is required to actually parse the property. Sorry about that!