WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
196758
[Web GPU] Indexed drawing and GPUCommandEncoder crash prevention
https://bugs.webkit.org/show_bug.cgi?id=196758
Summary
[Web GPU] Indexed drawing and GPUCommandEncoder crash prevention
Justin Fan
Reported
2019-04-09 19:00:32 PDT
[Web GPU] Indexed drawing and GPUCommandEncoder crash prevention
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Justin Fan
Comment 1
2019-04-09 19:15:24 PDT
Created
attachment 367096
[details]
Patch
Dean Jackson
Comment 2
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; }));
Justin Fan
Comment 3
2019-04-10 13:09:42 PDT
Created
attachment 367150
[details]
Patch for landing
WebKit Commit Bot
Comment 4
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
>
WebKit Commit Bot
Comment 5
2019-04-10 13:48:45 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 6
2019-04-10 13:49:32 PDT
<
rdar://problem/49788715
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug