Bug 58926

Summary: BitmapImage::destroyMetadataAndNotify should clear m_checkedForSolidColor
Product: WebKit Reporter: Pratik Solanki <psolanki>
Component: WebCore Misc.Assignee: Pratik Solanki <psolanki>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dpranke, eric, jamesr, joepeck, psolanki, simon.fraser, thestig, tonyg, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Fix assertion
none
Try to fix assertion failure on Chromium
none
Patch none

Description Pratik Solanki 2011-04-19 14:10:58 PDT
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.
Comment 1 Pratik Solanki 2011-04-19 14:11:54 PDT
<rdar://problem/9140052>
Comment 2 Pratik Solanki 2011-04-19 14:18:51 PDT
Created attachment 90252 [details]
Patch
Comment 3 Pratik Solanki 2011-04-19 16:17:58 PDT
Created attachment 90272 [details]
Patch
Comment 4 Simon Fraser (smfr) 2011-04-19 16:22:07 PDT
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())
Comment 5 Pratik Solanki 2011-04-19 17:20:28 PDT
(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.
Comment 6 Pratik Solanki 2011-04-19 17:36:33 PDT
Committed r84321 - <http://trac.webkit.org/changeset/84321>
Comment 7 Simon Fraser (smfr) 2011-04-19 18:13:36 PDT
I hit the assertion on LayoutTests/fast/backgrounds/background-fill-zero-area-crash.html with this change.
Comment 8 Pratik Solanki 2011-04-19 18:17:12 PDT
Oops. I'll take a look at what's happening.
Comment 9 WebKit Review Bot 2011-04-19 18:29:42 PDT
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
Comment 10 Pratik Solanki 2011-04-19 19:09:53 PDT
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?
Comment 11 Pratik Solanki 2011-04-19 22:27:45 PDT
Created attachment 90307 [details]
Fix assertion
Comment 12 Pratik Solanki 2011-04-19 22:29:25 PDT
(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 13 Simon Fraser (smfr) 2011-04-19 22:30:01 PDT
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-> ?
Comment 14 Pratik Solanki 2011-04-19 22:46:44 PDT
Follow on assertion fix committed as r84344 - <http://trac.webkit.org/changeset/84344>
Comment 15 Pratik Solanki 2011-04-20 09:04:37 PDT
(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.
Comment 16 Lei Zhang 2011-04-20 14:52:42 PDT
(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
Comment 17 James Robinson 2011-04-20 16:08:10 PDT
(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
Comment 18 Pratik Solanki 2011-04-20 16:48:16 PDT
(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.
Comment 19 Pratik Solanki 2011-04-20 16:57:24 PDT
Skia and wx are the two ports that don't have a proper implementation for checkForSolidColor(). I'll wrap the assert with proper ifdefs.
Comment 20 Pratik Solanki 2011-04-20 17:12:07 PDT
Created attachment 90449 [details]
Try to fix assertion failure on Chromium
Comment 21 Ryosuke Niwa 2011-04-20 17:15:07 PDT
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.
Comment 22 Pratik Solanki 2011-04-20 22:21:56 PDT
Committed Chromium fix r84468 - <http://trac.webkit.org/changeset/84468>
Comment 23 Tony Gentilcore 2011-04-21 05:44:03 PDT
Created attachment 90514 [details]
Patch
Comment 24 Tony Gentilcore 2011-04-21 05:45:38 PDT
Comment on attachment 90514 [details]
Patch

Clearing flags on attachment: 90514

Committed r84488: <http://trac.webkit.org/changeset/84488>
Comment 25 Tony Gentilcore 2011-04-21 05:45:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 Dirk Pranke 2011-04-21 15:36:11 PDT
*** Bug 59148 has been marked as a duplicate of this bug. ***