Bug 19964 – divide by zero crash in RenderBox::calculateBackgroundSize with 0,0 bmp background image

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Alexander Mohr]

there's a divide by zero crash in RenderBox::calculateBackgroundSize when you load a 0,0 bmp as the background image and specify -khtml-background-size

It crashes on:
w = bg->imageSize(this, style()->effectiveZoom()).width() * h / bg->imageSize(this, style()->effectiveZoom()).height();

since bg->imageSize returns 0,0

[Alexander Mohr]

Created attachment 22187 [details]
sample 0,0 bitmap

[Alexander Mohr]

Created attachment 22188 [details]
page that links to 0,0 bitmap bugfile that causes crash

[Chris Brichford]

I suspect change set 31981 introduced this bug when it removed the following statements from RenderBox::imageChanged:

if (!image || !image->canRender(style()->effectiveZoom()) || !parent() || !view()) 
        return;

canRender will return false if the image is zero width or zero height.  If we take the early out in RenderBox::imageChanged we never call calculateBackgroundSize, which assumes that any background images have non-zero height.

[Chris Brichford]

Created attachment 22479 [details]
Proposed fix

Proposed fix, including LayoutTest.

[Chris Brichford]

My fix is busted.  Breaks rendering of broken images.

[Mihnea Ovidenie]

Created attachment 22681 [details]
New patch for bug 19964

[Darin Adler]

Comment on attachment 22681 [details]
New patch for bug 19964

+        * ChangeLog:

The ChangeLog itself shouldn't be listed in the ChangeLog.

We need a ChangeLog for the LayoutTests directory too, not just WebCore.

Otherwise this patch looks good. I'm going to say review+ but having no patch for LayoutTests does add work for whoever commits the patch.

[Mihnea Ovidenie]

Hello,
Can you please explain me what did you mean by having no patch for LayoutTests? The patch i submitted included a test for the problem itself: "LayoutTests/css3/khtml-background-size-0x0-bmp.html" and the needed bitmap for the test.

If more tests are needed for the patch to land in the trunk, please tell me and i will be glad to do it.

Regards,
Mihnea Ovidenie


> (From update of attachment 22681 [details] [edit])
> +        * ChangeLog:
> 
> The ChangeLog itself shouldn't be listed in the ChangeLog.
> 
> We need a ChangeLog for the LayoutTests directory too, not just WebCore.
> 
> Otherwise this patch looks good. I'm going to say review+ but having no patch
> for LayoutTests does add work for whoever commits the patch.
> 

[Mark Rowe (bdash)]

Landed in r36047.

Mihnea, what Darin meant is that you did not include a ChangeLog entry for your changes inside LayoutTests.  I added one when I landed the change, but in the future you should include one with your patches.