Summary: | Index validation code validates too many vertex attributes | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kenneth Russell <kbr> | ||||||||||||||
Component: | WebGL | Assignee: | Zhenyao Mo <zmo> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | brettw, cmarrin, commit-queue, dglazkov, fishd, kbr, oliver, petersont, rlp, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Attachments: |
|
Description
Kenneth Russell
2009-11-25 16:24:25 PST
Created attachment 52797 [details]
patch
Attachment 52797 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/html/canvas/WebGLProgram.cpp:53: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Total errors found: 1 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 52798 [details]
revised patch: fix a typo
Attachment 52798 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/html/canvas/WebGLProgram.cpp:53: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Total errors found: 1 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 52800 [details]
revised patch: fix a style issue
Comment on attachment 52800 [details] revised patch: fix a style issue > Index: WebCore/html/canvas/WebGLProgram.cpp > =================================================================== > --- WebCore/html/canvas/WebGLProgram.cpp (revision 57235) > +++ WebCore/html/canvas/WebGLProgram.cpp (working copy) > @@ -48,6 +48,34 @@ void WebGLProgram::_deleteObject(Platfor > context()->graphicsContext3D()->deleteProgram(object); > } > > +bool WebGLProgram::cacheActiveAttribLocations() > +{ > + if (!object()) > + return false; > + GraphicsContext3D* context3d = context()->graphicsContext3D(); > + int linkStatus; > + context3d->getProgramiv(this, GraphicsContext3D::LINK_STATUS, &linkStatus); > + if (!linkStatus) Should call m_activeAttribLocations.clear() in this case. > Index: WebCore/html/canvas/WebGLProgram.h > =================================================================== > --- WebCore/html/canvas/WebGLProgram.h (revision 57235) > +++ WebCore/html/canvas/WebGLProgram.h (working copy) > @@ -30,6 +30,7 @@ > > #include <wtf/PassRefPtr.h> > #include <wtf/RefCounted.h> > +#include <wtf/Vector.h> > > namespace WebCore { > > @@ -38,11 +39,22 @@ namespace WebCore { > virtual ~WebGLProgram() { deleteObject(); } > > static PassRefPtr<WebGLProgram> create(WebGLRenderingContext*); > + > + // cacheActiveAttribLocation() is only called once after linkProgram() > + // succeeds. > + bool cacheActiveAttribLocations(); > + // doesAttribLocationBelongToProgram only returns true when > + // cacheActiveAttribLocations() already succeeded and attrib location > + // actually belongs to the program. > + bool doesAttribLocationBelongToProgram(unsigned location); Instead of doesAttribLocationBelongToProgram, please add two APIs, int numActiveAttribLocations() and int getActiveAttribLocation(int). > Index: WebCore/html/canvas/WebGLRenderingContext.cpp > =================================================================== > --- WebCore/html/canvas/WebGLRenderingContext.cpp (revision 57235) > +++ WebCore/html/canvas/WebGLRenderingContext.cpp (working copy) > @@ -688,11 +688,14 @@ bool WebGLRenderingContext::validateInde > > bool WebGLRenderingContext::validateRenderingState(long numElementsRequired) > { > + if (!m_currentProgram) > + return false; > + > // Look in each enabled vertex attrib and find the smallest buffer size > long smallestNumElements = LONG_MAX; > for (unsigned i = 0; i < m_vertexAttribState.size(); ++i) { > const VertexAttribState& state = m_vertexAttribState[i]; > - if (state.enabled && state.numElements < smallestNumElements) > + if (state.enabled && m_currentProgram->doesAttribLocationBelongToProgram(i) && state.numElements < smallestNumElements) > smallestNumElements = state.numElements; > } Please rewrite this loop to iterate from 0..m_currentProgram->numActiveAttribLocations(). Even though the number of active attributes is likely to be small, this loop is O(n*m) where it should be O(m). > Index: LayoutTests/ChangeLog > =================================================================== > --- LayoutTests/ChangeLog (revision 57240) > +++ LayoutTests/ChangeLog (working copy) > @@ -1,3 +1,13 @@ > +2010-04-07 Zhenyao Mo <zmo@google.com> > + > + Reviewed by NOBODY (OOPS!). > + > + Index validation code validates too many vertex attributes > + https://bugs.webkit.org/show_bug.cgi?id=31892 > + > + * fast/canvas/webgl/index-validation-expected.txt: Add new test cases for index validations. > + * fast/canvas/webgl/script-tests/index-validation.js: Ditto. > + > 2010-04-07 Erik Arvidsson <arv@chromium.org> > > Reviewed by Adam Barth. > Index: LayoutTests/fast/canvas/webgl/index-validation-expected.txt > =================================================================== > --- LayoutTests/fast/canvas/webgl/index-validation-expected.txt (revision 57235) > +++ LayoutTests/fast/canvas/webgl/index-validation-expected.txt (working copy) > @@ -1,11 +1,30 @@ > -Test of get calls against GL objects like getBufferParameter, etc. > +Test of validating indices for drawElements(). > > On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". > > +Testing with valid indices > PASS gl.checkFramebufferStatus(gl.FRAMEBUFFER) is gl.FRAMEBUFFER_COMPLETE > PASS gl.getError() is 0 > PASS gl.drawElements(gl.TRIANGLES, 3, gl.UNSIGNED_SHORT, 0) is undefined. > PASS gl.getError() is 0 > +Testing with out-of-range indices > +Enable vertices, valid > +PASS gl.getError() is 0 > +PASS gl.drawElements(gl.TRIANGLES, 3, gl.UNSIGNED_SHORT, 0) is undefined. > +PASS gl.getError() is 0 > +Enable normals, out-of-range > +PASS gl.getError() is 0 > +PASS gl.drawElements(gl.TRIANGLES, 3, gl.UNSIGNED_SHORT, 0) is undefined. > +PASS gl.getError() is gl.INVALID_OPERATION > +Test with enabled attribute that does not belong to current program > +Enable an extra attribute with null > +PASS gl.getError() is 0 > +PASS gl.drawElements(gl.TRIANGLES, 3, gl.UNSIGNED_SHORT, 0) is undefined. > +PASS gl.getError() is 0 > +Enable an extra attribute with inefficient data buffer > +PASS gl.getError() is 0 > +PASS gl.drawElements(gl.TRIANGLES, 3, gl.UNSIGNED_SHORT, 0) is undefined. > +PASS gl.getError() is 0 > PASS successfullyParsed is true > > TEST COMPLETE > Index: LayoutTests/fast/canvas/webgl/script-tests/index-validation.js > =================================================================== > --- LayoutTests/fast/canvas/webgl/script-tests/index-validation.js (revision 57235) > +++ LayoutTests/fast/canvas/webgl/script-tests/index-validation.js (working copy) > @@ -1,20 +1,27 @@ > -description("Test of get calls against GL objects like getBufferParameter, etc."); > +description("Test of validating indices for drawElements()."); > > var gl = create3DContext(); > var program = loadStandardProgram(gl); > > // 3 vertices => 1 triangle, interleaved data > -var data = new WebGLFloatArray([0, 0, 0, 1, > - 0, 0, 1, > - 1, 0, 0, 1, > - 0, 0, 1, > - 1, 1, 1, 1, > - 0, 0, 1]); > +var dataComplete = new WebGLFloatArray([0, 0, 0, 1, > + 0, 0, 1, > + 1, 0, 0, 1, > + 0, 0, 1, > + 1, 1, 1, 1, > + 0, 0, 1]); > +var dataIncomplete = new WebGLFloatArray([0, 0, 0, 1, > + 0, 0, 1, > + 1, 0, 0, 1, > + 0, 0, 1, > + 1, 1, 1, 1]); > var indices = new WebGLUnsignedShortArray([0, 1, 2]); > > -var buffer = gl.createBuffer(); > -gl.bindBuffer(gl.ARRAY_BUFFER, buffer); > -gl.bufferData(gl.ARRAY_BUFFER, data, gl.STATIC_DRAW); > +debug("Testing with valid indices"); > + > +var bufferComplete = gl.createBuffer(); > +gl.bindBuffer(gl.ARRAY_BUFFER, bufferComplete); > +gl.bufferData(gl.ARRAY_BUFFER, dataComplete, gl.STATIC_DRAW); > var elements = gl.createBuffer(); > gl.bindBuffer(gl.ELEMENT_ARRAY_BUFFER, elements); > gl.bufferData(gl.ELEMENT_ARRAY_BUFFER, indices, gl.STATIC_DRAW); > @@ -23,11 +30,45 @@ var vertexLoc = gl.getAttribLocation(pro > var normalLoc = gl.getAttribLocation(program, "a_normal"); > gl.vertexAttribPointer(vertexLoc, 4, gl.FLOAT, false, 7 * gl.sizeInBytes(gl.FLOAT), 0); > gl.enableVertexAttribArray(vertexLoc); > -gl.vertexAttribPointer(normalLoc, 3, gl.FLOAT, false, 7 * gl.sizeInBytes(gl.FLOAT), 3 * gl.sizeInBytes(gl.FLOAT)); > +gl.vertexAttribPointer(normalLoc, 3, gl.FLOAT, false, 7 * gl.sizeInBytes(gl.FLOAT), 4 * gl.sizeInBytes(gl.FLOAT)); > gl.enableVertexAttribArray(normalLoc); > shouldBe('gl.checkFramebufferStatus(gl.FRAMEBUFFER)', 'gl.FRAMEBUFFER_COMPLETE'); > shouldBe('gl.getError()', '0'); > shouldBeUndefined('gl.drawElements(gl.TRIANGLES, 3, gl.UNSIGNED_SHORT, 0)'); > shouldBe('gl.getError()', '0'); > > +debug("Testing with out-of-range indices"); > + > +var bufferIncomplete = gl.createBuffer(); > +gl.bindBuffer(gl.ARRAY_BUFFER, bufferIncomplete); > +gl.bufferData(gl.ARRAY_BUFFER, dataIncomplete, gl.STATIC_DRAW); > +gl.vertexAttribPointer(vertexLoc, 4, gl.FLOAT, false, 7 * gl.sizeInBytes(gl.FLOAT), 0); > +gl.enableVertexAttribArray(vertexLoc); > +gl.disableVertexAttribArray(normalLoc); > +debug("Enable vertices, valid"); > +shouldBe('gl.getError()', '0'); > +shouldBeUndefined('gl.drawElements(gl.TRIANGLES, 3, gl.UNSIGNED_SHORT, 0)'); > +shouldBe('gl.getError()', '0'); > +debug("Enable normals, out-of-range"); > +gl.vertexAttribPointer(normalLoc, 3, gl.FLOAT, false, 7 * gl.sizeInBytes(gl.FLOAT), 4 * gl.sizeInBytes(gl.FLOAT)); > +gl.enableVertexAttribArray(normalLoc); > +shouldBe('gl.getError()', '0'); > +shouldBeUndefined('gl.drawElements(gl.TRIANGLES, 3, gl.UNSIGNED_SHORT, 0)'); > +shouldBe('gl.getError()', 'gl.INVALID_OPERATION'); > + > +debug("Test with enabled attribute that does not belong to current program"); > + > +gl.disableVertexAttribArray(normalLoc); > +var extraLoc = Math.max(vertexLoc, normalLoc) + 1; > +gl.enableVertexAttribArray(extraLoc); > +debug("Enable an extra attribute with null"); > +shouldBe('gl.getError()', '0'); > +shouldBeUndefined('gl.drawElements(gl.TRIANGLES, 3, gl.UNSIGNED_SHORT, 0)'); > +shouldBe('gl.getError()', '0'); > +debug("Enable an extra attribute with inefficient data buffer"); inefficient -> insufficient. New tests look good otherwise. Thanks for fixing the bug in the preexisting test. Created attachment 52830 [details]
revised patch : responding to Ken Russell's review
Created attachment 52831 [details]
revised patch: fix the change log
Comment on attachment 52831 [details] revised patch: fix the change log Looks good. One comment. > Index: WebCore/html/canvas/WebGLProgram.cpp > =================================================================== > --- WebCore/html/canvas/WebGLProgram.cpp (revision 57240) > +++ WebCore/html/canvas/WebGLProgram.cpp (working copy) > @@ -48,6 +48,41 @@ void WebGLProgram::_deleteObject(Platfor > context()->graphicsContext3D()->deleteProgram(object); > } > > +bool WebGLProgram::cacheActiveAttribLocations() > +{ > + m_activeAttribLocations.clear(); > + if (!object()) > + return false; > + GraphicsContext3D* context3d = context()->graphicsContext3D(); > + int linkStatus; > + context3d->getProgramiv(this, GraphicsContext3D::LINK_STATUS, &linkStatus); > + if (!linkStatus) > + return false; > + > + int numAttribs = 0; > + context3d->getProgramiv(this, GraphicsContext3D::ACTIVE_ATTRIBUTES, &numAttribs); > + m_activeAttribLocations.resize(static_cast<size_t>(numAttribs)); > + for (int i = 0; i < numAttribs; ++i) { > + ActiveInfo info; > + context3d->getActiveAttrib(this, i, info); > + m_activeAttribLocations[i] = context3d->getAttribLocation(this, info.name.charactersWithNullTermination()); > + } > + > + return true; > +} > + > +int WebGLProgram::numActiveAttribLocations() > +{ > + return static_cast<int>(m_activeAttribLocations.size()); > +} > + > +int WebGLProgram::getActiveAttribLocation(int index) > +{ > + if (index < 0 || index >= static_cast<int>(m_activeAttribLocations.size())) This would be better written as if (index < 0 || index >= numActiveAttribLocations()) Created attachment 52895 [details]
revised patch: minor modification as Ken Russell suggested
Comment on attachment 52895 [details]
revised patch: minor modification as Ken Russell suggested
Looks good to me.
Comment on attachment 52895 [details]
revised patch: minor modification as Ken Russell suggested
rs=me given kbr approves.
Comment on attachment 52895 [details] revised patch: minor modification as Ken Russell suggested Clearing flags on attachment: 52895 Committed r57709: <http://trac.webkit.org/changeset/57709> All reviewed patches have been landed. Closing bug. |