Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make some errors not fatal #3094

Merged
merged 3 commits into from
Oct 11, 2022
Merged

Make some errors not fatal #3094

merged 3 commits into from
Oct 11, 2022

Conversation

nical
Copy link
Contributor

@nical nical commented Oct 11, 2022

Checklist

  • Run cargo clippy.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Connections

https://bugzilla.mozilla.org/show_bug.cgi?id=1791881

Description

Objects can be created with invalid parameters, putting them in an error state. When interacting with these objects, in the majority of cases validation should fail but wgpu should not panic.
buffer_get_mapped_range is one of these exceptions because the API user must not call it without getting consent from the status provided in the map_async callback.

Currently wgpu does panic when interacting with invalid resources in a few places and this PR starts fixing that. I did not go over all of them to make sure there is consensus before producing a large-ish patch, and to get the few changes needed to unblock fuzzing as soon as possible.

There two important changes in this patch:

  • in hub.rs, Some(vacant) and None are the same (it indicates the resource was not created or was already dropped) so they should both be fatal errors.
  • with the non-existent-or-freed error case out of the way, most places that call handle_error_fatal in backend/direct.rs should actually push the error into the error sink intsead of panicking.

Only the first change really affects Firefox, but I think that the second one better matches the spec and how Firefox uses wgpu-core.

Testing

Tests! I did it again.

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests make me very happy

wgpu/tests/resource_error.rs Outdated Show resolved Hide resolved
wgpu/tests/resource_error.rs Outdated Show resolved Hide resolved
wgpu/tests/resource_error.rs Outdated Show resolved Hide resolved
@nical
Copy link
Contributor Author

nical commented Oct 11, 2022

I updated the patch. fail can return values and is hoisted in common, there's a valid equivalent that asserts validation succeeded (useful when you want to assert that specific/individual calls pass validation).

I made a notable change to the error handling after checking the spec: buffer/texture.destroy does not actually raise any error even when called multiple times.

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Format mad, but otherwise LGTM

@cwfitzgerald cwfitzgerald merged commit eca04f5 into gfx-rs:master Oct 11, 2022
@nical nical deleted the bad-buffer branch October 11, 2022 20:40
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 13, 2022
…valid state. r=jgilbert

Once gfx-rs/wgpu#3094 is merged, unallocated and freed handles will panic in wgpu-core so we don't have to do it here. In the mean time it will produce the wrong error but still fail safely. DestroyError::Invalid means the handle exists but is not is in an invalid state, for example if the buffer was created with invalid parameter like in this bug's test case.

Differential Revision: https://phabricator.services.mozilla.com/D159054
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Oct 19, 2022
…valid state. r=jgilbert

Once gfx-rs/wgpu#3094 is merged, unallocated and freed handles will panic in wgpu-core so we don't have to do it here. In the mean time it will produce the wrong error but still fail safely. DestroyError::Invalid means the handle exists but is not is in an invalid state, for example if the buffer was created with invalid parameter like in this bug's test case.

Differential Revision: https://phabricator.services.mozilla.com/D159054
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants