Bug 238239

Created attachment 455455 [details]
Patch

Kimmo / Brent / Darin: please review. Thanks.

Confirmed by running:
https://www.khronos.org/registry/webgl/sdk/tests/conformance/extensions/webgl-multi-draw.html

locally that this fixes the test.

Comment on attachment 455455 [details]
Patch

This seems too complicated to me. Couldn’t this also be solved by removing the incorrect type cast of size - drawCount to GCGLuint?

Comment on attachment 455455 [details]
Patch

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

> Source/WebCore/html/canvas/WebGLMultiDraw.cpp:-137
> -    if (offset >= static_cast<GCGLuint>(size - drawcount)) {

The bug seems to be that this can chop a large integer to a smaller one. Can we just change this to not do that rather than adding more code?

Comment on attachment 455455 [details]
Patch

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

>> Source/WebCore/html/canvas/WebGLMultiDraw.cpp:-137
>> -    if (offset >= static_cast<GCGLuint>(size - drawcount)) {
> 
> The bug seems to be that this can chop a large integer to a smaller one. Can we just change this to not do that rather than adding more code?

Or is the real problem that this is used >=  rather than >?

I’d like to understand what the actual problem was before weighing in on the solution.

Your post about this says there are “gaps in WebGL testing in WebKit” but this patch does not include any new WebGL tests.

Webkit policy requires adding a new test case when fixing a bug so we do need to add a test case here.

We can turn https://www.khronos.org/registry/webgl/sdk/tests/conformance/extensions/webgl-multi-draw.html into a test case perhaps?

Is there something that prevents us from using these Kronos test cases as part of the Webkit test suite?

<rdar://problem/90694727>

<rdar://problem/90694765>

I see now, by to 20753 is the reason we had this untested. Is there something we can do to resolve that so that we can at least run some of that test?

Besides the disabled test leaving us vulnerable, my suggestion for a cleaner way to write the range check in the first place is probably what led to the mistake. I understand if others would like to take Ken‘s patch as is. I would, however, like to understand what was wrong with the original and I think that could lead to a more elegant fix. However I will not insist on this; urgent, I think, to have this fixed one way or another.

Created attachment 455495 [details]
alternative impl

Created attachment 455496 [details]
alternative impl

*** Bug 237418 has been marked as a duplicate of this bug. ***

Comment on attachment 455496 [details]
alternative impl

Yes, I like this better. i’m gonna say r=me on this one, assuming we make sure tests really are all passing

Created attachment 455532 [details]
Patch

Darin and Kimmo, thanks for your review and for pointing out the alternative implementation (and my unclear description of what was wrong). Anyway, here is Kimmo's patch with his authorship attributed in the ChangeLog, and the tests marked Slow in TestExpectations as they take a while to run. If the EWS is green I'll CQ this later today.

The http/wpt/webrtc/getUserMedia-processSwapping.html failure on mac-AS-debug-wk2 is unrelated; filed Bug 238303 about it. Notably, webgl-multi-draw.html passed in that run in both the WebGL 1.0.x and 2.0.y conformance tests:
https://ews-build.webkit.org/#/builders/60/builds/27223

Kimmo, could you take over this patch from me and try to land it during your workday? In particular, to see if the other EWS bots pass.

Committed r291791 (248819@main): <https://commits.webkit.org/248819@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 455532 [details].

This fix shipped with Safari 15.5 (all platforms).