Bug 238239 – REGRESSION (r287807): WEBGL_multi_draw validation rejecting valid arguments

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

A change was made to the validation code for WEBGL_multi_draw in Bug 234966 which was incorrect. Unfortunately, it causes about half of the tests in conformance/extensions/webgl-multi-draw.html to fail, and breaks customer applications, as reported on https://groups.google.com/g/webgl-dev-list .

[Kenneth Russell]

Created attachment 455455 [details]
Patch

[Kenneth Russell]

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.

[Darin Adler]

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?

[Darin Adler]

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?

[Darin Adler]

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.

[Darin Adler]

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

[Darin Adler]

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

[Darin Adler]

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

[Darin Adler]

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

[Radar WebKit Bug Importer]

<rdar://problem/90694727>

[Radar WebKit Bug Importer]

<rdar://problem/90694765>

[Darin Adler]

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?

[Darin Adler]

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.

[Kimmo Kinnunen]

Created attachment 455495 [details]
alternative impl

[Kimmo Kinnunen]

Created attachment 455496 [details]
alternative impl

[Kimmo Kinnunen]

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

[Darin Adler]

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

[Kenneth Russell]

Created attachment 455532 [details]
Patch

[Kenneth Russell]

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.

[Kenneth Russell]

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

[Kenneth Russell]

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.

[EWS]

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].

[Brent Fulgham]

This fix shipped with Safari 15.5 (all platforms).