ImageDecoder setSize() should check for backing store allocation failure.
Created attachment 116045 [details] Patch
Created attachment 116066 [details] Patch
Not sure we have a super-sized image test. Adding Mihai due to bug 48634, and folks from other ports.
Comment on attachment 116066 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116066&action=review Good idea! > Source/WebCore/ChangeLog:20 > + No new tests. Covered by existing tests. Is it really? If not, it should be pretty easy to construct a gigantic image that still has a small file size. > Source/WebCore/platform/image-decoders/ImageDecoder.cpp:193 > + size_t backingStoreSize = newWidth * newHeight; This could overflow on 32-bit platforms. > Source/WebCore/platform/image-decoders/ImageDecoder.cpp:194 > + if (!m_backingStore.tryReserveCapacity(backingStoreSize)) tryReserveCapacity() will not update Vector::m_size. You need to resize() it after tryReserveCapacity() succeeds.
fast/images/size-failure.html should be testing a large image.
(In reply to comment #4) > (From update of attachment 116066 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116066&action=review > > > Source/WebCore/platform/image-decoders/ImageDecoder.cpp:193 > > + size_t backingStoreSize = newWidth * newHeight; > > This could overflow on 32-bit platforms. D'oh, I completely missed the comment in the ChangeLog about overflow being covered. (In reply to comment #5) > fast/images/size-failure.html should be testing a large image. Great, thanks!
(In reply to comment #5) > fast/images/size-failure.html should be testing a large image. Added you for a reason :) will refer to that test to the ChangeLog. % find . -name Skipped | xargs grep size-failure ./gtk/Skipped:fast/images/size-failure.html ./mac-snowleopard/Skipped:fast/images/size-failure.html gtk covered by bug 72245, mac-snowleopard by a radar bug.
(In reply to comment #6) > (In reply to comment #4) > > (From update of attachment 116066 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=116066&action=review > > > > > Source/WebCore/platform/image-decoders/ImageDecoder.cpp:193 > > > + size_t backingStoreSize = newWidth * newHeight; > > > > This could overflow on 32-bit platforms. > > D'oh, I completely missed the comment in the ChangeLog about overflow being covered. No worries, bug 48634 was instructive and I thought it worth mentioning.
(In reply to comment #4) > (From update of attachment 116066 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116066&action=review > > > Source/WebCore/platform/image-decoders/ImageDecoder.cpp:194 > > + if (!m_backingStore.tryReserveCapacity(backingStoreSize)) > > tryReserveCapacity() will not update Vector::m_size. You need to resize() it after tryReserveCapacity() succeeds. Indeed you are correct. However, the image decoders don't use/require a Vector:m_size. They instead access the underlying Vector buffer() via the m_btyes member ... if (!m_backingStore.tryReserveCapacity(backingStoreSize)) return false; m_bytes = m_backingStore.data(); ... when writing decoded image pixel data into the buffer (m_backingStore). Still I don't mind adding the resize() if necessary. Your point did make me stare at the ASSERT(m_backingStore.isEmpty()) some more - it's wrong - changed it to ASSERT(!m_bytes).
Created attachment 116172 [details] Patch
Anything more I need to do here?
I mean, you could add a comment about not calling resize. I think the difference is that you're not going to call the constructor on all the elements of the vector.
Comment on attachment 116172 [details] Patch Clearing flags on attachment: 116172 Committed r101302: <http://trac.webkit.org/changeset/101302>
All reviewed patches have been landed. Closing bug.
The m_bytes assertion fires on lots of bots - rolling this out for now...
Reopened since the patch was rolled out.
(In reply to comment #15) > The m_bytes assertion fires on lots of bots - rolling this out for now... ImageFrame::ImageFrame() does not initialize m_size at construction time, seems easy to fix. Other ports ASSERT the setSize() contract - that it only be called once - using ASSERT(width() == 0 && height() == 0), so I'll use that. Add comment Adam asked for about not needing to resize the Vector backing store. Preparing a patch ...
ASSERT(width() == 0 && height() == 0) fails to pass webkit check style, so I'll use nots instead.
Created attachment 117068 [details] Patch
(In reply to comment #18) > ASSERT(width() == 0 && height() == 0) fails to pass webkit check style, so I'll > use nots instead. Filed bug 73490 to remove these style nits elsewhere. Here we're just waiting on the gtk ews to return and catch-up.
Question reviewers: is setting m_data to 0 correct? ImageFrame::operator() = is overloaded. It calls ImageFrame::copyBitmapData(const ImageFrame& other), which sets ... m_backingStore = other.m_backingStore; m_bytes = m_backingStore.data(); I would have expected "m_data = other.m_data" here, what am I missing?
Specifically, should we write it like this ... m_backingStore = other.m_backingStore; m_bytes = other.m_data; if (m_bytes) m_bytes = m_backingStore.data();
/shrug/, maybe not touch m_data at all.
Created attachment 117821 [details] Patch for landing
Comment on attachment 117821 [details] Patch for landing Clearing flags on attachment: 117821 Committed r101975: <http://trac.webkit.org/changeset/101975>
Following GIF tests were unhappy on GTK Release bots. Filed bug 73812. fast/images/dont-crash-with-null-gif-frames.html = CRASH fast/backgrounds/animated-gif-as-background.html = CRASH fast/images/gif-loop-count.html = CRASH