Bug 31239 - c3d test crash WebKit
Summary: c3d test crash WebKit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Chris Marrin
URL: http://cs.helsinki.fi/u/ilmarihe/c3d/...
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-08 10:21 PST by Simon Fraser (smfr)
Modified: 2009-11-23 16:36 PST (History)
0 users

See Also:


Attachments
Patch (47.92 KB, patch)
2009-11-13 10:13 PST, Chris Marrin
simon.fraser: review-
Details | Formatted Diff | Diff
Patch (58.42 KB, patch)
2009-11-13 16:15 PST, Chris Marrin
oliver: review-
Details | Formatted Diff | Diff
Replacement patch with test cases (69.17 KB, patch)
2009-11-23 13:50 PST, Chris Marrin
oliver: review-
Details | Formatted Diff | Diff
Desperate attempt at acceptance patch (61.57 KB, patch)
2009-11-23 14:44 PST, Chris Marrin
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2009-11-08 10:21:29 PST
When I load the page in the URL, webkit crashes. Stack trace coming...
Comment 1 Simon Fraser (smfr) 2009-11-08 10:22:08 PST
Crash is at:
Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x000000000000000c
Crashed Thread:  0  Dispatch queue: com.apple.main-thread

Thread 0 Crashed:  Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x00000001009e460e WebCore::GraphicsContext3D::useProgram(WebCore::CanvasProgram*) + 46
1   com.apple.WebCore             	0x0000000100784e65 WebCore::CanvasRenderingContext3D::useProgram(WebCore::CanvasProgram*) + 21
2   com.apple.WebCore             	0x0000000100aec991 WebCore::jsCanvasRenderingContext3DPrototypeFunctionUseProgram(JSC::ExecState*, JSC::JSObject*, JSC::JSValue, JSC::ArgList const&) + 129
3   ???                           	0x00003978462001c4 0 + 63188735361476
4   com.apple.JavaScriptCore      	0x000000010056b119 JSC::Interpreter::execute(JSC::FunctionExecutable*, JSC::ExecState*, JSC::JSFunction*, JSC::JSObject*, JSC::ArgList const&, JSC::ScopeChainNode*, JSC::JSValue*) + 585
5   ???                           	0x0000000116a257c0 0 + 4674705344
6   ???                           	0x0000000120727f78 0 + 4839341944
7   com.apple.WebCore             	0x0000000100b818c0 WebCore::JSDOMWindowShell::~JSDOMWindowShell() + 0
8   ???                           	0x0000441f0f66ffff 0 + 74900193083391
Comment 2 Chris Marrin 2009-11-13 10:13:37 PST
Created attachment 43166 [details]
Patch
Comment 3 Simon Fraser (smfr) 2009-11-13 11:28:26 PST
Comment on attachment 43166 [details]
Patch


From a quick scan:

> Index: WebCore/html/canvas/WebGLArrayBuffer.cpp
> ===================================================================

> +    PassRefPtr<WebGLArrayBuffer> WebGLArrayBuffer::create(WebGLArrayBuffer* other)
> +    {
> +        RefPtr<WebGLArrayBuffer> buffer = adoptRef(new WebGLArrayBuffer(other->byteLength()));
> +        memcpy(buffer->data(), other->data(), other->byteLength());
> +        return buffer;

The normal idiom is 
  return buffer.release();

> Index: WebCore/html/canvas/WebGLBuffer.cpp
> ===================================================================

>  WebGLBuffer::WebGLBuffer(WebGLRenderingContext* ctx)
> -    : CanvasObject(ctx)
> + : CanvasObject(ctx)
> + , m_elementArrayBufferByteLength(0)
> + , m_arrayBufferByteLength(0)

The old indentation was correct.

> +bool WebGLBuffer::associateBufferData(unsigned long target, int size)
> +{
> +    if (target == GraphicsContext3D::ELEMENT_ARRAY_BUFFER)
> +        m_elementArrayBufferByteLength = size;
> +    else if (target == GraphicsContext3D::ARRAY_BUFFER)
> +        m_arrayBufferByteLength = size;
> +    else
> +        return false;
> +    return true;
> +}

This would be clearer with early returns.

> +
> +bool WebGLBuffer::associateBufferData(unsigned long target, WebGLArray* array)
> +{
> +    if (!array)
> +        return false;
> +        
> +    if (target == GraphicsContext3D::ELEMENT_ARRAY_BUFFER) {
> +        m_elementArrayBufferByteLength = array->sizeInBytes();
> +        m_elementArrayBuffer = array->buffer();
> +    }
> +    else if (target == GraphicsContext3D::ARRAY_BUFFER)
> +        m_arrayBufferByteLength = array->sizeInBytes();
> +    else
> +        return false;
> +    return true;
> +}

This would be clearer with early returns.

> +bool WebGLBuffer::associateBufferSubData(unsigned long target, long offset, WebGLArray* array)
> +{
> +    if (!array)
> +        return false;
> +        
> +    if (target == GraphicsContext3D::ELEMENT_ARRAY_BUFFER) {
> +        if (array->sizeInBytes() + offset > m_elementArrayBufferByteLength)
> +            return false;

What if offset is a large negative number, or MAX_LONG?

> +        // If we already have a buffer, we need to clone it and add the new data
> +        if (m_elementArrayBuffer)
> +            m_elementArrayBuffer = WebGLArrayBuffer::create(m_elementArrayBuffer.get());
> +            
> +        memcpy(static_cast<unsigned char*>(m_elementArrayBuffer->data()) + offset, array->baseAddress(), array->sizeInBytes());
> +    }
> +    else if (target == GraphicsContext3D::ARRAY_BUFFER) {
> +        if (array->sizeInBytes() + offset > m_arrayBufferByteLength)
> +            return false;
> +    }
> +    else
> +        return false;
> +    return true;
> +}

This would be clearer with early returns.

> Index: WebCore/html/canvas/WebGLRenderingContext.cpp
> ===================================================================

> +bool WebGLRenderingContext::validateIndexArray(unsigned long count, unsigned long type, long offset, long& numElements)
> +{
> +    long lastIndex = -1;
> +    if (!m_boundElementArrayBuffer)
> +        return false;
> +    
> +    if (type == GraphicsContext3D::UNSIGNED_SHORT) {
> +        int n = m_boundElementArrayBuffer->byteLength(GraphicsContext3D::ELEMENT_ARRAY_BUFFER);
> +        if ((long) count * 2 + offset > n)
> +            return false;

You risk integer overflow here.

> +        n /= 2;
> +        const unsigned short* p = static_cast<const unsigned short*>(m_boundElementArrayBuffer->elementArrayBuffer()->data());
> +        while (n-- > 0) {
> +            if (*p > lastIndex)
> +                lastIndex = *p;
> +            ++p;
> +        }
> +    }
> +    else if (type == GraphicsContext3D::UNSIGNED_BYTE) {
> +        int n = m_boundElementArrayBuffer->byteLength(GraphicsContext3D::ELEMENT_ARRAY_BUFFER);
> +        const unsigned char* p = static_cast<const unsigned char*>(m_boundElementArrayBuffer->elementArrayBuffer()->data());
> +        if ((long) count + offset > n)
> +            return false;

You risk integer overflow here too.

> +    if (smallestNumElements == 1e9)
> +        smallestNumElements = 0;

1e9? Should that be a named constant?


> -void WebGLRenderingContext::vertexAttribPointer(unsigned long indx, long size, unsigned long type, bool normalized, unsigned long stride, unsigned long offset)
> +void WebGLRenderingContext::vertexAttribPointer(unsigned long indx, long size, unsigned long type, bool normalized, unsigned long stride, unsigned long offset, ExceptionCode& ec)

indx is a pretty ugly name.

> +    if (indx >= m_vertexAttribState.size())
> +        m_vertexAttribState.resize(indx + 1);

What if indx is MAX_UNSIGNED_LONG? (Here and elsewhere).

> Index: WebCore/html/canvas/WebGLShader.h
> ===================================================================
> --- WebCore/html/canvas/WebGLShader.h	(revision 50913)
> +++ WebCore/html/canvas/WebGLShader.h	(working copy)
> @@ -37,10 +37,10 @@ namespace WebCore {
>      public:
>          virtual ~WebGLShader() { deleteObject(); }
>          
> -        static PassRefPtr<WebGLShader> create(WebGLRenderingContext*, GraphicsContext3D::ShaderType);
> +        static PassRefPtr<WebGLShader> create(WebGLRenderingContext*, unsigned long);
>          
>      private:
> -        WebGLShader(WebGLRenderingContext*, GraphicsContext3D::ShaderType);
> +        WebGLShader(WebGLRenderingContext*, unsigned long);

These changes seem like a regression in readability, and I don't see any notes in the Changelog about why they were changed.
Comment 4 Chris Marrin 2009-11-13 16:15:35 PST
Created attachment 43212 [details]
Patch

Updated patch addressing issues from Simon
Comment 5 Chris Marrin 2009-11-13 16:46:20 PST
Simon pointed out that there is an errant, unfinished comment in WebGLRenderingContext. I also forgot to change createShader() to include the new WebGLEnumType. These will be fixed before landing.
Comment 6 Oliver Hunt 2009-11-13 17:00:12 PST
Comment on attachment 43212 [details]
Patch

Minor formatting bug
> +bool WebGLRenderingContext::validateIndexArray(unsigned long count, unsigned long type, long offset, long& numElements)
> +{
...
> +    }
> +    else if

File a bug on createShader -- it will accept any non-FRAGMENT_SHADER value as GL_VERTEX_SHADER

r- until there are testcases for all of these as well :-(

--Oliver
Comment 7 Chris Marrin 2009-11-23 13:50:56 PST
Created attachment 43732 [details]
Replacement patch with test cases

I also added a bug as noted by Ollie (https://bugs.webkit.org/show_bug.cgi?id=31808)
Comment 8 Oliver Hunt 2009-11-23 14:00:14 PST
Comment on attachment 43732 [details]
Replacement patch with test cases

r- due to the large number of unnecessary changes (the indx->index renames)
Comment 9 Chris Marrin 2009-11-23 14:44:37 PST
Created attachment 43736 [details]
Desperate attempt at acceptance patch

One more try with spurious indx->index change reverted
Comment 10 Oliver Hunt 2009-11-23 15:05:37 PST
Comment on attachment 43736 [details]
Desperate attempt at acceptance patch

WebGLRenderingContext::validateIndexArray

has an else start on the line after }

You've removed the type checks from createShader, which is a bad thing.

r- mostly due to removing the type checks
Comment 11 Oliver Hunt 2009-11-23 15:40:41 PST
Comment on attachment 43736 [details]
Desperate attempt at acceptance patch

r=me, assuming you fix the style error, and fix bug 31808 quickly
Comment 12 Chris Marrin 2009-11-23 16:36:03 PST
Sending        LayoutTests/ChangeLog
Adding         LayoutTests/fast/canvas/webgl/drawArraysOutOfBounds-expected.txt
Adding         LayoutTests/fast/canvas/webgl/drawArraysOutOfBounds.html
Adding         LayoutTests/fast/canvas/webgl/drawElementssOutOfBounds-expected.txt
Adding         LayoutTests/fast/canvas/webgl/drawElementssOutOfBounds.html
Sending        WebCore/ChangeLog
Sending        WebCore/bindings/js/JSWebGLArrayCustom.cpp
Sending        WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp
Sending        WebCore/html/canvas/WebGLArrayBuffer.cpp
Sending        WebCore/html/canvas/WebGLArrayBuffer.h
Sending        WebCore/html/canvas/WebGLBuffer.cpp
Sending        WebCore/html/canvas/WebGLBuffer.h
Sending        WebCore/html/canvas/WebGLRenderingContext.cpp
Sending        WebCore/html/canvas/WebGLRenderingContext.h
Sending        WebCore/html/canvas/WebGLRenderingContext.idl
Sending        WebCore/html/canvas/WebGLShader.cpp
Sending        WebCore/html/canvas/WebGLShader.h
Sending        WebCore/platform/graphics/GraphicsContext3D.h
Sending        WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp
Transmitting file data ...................
Committed revision 51327.