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