WebGLBuffer::associateBufferData(ArrayBufferView* array) copies incorrect data for sliced views

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

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.

[Zhenyao Mo]

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 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 @@
> [phone]  Zhenyao Mo  <[e-mail]>
> +
> +        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  <[e-mail]>
>  
>          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 @@
> [phone]  Zhenyao Mo  <[e-mail]>
> +
> +        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  <[e-mail]>
>  
>          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]

(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 @@
> > [phone]  Zhenyao Mo  <[e-mail]>
> > +
> > +        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  <[e-mail]>
> >  
> >          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 @@
> > [phone]  Zhenyao Mo  <[e-mail]>
> > +
> > +        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  <[e-mail]>
> >  
> >          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]

Created attachment 63530 [details]
revised patch: responding to kbr's review

[WebKit Review Bot]

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]

Created attachment 63532 [details]
revised patch: tiny modification

[Zhenyao Mo]

Created attachment 63533 [details]
revised patch: fix a style issue

[WebKit Review Bot]

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 on attachment 63533 [details]
revised patch: fix a style issue

Looks good to me.

[Dimitri Glazkov (Google)]

Comment on attachment 63533 [details]
revised patch: fix a style issue

ok.

[Zhenyao Mo]

Committed r64859: <http://trac.webkit.org/changeset/64859>