Currently readPixels will crash if width/height are negative. Instead, INVALID_VALUE should be generated.
Also, with very large width/height and WebGL*Array creation failure, a INVALID_VALUE error will generate.
Created attachment 57063 [details] patch
What bug is this new line fixing? 1707 memset(data, 0, totalBytes); And does the layout test cover that? (If not, can it be added?)
(In reply to comment #3) > What bug is this new line fixing? > > 1707 memset(data, 0, totalBytes); > > And does the layout test cover that? (If not, can it be added?) This is not a bug fix. For safety reason, we clear the buffer so users will not get access to content they should not. According to GLES spec, for the un-covered portion of the memory, their content is un-defined. I don't think we should write a definitive test against something undefined, because whatever the content is, it should not be "incorrect".
(In reply to comment #4) > (In reply to comment #3) > > What bug is this new line fixing? > > > > 1707 memset(data, 0, totalBytes); > > > > And does the layout test cover that? (If not, can it be added?) > > This is not a bug fix. For safety reason, we clear the buffer so users will not get access to content they should not. According to GLES spec, for the un-covered portion of the memory, their content is un-defined. I don't think we should write a definitive test against something undefined, because whatever the content is, it should not be "incorrect". Should it be set to something (like -10 for example) that is less likely to make users rely on undefined behavior? 0 tends to make things just work (that maybe shouldn't) and thus hide problems.
(In reply to comment #4) > (In reply to comment #3) > > What bug is this new line fixing? > > > > 1707 memset(data, 0, totalBytes); > > > > And does the layout test cover that? (If not, can it be added?) > > This is not a bug fix. For safety reason, we clear the buffer so users will not get access to content they should not. According to GLES spec, for the un-covered portion of the memory, their content is un-defined. I don't think we should write a definitive test against something undefined, because whatever the content is, it should not be "incorrect". There should be a test, otherwise there's nothing to verify that the data is safe -- if some other implementation decides to produces something other than a zero-filled buffer they can add platform specific expected results.
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > What bug is this new line fixing? > > > > > > 1707 memset(data, 0, totalBytes); > > > > > > And does the layout test cover that? (If not, can it be added?) > > > > This is not a bug fix. For safety reason, we clear the buffer so users will not get access to content they should not. According to GLES spec, for the un-covered portion of the memory, their content is un-defined. I don't think we should write a definitive test against something undefined, because whatever the content is, it should not be "incorrect". > > Should it be set to something (like -10 for example) that is less likely to make users rely on undefined behavior? > > 0 tends to make things just work (that maybe shouldn't) and thus hide problems. Good point. I'll double check WebGL spec, if it doesn't have extra constraints about buffers initialized to 0, I'll use an odd number instead.
(In reply to comment #6) > (In reply to comment #4) > > (In reply to comment #3) > > > What bug is this new line fixing? > > > > > > 1707 memset(data, 0, totalBytes); > > > > > > And does the layout test cover that? (If not, can it be added?) > > > > This is not a bug fix. For safety reason, we clear the buffer so users will not get access to content they should not. According to GLES spec, for the un-covered portion of the memory, their content is un-defined. I don't think we should write a definitive test against something undefined, because whatever the content is, it should not be "incorrect". > > > There should be a test, otherwise there's nothing to verify that the data is safe -- if some other implementation decides to produces something other than a zero-filled buffer they can add platform specific expected results. Ok, will add a case then.
ReadPixels signature will change. This patch becomes invalid.
Due to https://bugs.webkit.org/show_bug.cgi?id=40322, negative width/height will no longer cause crash, but still, we need to deal with it correctly, i.e., generate INVALID_VALUE and return.
Created attachment 58418 [details] patch
Comment on attachment 58418 [details] patch WebCore/html/canvas/WebGLRenderingContext.cpp:1673 + } Let's put this check right after the NULL check for the "pixels" argument tomorrow to try to follow the request for prioritization of error returns from Mozilla: https://www.khronos.org/webgl/public-mailing-list/archives/1006/msg00040.html LayoutTests/fast/canvas/webgl/read-pixels.html:184 + return; Should presumably say: } else if (!array.length) { shouldBe("gl.getError()", "gl.NO_ERROR"); return; }
Created attachment 58426 [details] revised patch: responding to kbr's review
Comment on attachment 58426 [details] revised patch: responding to kbr's review Looks good.
Comment on attachment 58426 [details] revised patch: responding to kbr's review Make it so.
Comment on attachment 58426 [details] revised patch: responding to kbr's review Clearing flags on attachment: 58426 Committed r61019: <http://trac.webkit.org/changeset/61019>
All reviewed patches have been landed. Closing bug.