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

fixture.ts is checking for GPUPipelineError.stack to exist but it is not required on DOMException #3222

Open
mwyrzykowski opened this issue Dec 12, 2023 · 3 comments

Comments

@mwyrzykowski
Copy link

On this line:
https://github.com/gpuweb/cts/blob/7a6ef7301b5d84f751b483f9d5466b3696749c26/src/common/framework/fixture.ts#L254C11-L254C11

we see a check for:

if (!(ex instanceof Error && typeof ex.stack === 'string')) {

however neither GPUPipelineError as defined in the specification nor DOMException are required to have such a field.

https://webidl.spec.whatwg.org/#es-DOMException-specialness says that:

if an implementation gives native Error objects special powers or nonstandard properties (such as a stack property), it should also expose those on DOMException objects.

Which uses the word 'should' and not 'shall' or 'must', meaning it is recommended but not required by the specification.

In any case, the WebGPU tests expecting a stack property on DOMException seems outside the scope of these tests.

Would it be acceptable to remove this check for ex.stack in the CTS? It does not seem to be doing anything useful.

@mwyrzykowski
Copy link
Author

It looks like this was changed by #3105 cc @kainino0x

I would propose the WebGPU CTS does not test this as it does not appear required by any standard.

webkit-commit-queue pushed a commit to mwyrzykowski/WebKit that referenced this issue Jan 2, 2024
https://bugs.webkit.org/show_bug.cgi?id=266668
<radar://119898641>

Reviewed by Tadeu Zagallo.

Add validation for the RenderPipeline. CTS test validation/render_pipeline/depth_stencil_state is
now passing after this change.

* LayoutTests/http/tests/webgpu/webgpu/api/validation/render_pipeline/depth_stencil_state-expected.txt:
Add passing expectations.

* LayoutTests/http/tests/webgpu/webgpu/api/validation/render_pipeline/depth_stencil_state.spec.js:
Update to latest version of this test.

* Source/WebCore/Modules/WebGPU/GPUDepthStencilState.h:
(WebCore::GPUDepthStencilState::convertToBacking const):

* Source/WebCore/Modules/WebGPU/GPUDepthStencilState.idl:
depthCompare and depthWriteEnabled are now optional.

* Source/WebCore/Modules/WebGPU/GPUInternalError.h:
(WebCore::GPUInternalError::stack const):
* Source/WebCore/Modules/WebGPU/GPUInternalError.idl:
* Source/WebCore/Modules/WebGPU/GPUPipelineError.h:
* Source/WebCore/Modules/WebGPU/GPUPipelineError.idl:
* Source/WebCore/Modules/WebGPU/GPUValidationError.h:
(WebCore::GPUValidationError::stack const):
* Source/WebCore/Modules/WebGPU/GPUValidationError.idl:
Workaround lack of stack property until gpuweb/cts#3222
is addressed as it causes too many false failures in the live version
of the CTS.

* Source/WebCore/Modules/WebGPU/Implementation/WebGPUDeviceImpl.cpp:
(WebCore::WebGPU::convertToBacking):
* Source/WebCore/Modules/WebGPU/InternalAPI/WebGPUDepthStencilState.h:
* Source/WebGPU/WGSL/Parser.cpp:
(WGSL::Parser<Lexer>::parseAttribute):
* Source/WebGPU/WGSL/WGSLShaderModule.h:
(WGSL::ShaderModule::usesFragDepth const):
(WGSL::ShaderModule::setUsesFragDepth):
* Source/WebGPU/WebGPU/RenderPipeline.mm:
(WebGPU::returnInvalidRenderPipeline):
(WebGPU::name):
(WebGPU::errorValidatingDepthStencilState):
(WebGPU::Device::createRenderPipeline):
* Source/WebGPU/WebGPU/Texture.h:
* Source/WebGPU/WebGPU/Texture.mm:
(WebGPU::Texture::isRenderableFormat):
(WebGPU::Texture::supportsMultisampling):
(WebGPU::Texture::supportsBlending):
(WebGPU::Device::errorValidatingTextureCreation):
(WebGPU::isRenderableFormat): Deleted.
(WebGPU::supportsMultisampling): Deleted.
* Source/WebGPU/WebGPU/WebGPU.h:
* Source/WebKit/Shared/WebGPU/WebGPUDepthStencilState.h:
* Source/WebKit/Shared/WebGPU/WebGPUDepthStencilState.serialization.in:
Implement validation in render pipeline creation.

Canonical link: https://commits.webkit.org/272562@main
@kainino0x
Copy link
Collaborator

Apologies for the delay on this, fell off my plate before the holidays.

I didn't dig into the specs because I didn't expect something this foundational to be unspecified behavior. I actually would strongly like to keep these checks, because I think it's a real source of inter-browser incompatibility if stack exists on some browsers and not others. IMO sometimes the tests can be aspirational and go beyond what the specs strictly require (though this philosophy is more applicable to things like the behavior of anisotropic filtering, than error stacks).

However if you think these should not be required then I can at least relegate the check to a single test, or a warning, or something, instead.

@mwyrzykowski
Copy link
Author

No worries, it is not blocking as it is simple to work around. However to quote the readme of the CTS:

The contents of this test suite are considered normative; implementations must pass them to be WebGPU-conformant. Mismatches between the specification and tests are bugs.

unless the WebGPU specification specifically requires the stack property on GPUPipelineError, GPUInternalError, and GPUValidationError, which it currently does not, then testing for stack in the CTS seems the wrong place as the specification for DOMException does not require a stack property either. It seems contradictory to have aspirational tests in the CTS yet also state an implementation must pass the suite to be WebGPU-conformant.

A warning sounds great, but no rush as I just worked around it locally to ensure we didn't have other bugs preventing those suites from passing.

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

No branches or pull requests

2 participants