[chromium] Properly handle huge initial dimensions for 2d canvas when accelerated 2d canvas is enabled

[James Robinson]

[chromium] Properly handle huge initial dimensions for 2d canvas when accelerated 2d canvas is enabled

[James Robinson]

Created attachment 92155 [details]
Patch

[Kenneth Russell]

Comment on attachment 92155 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92155&action=review

> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:101
> +    if (!reset(size)) {
> +        m_context.clear();
> +        return;
> +    }

The implementation of reset() references m_fbo which isn't initialized until the following line. Was this patch tested?

[James Robinson]

(In reply to comment #2)
> (From update of attachment 92155 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92155&action=review
> 
> > Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:101
> > +    if (!reset(size)) {
> > +        m_context.clear();
> > +        return;
> > +    }
> 
> The implementation of reset() references m_fbo which isn't initialized until the following line. Was this patch tested?

Whoops, I tested cases where the reset() fails but didn't test the case where it succeeds carefully enough.  Good catch.

[James Robinson]

Created attachment 94532 [details]
Patch

[James Robinson]

(In reply to comment #2)
> (From update of attachment 92155 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92155&action=review
> 
> > Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:101
> > +    if (!reset(size)) {
> > +        m_context.clear();
> > +        return;
> > +    }
> 
> The implementation of reset() references m_fbo which isn't initialized until the following line. Was this patch tested?

Good catch.

Patch updated.  if m_context is cleared then the ::create() function returns 0 and the DrawingBuffer and its resources are released.

[Kenneth Russell]

Comment on attachment 94532 [details]
Patch

Looks fine. I do wonder whether we should attempt to allocate the color buffer (generateColorTexture) at all if the size is too large, but it should fail with an OpenGL error and the cleanup code should work.

[James Robinson]

(In reply to comment #6)
> (From update of attachment 94532 [details])
> Looks fine. I do wonder whether we should attempt to allocate the color buffer (generateColorTexture) at all if the size is too large, but it should fail with an OpenGL error and the cleanup code should work.

I think it'd be better to not attempt to create it, but everything should work correctly even if it fails.  This path is not gonna be fast or resource light no matter what we do :)

[WebKit Commit Bot]

Comment on attachment 94532 [details]
Patch

Clearing flags on attachment: 94532

Committed r87136: <http://trac.webkit.org/changeset/87136>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.