destroyMetadataAndNotify sets m_isSolidColor flag to false but does not reset m_checkedForSolidColor. This means that a subsequent call to mayFillWithSolidColor would not call checkForSolidColor() and could end up returning false instead of true for a 1x1 image.
<rdar://problem/9140052>
Created attachment 90252 [details] Patch
Created attachment 90272 [details] Patch
Comment on attachment 90272 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=90272&action=review r=me either way > Source/WebCore/platform/graphics/BitmapImage.h:167 > +#if !ASSERT_DISABLED > + void assertNotSolidColor() > + { > + ASSERT(size().width() != 1 || size().height() != 1 || frameCount() > 1); > + } > +#endif You could make this notSolidColor() const... > Source/WebCore/platform/graphics/cg/ImageCG.cpp:273 > + static_cast<BitmapImage*>(this)->assertNotSolidColor(); ... and here just ASSERT(notSolidColor())
(In reply to comment #4) > (From update of attachment 90272 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=90272&action=review > > r=me either way > > > Source/WebCore/platform/graphics/BitmapImage.h:167 > > +#if !ASSERT_DISABLED > > + void assertNotSolidColor() > > + { > > + ASSERT(size().width() != 1 || size().height() != 1 || frameCount() > 1); > > + } > > +#endif > > You could make this notSolidColor() const... Couldn't make it const because we call franmeCount() that's not a const method. But I(In reply to comment #4) > (From update of attachment 90272 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=90272&action=review > > r=me either way > > > Source/WebCore/platform/graphics/BitmapImage.h:167 > > +#if !ASSERT_DISABLED > > + void assertNotSolidColor() > > + { > > + ASSERT(size().width() != 1 || size().height() != 1 || frameCount() > 1); > > + } > > +#endif > > You could make this notSolidColor() const... > > > Source/WebCore/platform/graphics/cg/ImageCG.cpp:273 > > + static_cast<BitmapImage*>(this)->assertNotSolidColor(); > > ... and here just ASSERT(notSolidColor()) Thanks for the review. I made the change you suggested. I couldn't make notSolidColor() be a const method since we need to call frameCount() which isn't const.
Committed r84321 - <http://trac.webkit.org/changeset/84321>
I hit the assertion on LayoutTests/fast/backgrounds/background-fill-zero-area-crash.html with this change.
Oops. I'll take a look at what's happening.
http://trac.webkit.org/changeset/84321 might have broken Leopard Intel Debug (Tests) The following tests are not passing: fast/backgrounds/background-fill-zero-area-crash.html
The stacktrace looks like (gdb) bt #0 0x00000001015af030 in WebCore::Image::drawPattern (this=0x1063f2650, ctxt=0x7fff5fbfc1b0, tileRect=@0x7fff5fbfac70, patternTransform=@0x7fff5fbfadc0, phase=@0x7fff5fbfae20, styleColorSpace=WebCore::ColorSpaceDeviceRGB, op=WebCore::CompositeSourceOver, destRect=@0x7fff5fbfaef0) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/platform/graphics/cg/ImageCG.cpp:273 #1 0x00000001015acb18 in WebCore::ImageBuffer::drawPattern (this=0x1063ef2c0, destContext=0x7fff5fbfc1b0, srcRect=@0x7fff5fbfac70, patternTransform=@0x7fff5fbfadc0, phase=@0x7fff5fbfae20, styleColorSpace=WebCore::ColorSpaceDeviceRGB, op=WebCore::CompositeSourceOver, destRect=@0x7fff5fbfaef0) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:229 #2 0x000000010144961f in WebCore::GeneratedImage::drawPattern (this=0x1063ee520, context=0x7fff5fbfc1b0, srcRect=@0x7fff5fbfadf0, patternTransform=@0x7fff5fbfadc0, phase=@0x7fff5fbfae20, styleColorSpace=WebCore::ColorSpaceDeviceRGB, compositeOp=WebCore::CompositeSourceOver, destRect=@0x7fff5fbfaef0) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/platform/graphics/GeneratedImage.cpp:68 #3 0x00000001015abae6 in WebCore::Image::drawTiled (this=0x1063ee520, ctxt=0x7fff5fbfc1b0, destRect=@0x7fff5fbfaef0, srcPoint=@0x7fff5fbfaf20, scaledTileSize=@0x7fff5fbfaf10, styleColorSpace=WebCore::ColorSpaceDeviceRGB, op=WebCore::CompositeSourceOver) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/platform/graphics/Image.cpp:142 Since its a GeneratedImage, the call to mayFillWithSolidColor() returns false in Image::drawTiled(). Then we come down to ImageBuffer::drawPattern() where we create a new BitmapImage (1x1) and call drawPattern() on it which results in this assertion being triggered. Maybe I should just move this assertion to be in Image::drawPattern() right after the call to mayFillWithSolidColor(). Or maybe ImageBuffer::drawPattern() should check for mayFillWithSolidColor() as well?
Created attachment 90307 [details] Fix assertion
(In reply to comment #10) > Maybe I should just move this assertion to be in Image::drawPattern() right after the call to mayFillWithSolidColor(). This is what I ended up doing in the patch attached. > Or maybe ImageBuffer::drawPattern() should check for mayFillWithSolidColor() as well? Couldn't do this in ImageBuffer because mayFillWithSolidColor() is protected, not public.
Comment on attachment 90307 [details] Fix assertion View in context: https://bugs.webkit.org/attachment.cgi?id=90307&action=review > Source/WebCore/platform/graphics/Image.cpp:117 > + if (this->isBitmapImage()) > + ASSERT(static_cast<BitmapImage*>(this)->notSolidColor()); How about ASSERT(!isBitmapImage() || notSolidColor()) ? Then you don't need the ASSERT_DISABLED stuff. Why the this-> ?
Follow on assertion fix committed as r84344 - <http://trac.webkit.org/changeset/84344>
(In reply to comment #13) > (From update of attachment 90307 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=90307&action=review > > > Source/WebCore/platform/graphics/Image.cpp:117 > > + if (this->isBitmapImage()) > > + ASSERT(static_cast<BitmapImage*>(this)->notSolidColor()); > > How about ASSERT(!isBitmapImage() || notSolidColor()) ? > Then you don't need the ASSERT_DISABLED stuff. Much better. Thanks. I committed with your suggested change. > Why the this-> ? Not sure why I wrote that. Maybe a habit from my java days. I removed it.
(In reply to comment #14) > Follow on assertion fix committed as r84344 - <http://trac.webkit.org/changeset/84344> With this commit, now I see the assertion failing on ToT Chromium for http://en.wikipedia.org/wiki/T._S._Eliot
(In reply to comment #16) > (In reply to comment #14) > > Follow on assertion fix committed as r84344 - <http://trac.webkit.org/changeset/84344> > > With this commit, now I see the assertion failing on ToT Chromium for http://en.wikipedia.org/wiki/T._S._Eliot This assertion also trips in Chromium in debug mode on fast/table/stale-grid-crash.html
(In reply to comment #17) > (In reply to comment #16) > > (In reply to comment #14) > > > Follow on assertion fix committed as r84344 - <http://trac.webkit.org/changeset/84344> > > > > With this commit, now I see the assertion failing on ToT Chromium for http://en.wikipedia.org/wiki/T._S._Eliot > > This assertion also trips in Chromium in debug mode on fast/table/stale-grid-crash.html I'm not seeing the assert triggered on debug mac builds on either of the 2 pages above. The code in Image::drawTiled() is if (mayFillWithSolidColor()) { fillWithSolidColor(ctxt, destRect, solidColor(), styleColorSpace, op); return; } ASSERT(!isBitmapImage() || static_cast<BitmapImage*>(this)->notSolidColor()); The fact that this is triggered means that we had a 1x1 image which isn't getting filled by a solid color. Looking at ImageSkia.cpp, I see void BitmapImage::checkForSolidColor() { m_checkedForSolidColor = true; } No wonder its failing. That method on other ports checks if the image is 1x1 and if so it sets m_isSolidColor to true and sets m_solidColor to the color.
Skia and wx are the two ports that don't have a proper implementation for checkForSolidColor(). I'll wrap the assert with proper ifdefs.
Created attachment 90449 [details] Try to fix assertion failure on Chromium
Comment on attachment 90449 [details] Try to fix assertion failure on Chromium View in context: https://bugs.webkit.org/attachment.cgi?id=90449&action=review > Source/WebCore/platform/graphics/Image.cpp:115 > + // See bugs 59041 and 59043. Maybe nicer to have bug URLs here.
Committed Chromium fix r84468 - <http://trac.webkit.org/changeset/84468>
Created attachment 90514 [details] Patch
Comment on attachment 90514 [details] Patch Clearing flags on attachment: 90514 Committed r84488: <http://trac.webkit.org/changeset/84488>
All reviewed patches have been landed. Closing bug.
*** Bug 59148 has been marked as a duplicate of this bug. ***