Summary: | [chromium] Properly handle huge initial dimensions for 2d canvas when accelerated 2d canvas is enabled | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Robinson <jamesr> | ||||||
Component: | New Bugs | Assignee: | James Robinson <jamesr> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, enne, kbr, senorblanco, vangelis | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Other | ||||||||
OS: | OS X 10.5 | ||||||||
Attachments: |
|
Description
James Robinson
2011-05-03 16:04:13 PDT
Created attachment 92155 [details]
Patch
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? (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. Created attachment 94532 [details]
Patch
(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. 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.
(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 :) Comment on attachment 94532 [details] Patch Clearing flags on attachment: 94532 Committed r87136: <http://trac.webkit.org/changeset/87136> All reviewed patches have been landed. Closing bug. |