Bug 196758 - [Web GPU] Indexed drawing and GPUCommandEncoder crash prevention
Summary: [Web GPU] Indexed drawing and GPUCommandEncoder crash prevention
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Fan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-04-09 19:00 PDT by Justin Fan
Modified: 2019-04-10 13:49 PDT (History)
8 users (show)

See Also:


Attachments
Patch (27.82 KB, patch)
2019-04-09 19:15 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch for landing (28.23 KB, patch)
2019-04-10 13:09 PDT, Justin Fan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Fan 2019-04-09 19:00:32 PDT
[Web GPU] Indexed drawing and GPUCommandEncoder crash prevention
Comment 1 Justin Fan 2019-04-09 19:15:24 PDT
Created attachment 367096 [details]
Patch
Comment 2 Dean Jackson 2019-04-10 11:49:33 PDT
Comment on attachment 367096 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=367096&action=review

> Source/WebCore/ChangeLog:7
> +

Please give a summary of what you did.

> Source/WebCore/Modules/webgpu/WebGPUCommandEncoder.cpp:171
> +    if (!m_commandBuffer || m_commandBuffer->isEncodingPass()) {
>          LOG(WebGPU, "WebGPUCommandEncoder::finish(): Invalid operation!");

I wonder if we should give a better error message for this case.

e.g. "Unable to finish encoder because it's still encoding" or something.

Also, can you write a test for this case?

> Source/WebCore/Modules/webgpu/WebGPURenderPassEncoder.cpp:130
> +    for (auto& buffer : buffers) {

Why can't this be const?

> Source/WebCore/platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm:98
> +    if (isEncodingPass() || !src->isTransferSource() || !dst->isTransferDestination()) {

Again, it would be nice to have a more clear message here, as well as tests.

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPassEncoderMetal.mm:254
> +    if (offset >= buffer.byteLength() || offset % 4) {
> +        LOG(WebGPU, "GPURenderPassEncoder::setIndexBuffer(): Invalid offset!");
> +        return;
> +    }

Another case where we should have separate messages and tests.

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:528
> +

Nit: blank line

> LayoutTests/webgpu/draw-indexed-triangles.html:51
> +        // float4 xyzw, float g
> +        -1, 1, 0, 1, 1,
> +        -1, -1, 0, 1, 1,
> +        1, 1, 0, 1, 1,
> +        1, -1, 0, 1, 1
> +    ]);

You could put a few non-green values in there to enhance the test (or do it in a separate test). e.g.

1, 1, 0, 1, 0

> LayoutTests/webgpu/draw-indexed-triangles.html:59
> +const indexBufferOffset = 2048;
> +const indexOffset = -9001;

Can you write some comments explaining what you're trying to test here?

> LayoutTests/webgpu/draw-indexed-triangles.html:65
> +    const indexArray = new Uint32Array([0, 1, 2, 1, 2, 3]);
> +    indexArray.forEach((value, index) => {
> +        indexArray[index] = value - indexOffset;
> +    });

I guess this works, but seems a bit scary - changing values from inside an iterator. It might be better as something like:

const indexArray = new Uint32Array([0, 1, 2, 1, 2, 3].map(v => { v - indexOffset; }));
Comment 3 Justin Fan 2019-04-10 13:09:42 PDT
Created attachment 367150 [details]
Patch for landing
Comment 4 WebKit Commit Bot 2019-04-10 13:48:44 PDT
Comment on attachment 367150 [details]
Patch for landing

Clearing flags on attachment: 367150

Committed r244147: <https://trac.webkit.org/changeset/244147>
Comment 5 WebKit Commit Bot 2019-04-10 13:48:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Radar WebKit Bug Importer 2019-04-10 13:49:32 PDT
<rdar://problem/49788715>