Bug 58926 – BitmapImage::destroyMetadataAndNotify should clear m_checkedForSolidColor

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Pratik Solanki]

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.

[Pratik Solanki]

<rdar://problem/9140052>

[Pratik Solanki]

Created attachment 90252 [details]
Patch

[Pratik Solanki]

Created attachment 90272 [details]
Patch

[Simon Fraser (smfr)]

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())

[Pratik Solanki]

(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.

[Pratik Solanki]

Committed r84321 - <http://trac.webkit.org/changeset/84321>

[Simon Fraser (smfr)]

I hit the assertion on LayoutTests/fast/backgrounds/background-fill-zero-area-crash.html with this change.

[Pratik Solanki]

Oops. I'll take a look at what's happening.

[WebKit Review Bot]

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

[Pratik Solanki]

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?

[Pratik Solanki]

Created attachment 90307 [details]
Fix assertion

[Pratik Solanki]

(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.

[Simon Fraser (smfr)]

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-> ?

[Pratik Solanki]

Follow on assertion fix committed as r84344 - <http://trac.webkit.org/changeset/84344>

[Pratik Solanki]

(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.

[Lei Zhang]

(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

[James Robinson]

(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

[Pratik Solanki]

(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.

[Pratik Solanki]

Skia and wx are the two ports that don't have a proper implementation for checkForSolidColor(). I'll wrap the assert with proper ifdefs.

[Pratik Solanki]

Created attachment 90449 [details]
Try to fix assertion failure on Chromium

[Ryosuke Niwa]

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.

[Pratik Solanki]

Committed Chromium fix r84468 - <http://trac.webkit.org/changeset/84468>

[Tony Gentilcore]

Created attachment 90514 [details]
Patch

[Tony Gentilcore]

Comment on attachment 90514 [details]
Patch

Clearing flags on attachment: 90514

Committed r84488: <http://trac.webkit.org/changeset/84488>

[Tony Gentilcore]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

*** Bug 59148 has been marked as a duplicate of this bug. ***