RESOLVED FIXED 42124
WebGLBuffer::associateBufferData(ArrayBufferView* array) copies incorrect data for sliced views
https://bugs.webkit.org/show_bug.cgi?id=42124
Summary WebGLBuffer::associateBufferData(ArrayBufferView* array) copies incorrect dat...
Kenneth Russell
Reported 2010-07-12 18:13:57 PDT
From code inspection, WebGLBuffer::associateBufferData(ArrayBufferView* array) will copy the wrong data from the passed ArrayBufferView in the case where the WebGLBuffer is bound to the ELEMENT_ARRAY_BUFFER binding point, and the passed ArrayBufferView is a slice of a larger ArrayBuffer with a non-zero offset. The reason is that the m_elementArrayBuffer is a copy of the entire backing store of the ArrayBufferView (ArrayBuffer::create(array->buffer().get())), but actually needs to be a copy of only the portion referenced by the view. Ideally, both associateBufferData(ArrayBufferView*) and the new WebGLBuffer::associateBufferData(ArrayBuffer* array) being introduced in bug 41884 would be implemented in terms of an associateBufferDataImpl(ArrayBuffer*, int offset, int length). associateBufferSubData(long, ArrayBufferView*) and associateBufferSubData(long, ArrayBuffer*) should also share their implementations.
Attachments
patch (12.60 KB, patch)
2010-08-04 15:41 PDT, Zhenyao Mo
zmo: commit-queue-
revised patch: responding to kbr's review (13.41 KB, patch)
2010-08-04 19:17 PDT, Zhenyao Mo
zmo: commit-queue-
revised patch: tiny modification (13.47 KB, patch)
2010-08-04 19:20 PDT, Zhenyao Mo
zmo: commit-queue-
revised patch: fix a style issue (13.49 KB, patch)
2010-08-04 19:22 PDT, Zhenyao Mo
dglazkov: review+
zmo: commit-queue-
Zhenyao Mo
Comment 1 2010-08-04 15:41:45 PDT
Created attachment 63505 [details] patch The test cases have been committed to khronos already and are copied over using copy-from-khronos script.
Kenneth Russell
Comment 2 2010-08-04 18:02:52 PDT
Comment on attachment 63505 [details] patch Generally looks very good. A few comments / questions. > Index: WebCore/ChangeLog > =================================================================== > --- WebCore/ChangeLog (revision 64684) > +++ WebCore/ChangeLog (working copy) > @@ -1,3 +1,17 @@ > +2010-08-04 Zhenyao Mo <zmo@google.com> > + > + Reviewed by NOBODY (OOPS!). > + > + WebGLBuffer::associateBufferData(ArrayBufferView* array) copies incorrect data for sliced views > + https://bugs.webkit.org/show_bug.cgi?id=42124 > + > + * html/canvas/WebGLBuffer.cpp: > + (WebCore::WebGLBuffer::associateBufferDataImpl): Helper function that's called by all associateBufferData(). > + (WebCore::WebGLBuffer::associateBufferData): Call associateBufferDataImpl(). > + (WebCore::WebGLBuffer::associateBufferSubDataImpl): Helper function that's called by all associateBufferSubData(). > + (WebCore::WebGLBuffer::associateBufferSubData): Call associateBufferSubDataImpl(). > + * html/canvas/WebGLBuffer.h: Declare helper functions. > + > 2010-08-04 Dan Bernstein <mitz@apple.com> > > Build fix. > Index: WebCore/html/canvas/WebGLBuffer.cpp > =================================================================== > --- WebCore/html/canvas/WebGLBuffer.cpp (revision 64660) > +++ WebCore/html/canvas/WebGLBuffer.cpp (working copy) > @@ -52,148 +52,96 @@ void WebGLBuffer::_deleteObject(Platform > context()->graphicsContext3D()->deleteBuffer(object); > } > > -bool WebGLBuffer::associateBufferData(int size) > +bool WebGLBuffer::associateBufferDataImpl(ArrayBuffer* array, unsigned byteOffset, unsigned byteLength) > { > - if (!m_target) > + if (array && byteLength && (byteOffset >= array->byteLength() || array->byteLength() - byteOffset < byteLength)) Are these checks sufficient to guard against integer overflow? Should we use the CheckedInt class? > return false; > > - if (m_target == GraphicsContext3D::ELEMENT_ARRAY_BUFFER) { > - m_byteLength = size; > + switch (m_target) { > + case GraphicsContext3D::ELEMENT_ARRAY_BUFFER: > + m_byteLength = byteLength; > clearCachedMaxIndices(); > - m_elementArrayBuffer = ArrayBuffer::create(size, 1); > - if (!m_elementArrayBuffer) { > + m_elementArrayBuffer = ArrayBuffer::create(byteLength, 1); > + if (byteLength && !m_elementArrayBuffer) { Would it be better to explicitly test whether byteLength == 0? > m_byteLength = 0; > return false; > } > + if (array) { > + // We must always clone the incoming data because client-side > + // modifications without calling bufferData or bufferSubData > + // must never be able to change the validation results. > + memcpy(static_cast<unsigned char*>(m_elementArrayBuffer->data()), > + static_cast<unsigned char*>(array->data()) + byteOffset, > + byteLength); > + } > return true; > - } else if (m_target == GraphicsContext3D::ARRAY_BUFFER) { > - m_byteLength = size; > + case GraphicsContext3D::ARRAY_BUFFER: > + m_byteLength = byteLength; > return true; > + default: > + return false; > } > +} > > - return false; > +bool WebGLBuffer::associateBufferData(int size) > +{ > + if (size < 0) > + return false; > + return associateBufferDataImpl(0, 0, static_cast<unsigned>(size)); > } > > bool WebGLBuffer::associateBufferData(ArrayBuffer* array) > { > - if (!m_target) > - return false; > if (!array) > return false; > - > - if (m_target == GraphicsContext3D::ELEMENT_ARRAY_BUFFER) { > - clearCachedMaxIndices(); > - m_byteLength = array->byteLength(); > - // We must always clone the incoming data because client-side > - // modifications without calling bufferData or bufferSubData > - // must never be able to change the validation results. > - m_elementArrayBuffer = ArrayBuffer::create(array); > - if (!m_elementArrayBuffer) { > - m_byteLength = 0; > - return false; > - } > - return true; > - } > - > - if (m_target == GraphicsContext3D::ARRAY_BUFFER) { > - m_byteLength = array->byteLength(); > - return true; > - } > - > - return false; > + return associateBufferDataImpl(array, 0, array->byteLength()); > } > > bool WebGLBuffer::associateBufferData(ArrayBufferView* array) > { > - if (!m_target) > - return false; > if (!array) > return false; > - > - if (m_target == GraphicsContext3D::ELEMENT_ARRAY_BUFFER) { > - clearCachedMaxIndices(); > - m_byteLength = array->byteLength(); > - // We must always clone the incoming data because client-side > - // modifications without calling bufferData or bufferSubData > - // must never be able to change the validation results. > - m_elementArrayBuffer = ArrayBuffer::create(array->buffer().get()); > - if (!m_elementArrayBuffer) { > - m_byteLength = 0; > - return false; > - } > - return true; > - } > - > - if (m_target == GraphicsContext3D::ARRAY_BUFFER) { > - m_byteLength = array->byteLength(); > - return true; > - } > - > - return false; > + return associateBufferDataImpl(array->buffer().get(), array->byteOffset(), array->byteLength()); > } > > -bool WebGLBuffer::associateBufferSubData(long offset, ArrayBuffer* array) > +bool WebGLBuffer::associateBufferSubDataImpl(long offset, ArrayBuffer* array, unsigned arrayByteOffset, unsigned arrayByteLength) > { > - if (!m_target) > + if (!array || offset < 0) > return false; > - if (!array) > + if (arrayByteLength && (arrayByteOffset >= array->byteLength() || array->byteLength() - arrayByteOffset < arrayByteLength)) Same question as above about whether we should use CheckedInt. > + return false; > + unsigned long uoffset = static_cast<unsigned long>(offset); > + if (arrayByteLength && (uoffset >= m_byteLength || arrayByteLength > m_byteLength - uoffset)) Same question about CheckedInt. > return false; > > - if (m_target == GraphicsContext3D::ELEMENT_ARRAY_BUFFER) { > + switch (m_target) { > + case GraphicsContext3D::ELEMENT_ARRAY_BUFFER: > clearCachedMaxIndices(); > - > - // We need to protect against integer overflow with these tests > - if (offset < 0) > - return false; > - > - unsigned long uoffset = static_cast<unsigned long>(offset); > - if (uoffset > m_byteLength || array->byteLength() > m_byteLength - uoffset) > - return false; > - > if (!m_elementArrayBuffer) > return false; > - > memcpy(static_cast<unsigned char*>(m_elementArrayBuffer->data()) + offset, > - static_cast<unsigned char*>(array->data()), > - array->byteLength()); > + static_cast<unsigned char*>(array->data()) + arrayByteOffset, > + arrayByteLength); > + return true; > + case GraphicsContext3D::ARRAY_BUFFER: > return true; > + default: > + return false; > } > +} > > - if (m_target == GraphicsContext3D::ARRAY_BUFFER) > - return array->byteLength() + offset <= m_byteLength; > - > - return false; > +bool WebGLBuffer::associateBufferSubData(long offset, ArrayBuffer* array) > +{ > + if (!array) > + return false; > + return associateBufferSubDataImpl(offset, array, 0, array->byteLength()); > } > > bool WebGLBuffer::associateBufferSubData(long offset, ArrayBufferView* array) > { > - if (!m_target) > - return false; > if (!array) > return false; > - > - if (m_target == GraphicsContext3D::ELEMENT_ARRAY_BUFFER) { > - clearCachedMaxIndices(); > - > - // We need to protect against integer overflow with these tests > - if (offset < 0) > - return false; > - > - unsigned long uoffset = static_cast<unsigned long>(offset); > - if (uoffset > m_byteLength || array->byteLength() > m_byteLength - uoffset) > - return false; > - > - if (!m_elementArrayBuffer) > - return false; > - > - memcpy(static_cast<unsigned char*>(m_elementArrayBuffer->data()) + offset, array->baseAddress(), array->byteLength()); > - return true; > - } > - > - if (m_target == GraphicsContext3D::ARRAY_BUFFER) > - return array->byteLength() + offset <= m_byteLength; > - > - return false; > + return associateBufferSubDataImpl(offset, array->buffer().get(), array->byteOffset(), array->byteLength()); > } > > unsigned WebGLBuffer::byteLength() const > Index: WebCore/html/canvas/WebGLBuffer.h > =================================================================== > --- WebCore/html/canvas/WebGLBuffer.h (revision 64660) > +++ WebCore/html/canvas/WebGLBuffer.h (working copy) > @@ -90,6 +90,11 @@ namespace WebCore { > > // Clears all of the cached max indices. > void clearCachedMaxIndices(); > + > + // Helper function called by the three associateBufferData(). > + bool associateBufferDataImpl(ArrayBuffer* array, unsigned byteOffset, unsigned byteLength); > + // Helper function called by the two associateBufferSubData(). > + bool associateBufferSubDataImpl(long offset, ArrayBuffer* array, unsigned arrayByteOffset, unsigned arrayByteLength); > }; > > } // namespace WebCore > Index: LayoutTests/ChangeLog > =================================================================== > --- LayoutTests/ChangeLog (revision 64684) > +++ LayoutTests/ChangeLog (working copy) > @@ -1,3 +1,13 @@ > +2010-08-04 Zhenyao Mo <zmo@google.com> > + > + Reviewed by NOBODY (OOPS!). > + > + WebGLBuffer::associateBufferData(ArrayBufferView* array) copies incorrect data for sliced views > + https://bugs.webkit.org/show_bug.cgi?id=42124 > + > + * fast/canvas/webgl/draw-elements-out-of-bounds-expected.txt: Adding test case for bufferData and bufferSubData with ArrayBufferView input. > + * fast/canvas/webgl/draw-elements-out-of-bounds.html: Ditto. > + > 2010-08-04 Dan Bernstein <mitz@apple.com> > > Reviewed by Simon Fraser. > Index: LayoutTests/fast/canvas/webgl/draw-elements-out-of-bounds-expected.txt > =================================================================== > --- LayoutTests/fast/canvas/webgl/draw-elements-out-of-bounds-expected.txt (revision 64660) > +++ LayoutTests/fast/canvas/webgl/draw-elements-out-of-bounds-expected.txt (working copy) > @@ -28,6 +28,12 @@ PASS context.drawElements(context.TRIANG > PASS context.drawElements(context.TRIANGLES, 0xffffffff, context.UNSIGNED_BYTE, 0) generated expected GL error: INVALID_VALUE. > PASS context.drawElements(context.TRIANGLES, 0x7fffffff, context.UNSIGNED_BYTE, 0) generated expected GL error: INVALID_OPERATION. > PASS context.drawElements(context.TRIANGLES, 0x7fffffff, context.UNSIGNED_BYTE, 0x7fffffff) generated expected GL error: INVALID_OPERATION. > +PASS context.bufferData(context.ELEMENT_ARRAY_BUFFER, (new Uint8Array([ 3, 0, 1, 2 ])).slice(1), context.STATIC_DRAW) generated expected GL error: NO_ERROR. > +PASS context.drawElements(context.TRIANGLES, 3, context.UNSIGNED_BYTE, 0) generated expected GL error: NO_ERROR. > +PASS context.bufferSubData(context.ELEMENT_ARRAY_BUFFER, 0, new Uint8Array([ 3, 0, 1])) generated expected GL error: NO_ERROR. > +PASS context.drawElements(context.TRIANGLES, 3, context.UNSIGNED_BYTE, 0) generated expected GL error: INVALID_OPERATION. > +PASS context.bufferSubData(context.ELEMENT_ARRAY_BUFFER, 0, (new Uint8Array([ 3, 0, 1, 2 ])).slice(1)) generated expected GL error: NO_ERROR. > +PASS context.drawElements(context.TRIANGLES, 3, context.UNSIGNED_BYTE, 0) generated expected GL error: NO_ERROR. > > Test buffer with interleaved (3+2) float vectors > PASS context.drawElements(context.TRIANGLES, 9, context.UNSIGNED_SHORT, 0) generated expected GL error: NO_ERROR. > Index: LayoutTests/fast/canvas/webgl/draw-elements-out-of-bounds.html > =================================================================== > --- LayoutTests/fast/canvas/webgl/draw-elements-out-of-bounds.html (revision 64660) > +++ LayoutTests/fast/canvas/webgl/draw-elements-out-of-bounds.html (working copy) > @@ -55,6 +55,13 @@ shouldGenerateGLError(context, context.I > shouldGenerateGLError(context, context.INVALID_OPERATION, "context.drawElements(context.TRIANGLES, 0x7fffffff, context.UNSIGNED_BYTE, 0)"); > shouldGenerateGLError(context, context.INVALID_OPERATION, "context.drawElements(context.TRIANGLES, 0x7fffffff, context.UNSIGNED_BYTE, 0x7fffffff)"); > > +shouldGenerateGLError(context, context.NO_ERROR, "context.bufferData(context.ELEMENT_ARRAY_BUFFER, (new Uint8Array([ 3, 0, 1, 2 ])).slice(1), context.STATIC_DRAW)"); > +shouldGenerateGLError(context, context.NO_ERROR, "context.drawElements(context.TRIANGLES, 3, context.UNSIGNED_BYTE, 0)"); > +shouldGenerateGLError(context, context.NO_ERROR, "context.bufferSubData(context.ELEMENT_ARRAY_BUFFER, 0, new Uint8Array([ 3, 0, 1]))"); > +shouldGenerateGLError(context, context.INVALID_OPERATION, "context.drawElements(context.TRIANGLES, 3, context.UNSIGNED_BYTE, 0)"); > +shouldGenerateGLError(context, context.NO_ERROR, "context.bufferSubData(context.ELEMENT_ARRAY_BUFFER, 0, (new Uint8Array([ 3, 0, 1, 2 ])).slice(1))"); > +shouldGenerateGLError(context, context.NO_ERROR, "context.drawElements(context.TRIANGLES, 3, context.UNSIGNED_BYTE, 0)"); > + > debug("") > debug("Test buffer with interleaved (3+2) float vectors") >
Zhenyao Mo
Comment 3 2010-08-04 18:37:27 PDT
(In reply to comment #2) > (From update of attachment 63505 [details]) > Generally looks very good. A few comments / questions. > > > Index: WebCore/ChangeLog > > =================================================================== > > --- WebCore/ChangeLog (revision 64684) > > +++ WebCore/ChangeLog (working copy) > > @@ -1,3 +1,17 @@ > > +2010-08-04 Zhenyao Mo <zmo@google.com> > > + > > + Reviewed by NOBODY (OOPS!). > > + > > + WebGLBuffer::associateBufferData(ArrayBufferView* array) copies incorrect data for sliced views > > + https://bugs.webkit.org/show_bug.cgi?id=42124 > > + > > + * html/canvas/WebGLBuffer.cpp: > > + (WebCore::WebGLBuffer::associateBufferDataImpl): Helper function that's called by all associateBufferData(). > > + (WebCore::WebGLBuffer::associateBufferData): Call associateBufferDataImpl(). > > + (WebCore::WebGLBuffer::associateBufferSubDataImpl): Helper function that's called by all associateBufferSubData(). > > + (WebCore::WebGLBuffer::associateBufferSubData): Call associateBufferSubDataImpl(). > > + * html/canvas/WebGLBuffer.h: Declare helper functions. > > + > > 2010-08-04 Dan Bernstein <mitz@apple.com> > > > > Build fix. > > Index: WebCore/html/canvas/WebGLBuffer.cpp > > =================================================================== > > --- WebCore/html/canvas/WebGLBuffer.cpp (revision 64660) > > +++ WebCore/html/canvas/WebGLBuffer.cpp (working copy) > > @@ -52,148 +52,96 @@ void WebGLBuffer::_deleteObject(Platform > > context()->graphicsContext3D()->deleteBuffer(object); > > } > > > > -bool WebGLBuffer::associateBufferData(int size) > > +bool WebGLBuffer::associateBufferDataImpl(ArrayBuffer* array, unsigned byteOffset, unsigned byteLength) > > { > > - if (!m_target) > > + if (array && byteLength && (byteOffset >= array->byteLength() || array->byteLength() - byteOffset < byteLength)) > > Are these checks sufficient to guard against integer overflow? Should we use the CheckedInt class? Yes, the logic is designed to avoid overflow. However, I agree that using CheckedInt is a better option for readability. Will revise. > > > return false; > > > > - if (m_target == GraphicsContext3D::ELEMENT_ARRAY_BUFFER) { > > - m_byteLength = size; > > + switch (m_target) { > > + case GraphicsContext3D::ELEMENT_ARRAY_BUFFER: > > + m_byteLength = byteLength; > > clearCachedMaxIndices(); > > - m_elementArrayBuffer = ArrayBuffer::create(size, 1); > > - if (!m_elementArrayBuffer) { > > + m_elementArrayBuffer = ArrayBuffer::create(byteLength, 1); > > + if (byteLength && !m_elementArrayBuffer) { > > Would it be better to explicitly test whether byteLength == 0? > > > m_byteLength = 0; > > return false; > > } > > + if (array) { > > + // We must always clone the incoming data because client-side > > + // modifications without calling bufferData or bufferSubData > > + // must never be able to change the validation results. > > + memcpy(static_cast<unsigned char*>(m_elementArrayBuffer->data()), > > + static_cast<unsigned char*>(array->data()) + byteOffset, > > + byteLength); > > + } > > return true; > > - } else if (m_target == GraphicsContext3D::ARRAY_BUFFER) { > > - m_byteLength = size; > > + case GraphicsContext3D::ARRAY_BUFFER: > > + m_byteLength = byteLength; > > return true; > > + default: > > + return false; > > } > > +} > > > > - return false; > > +bool WebGLBuffer::associateBufferData(int size) > > +{ > > + if (size < 0) > > + return false; > > + return associateBufferDataImpl(0, 0, static_cast<unsigned>(size)); > > } > > > > bool WebGLBuffer::associateBufferData(ArrayBuffer* array) > > { > > - if (!m_target) > > - return false; > > if (!array) > > return false; > > - > > - if (m_target == GraphicsContext3D::ELEMENT_ARRAY_BUFFER) { > > - clearCachedMaxIndices(); > > - m_byteLength = array->byteLength(); > > - // We must always clone the incoming data because client-side > > - // modifications without calling bufferData or bufferSubData > > - // must never be able to change the validation results. > > - m_elementArrayBuffer = ArrayBuffer::create(array); > > - if (!m_elementArrayBuffer) { > > - m_byteLength = 0; > > - return false; > > - } > > - return true; > > - } > > - > > - if (m_target == GraphicsContext3D::ARRAY_BUFFER) { > > - m_byteLength = array->byteLength(); > > - return true; > > - } > > - > > - return false; > > + return associateBufferDataImpl(array, 0, array->byteLength()); > > } > > > > bool WebGLBuffer::associateBufferData(ArrayBufferView* array) > > { > > - if (!m_target) > > - return false; > > if (!array) > > return false; > > - > > - if (m_target == GraphicsContext3D::ELEMENT_ARRAY_BUFFER) { > > - clearCachedMaxIndices(); > > - m_byteLength = array->byteLength(); > > - // We must always clone the incoming data because client-side > > - // modifications without calling bufferData or bufferSubData > > - // must never be able to change the validation results. > > - m_elementArrayBuffer = ArrayBuffer::create(array->buffer().get()); > > - if (!m_elementArrayBuffer) { > > - m_byteLength = 0; > > - return false; > > - } > > - return true; > > - } > > - > > - if (m_target == GraphicsContext3D::ARRAY_BUFFER) { > > - m_byteLength = array->byteLength(); > > - return true; > > - } > > - > > - return false; > > + return associateBufferDataImpl(array->buffer().get(), array->byteOffset(), array->byteLength()); > > } > > > > -bool WebGLBuffer::associateBufferSubData(long offset, ArrayBuffer* array) > > +bool WebGLBuffer::associateBufferSubDataImpl(long offset, ArrayBuffer* array, unsigned arrayByteOffset, unsigned arrayByteLength) > > { > > - if (!m_target) > > + if (!array || offset < 0) > > return false; > > - if (!array) > > + if (arrayByteLength && (arrayByteOffset >= array->byteLength() || array->byteLength() - arrayByteOffset < arrayByteLength)) > > Same question as above about whether we should use CheckedInt. > > > + return false; > > + unsigned long uoffset = static_cast<unsigned long>(offset); > > + if (arrayByteLength && (uoffset >= m_byteLength || arrayByteLength > m_byteLength - uoffset)) > > Same question about CheckedInt. > > > return false; > > > > - if (m_target == GraphicsContext3D::ELEMENT_ARRAY_BUFFER) { > > + switch (m_target) { > > + case GraphicsContext3D::ELEMENT_ARRAY_BUFFER: > > clearCachedMaxIndices(); > > - > > - // We need to protect against integer overflow with these tests > > - if (offset < 0) > > - return false; > > - > > - unsigned long uoffset = static_cast<unsigned long>(offset); > > - if (uoffset > m_byteLength || array->byteLength() > m_byteLength - uoffset) > > - return false; > > - > > if (!m_elementArrayBuffer) > > return false; > > - > > memcpy(static_cast<unsigned char*>(m_elementArrayBuffer->data()) + offset, > > - static_cast<unsigned char*>(array->data()), > > - array->byteLength()); > > + static_cast<unsigned char*>(array->data()) + arrayByteOffset, > > + arrayByteLength); > > + return true; > > + case GraphicsContext3D::ARRAY_BUFFER: > > return true; > > + default: > > + return false; > > } > > +} > > > > - if (m_target == GraphicsContext3D::ARRAY_BUFFER) > > - return array->byteLength() + offset <= m_byteLength; > > - > > - return false; > > +bool WebGLBuffer::associateBufferSubData(long offset, ArrayBuffer* array) > > +{ > > + if (!array) > > + return false; > > + return associateBufferSubDataImpl(offset, array, 0, array->byteLength()); > > } > > > > bool WebGLBuffer::associateBufferSubData(long offset, ArrayBufferView* array) > > { > > - if (!m_target) > > - return false; > > if (!array) > > return false; > > - > > - if (m_target == GraphicsContext3D::ELEMENT_ARRAY_BUFFER) { > > - clearCachedMaxIndices(); > > - > > - // We need to protect against integer overflow with these tests > > - if (offset < 0) > > - return false; > > - > > - unsigned long uoffset = static_cast<unsigned long>(offset); > > - if (uoffset > m_byteLength || array->byteLength() > m_byteLength - uoffset) > > - return false; > > - > > - if (!m_elementArrayBuffer) > > - return false; > > - > > - memcpy(static_cast<unsigned char*>(m_elementArrayBuffer->data()) + offset, array->baseAddress(), array->byteLength()); > > - return true; > > - } > > - > > - if (m_target == GraphicsContext3D::ARRAY_BUFFER) > > - return array->byteLength() + offset <= m_byteLength; > > - > > - return false; > > + return associateBufferSubDataImpl(offset, array->buffer().get(), array->byteOffset(), array->byteLength()); > > } > > > > unsigned WebGLBuffer::byteLength() const > > Index: WebCore/html/canvas/WebGLBuffer.h > > =================================================================== > > --- WebCore/html/canvas/WebGLBuffer.h (revision 64660) > > +++ WebCore/html/canvas/WebGLBuffer.h (working copy) > > @@ -90,6 +90,11 @@ namespace WebCore { > > > > // Clears all of the cached max indices. > > void clearCachedMaxIndices(); > > + > > + // Helper function called by the three associateBufferData(). > > + bool associateBufferDataImpl(ArrayBuffer* array, unsigned byteOffset, unsigned byteLength); > > + // Helper function called by the two associateBufferSubData(). > > + bool associateBufferSubDataImpl(long offset, ArrayBuffer* array, unsigned arrayByteOffset, unsigned arrayByteLength); > > }; > > > > } // namespace WebCore > > Index: LayoutTests/ChangeLog > > =================================================================== > > --- LayoutTests/ChangeLog (revision 64684) > > +++ LayoutTests/ChangeLog (working copy) > > @@ -1,3 +1,13 @@ > > +2010-08-04 Zhenyao Mo <zmo@google.com> > > + > > + Reviewed by NOBODY (OOPS!). > > + > > + WebGLBuffer::associateBufferData(ArrayBufferView* array) copies incorrect data for sliced views > > + https://bugs.webkit.org/show_bug.cgi?id=42124 > > + > > + * fast/canvas/webgl/draw-elements-out-of-bounds-expected.txt: Adding test case for bufferData and bufferSubData with ArrayBufferView input. > > + * fast/canvas/webgl/draw-elements-out-of-bounds.html: Ditto. > > + > > 2010-08-04 Dan Bernstein <mitz@apple.com> > > > > Reviewed by Simon Fraser. > > Index: LayoutTests/fast/canvas/webgl/draw-elements-out-of-bounds-expected.txt > > =================================================================== > > --- LayoutTests/fast/canvas/webgl/draw-elements-out-of-bounds-expected.txt (revision 64660) > > +++ LayoutTests/fast/canvas/webgl/draw-elements-out-of-bounds-expected.txt (working copy) > > @@ -28,6 +28,12 @@ PASS context.drawElements(context.TRIANG > > PASS context.drawElements(context.TRIANGLES, 0xffffffff, context.UNSIGNED_BYTE, 0) generated expected GL error: INVALID_VALUE. > > PASS context.drawElements(context.TRIANGLES, 0x7fffffff, context.UNSIGNED_BYTE, 0) generated expected GL error: INVALID_OPERATION. > > PASS context.drawElements(context.TRIANGLES, 0x7fffffff, context.UNSIGNED_BYTE, 0x7fffffff) generated expected GL error: INVALID_OPERATION. > > +PASS context.bufferData(context.ELEMENT_ARRAY_BUFFER, (new Uint8Array([ 3, 0, 1, 2 ])).slice(1), context.STATIC_DRAW) generated expected GL error: NO_ERROR. > > +PASS context.drawElements(context.TRIANGLES, 3, context.UNSIGNED_BYTE, 0) generated expected GL error: NO_ERROR. > > +PASS context.bufferSubData(context.ELEMENT_ARRAY_BUFFER, 0, new Uint8Array([ 3, 0, 1])) generated expected GL error: NO_ERROR. > > +PASS context.drawElements(context.TRIANGLES, 3, context.UNSIGNED_BYTE, 0) generated expected GL error: INVALID_OPERATION. > > +PASS context.bufferSubData(context.ELEMENT_ARRAY_BUFFER, 0, (new Uint8Array([ 3, 0, 1, 2 ])).slice(1)) generated expected GL error: NO_ERROR. > > +PASS context.drawElements(context.TRIANGLES, 3, context.UNSIGNED_BYTE, 0) generated expected GL error: NO_ERROR. > > > > Test buffer with interleaved (3+2) float vectors > > PASS context.drawElements(context.TRIANGLES, 9, context.UNSIGNED_SHORT, 0) generated expected GL error: NO_ERROR. > > Index: LayoutTests/fast/canvas/webgl/draw-elements-out-of-bounds.html > > =================================================================== > > --- LayoutTests/fast/canvas/webgl/draw-elements-out-of-bounds.html (revision 64660) > > +++ LayoutTests/fast/canvas/webgl/draw-elements-out-of-bounds.html (working copy) > > @@ -55,6 +55,13 @@ shouldGenerateGLError(context, context.I > > shouldGenerateGLError(context, context.INVALID_OPERATION, "context.drawElements(context.TRIANGLES, 0x7fffffff, context.UNSIGNED_BYTE, 0)"); > > shouldGenerateGLError(context, context.INVALID_OPERATION, "context.drawElements(context.TRIANGLES, 0x7fffffff, context.UNSIGNED_BYTE, 0x7fffffff)"); > > > > +shouldGenerateGLError(context, context.NO_ERROR, "context.bufferData(context.ELEMENT_ARRAY_BUFFER, (new Uint8Array([ 3, 0, 1, 2 ])).slice(1), context.STATIC_DRAW)"); > > +shouldGenerateGLError(context, context.NO_ERROR, "context.drawElements(context.TRIANGLES, 3, context.UNSIGNED_BYTE, 0)"); > > +shouldGenerateGLError(context, context.NO_ERROR, "context.bufferSubData(context.ELEMENT_ARRAY_BUFFER, 0, new Uint8Array([ 3, 0, 1]))"); > > +shouldGenerateGLError(context, context.INVALID_OPERATION, "context.drawElements(context.TRIANGLES, 3, context.UNSIGNED_BYTE, 0)"); > > +shouldGenerateGLError(context, context.NO_ERROR, "context.bufferSubData(context.ELEMENT_ARRAY_BUFFER, 0, (new Uint8Array([ 3, 0, 1, 2 ])).slice(1))"); > > +shouldGenerateGLError(context, context.NO_ERROR, "context.drawElements(context.TRIANGLES, 3, context.UNSIGNED_BYTE, 0)"); > > + > > debug("") > > debug("Test buffer with interleaved (3+2) float vectors") > >
Zhenyao Mo
Comment 4 2010-08-04 19:17:39 PDT
Created attachment 63530 [details] revised patch: responding to kbr's review
WebKit Review Bot
Comment 5 2010-08-04 19:20:16 PDT
Attachment 63530 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/html/canvas/WebGLBuffer.cpp:30: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Zhenyao Mo
Comment 6 2010-08-04 19:20:41 PDT
Created attachment 63532 [details] revised patch: tiny modification
Zhenyao Mo
Comment 7 2010-08-04 19:22:24 PDT
Created attachment 63533 [details] revised patch: fix a style issue
WebKit Review Bot
Comment 8 2010-08-04 19:22:54 PDT
Attachment 63532 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/html/canvas/WebGLBuffer.cpp:30: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Kenneth Russell
Comment 9 2010-08-05 12:26:41 PDT
Comment on attachment 63533 [details] revised patch: fix a style issue Looks good to me.
Dimitri Glazkov (Google)
Comment 10 2010-08-05 14:43:34 PDT
Comment on attachment 63533 [details] revised patch: fix a style issue ok.
Zhenyao Mo
Comment 11 2010-08-06 11:04:37 PDT
Note You need to log in before you can comment on or make changes to this bug.