Bug 57052 – Infinite layout loop with two images sharing the same SVG image source

• Component: SVG
• Status: NEW

[Fady Samuel]

I've attached a page that seems to produce an infinite re-layout loop. I accidentally discovered this by adding the following two lines to 
RenderBlock::layout():

static int print_count = 0;
printf("%d: RenderBlock::layout()\n");

[Fady Samuel]

(In reply to comment #0)
> I've attached a page that seems to produce an infinite re-layout loop. I accidentally discovered this by adding the following two lines to 
> RenderBlock::layout():
> 
> static int print_count = 0;
> printf("%d: RenderBlock::layout()\n");


Sorry, that should say:

static int print_count = 0;
printf("%d: RenderBlock::layout()\n", ++print_count);

[Alexey Proskuryakov]

There is no test case attached at the moment, please do attach it.

[Fady Samuel]

Created attachment 87605 [details]
Infinite Relayout Loop reduction

Sorry for the delay. I had a hard time deciding whether this was a platform-specific bug or a general bug, and what exactly was causing it. It seems to be an SVG bug. I will describe the issue in detail in comments.

[Fady Samuel]

(In reply to comment #3)
> Created an attachment (id=87605) [details]
> Infinite Relayout Loop reduction
> 
> Sorry for the delay. I had a hard time deciding whether this was a platform-specific bug or a general bug, and what exactly was causing it. It seems to be an SVG bug. I will describe the issue in detail in comments.

So here's what's going on:

From a high level here, we have two divs (two RenderBoxes) that have the same SVG as a background image. The background layer for each box has a StyleCachedImage proxy object, to a single *SHARED* CachedImage object. This, in turn, has access to an SVGImage object which creates a *fake* page containing an SVGDocument, and SVGSVGElement root element. 

When a RenderBox is in the process of laying out, at some point RenderBoxModelObject::calculateFillTileSize (called from RenderBox::calculateBackgroundImageGeometry) is called. Inside that method, image->setImageContainerSize(..) is called. This ultimately updates m_containerSize in SVGSVGElement (which is shared by the two boxes in this example) to the size of one of the containers. 

When SVGImage::draw is called to paint the background, it resizes the FrameView of the fake page to fit the SVGSVGElement (which is sized according to the other container). This triggers a FrameView::layout(). Ultimately, through FrameView::layout(), we reach ScrollView::repaintContentRectangle(..)which calls it's hostWindow's ChromeClient (which is an SVGImageChromeClient) to "invalidateContentsAndWindow", which then calls CachedImage::changedInRect(..). 

This causes the CachedImage to inform its observers (the two RenderBoxes in this case), that the imageChanged (RenderBox::imageChanged)which eventually calls RenderBox::calculateBackgroundImageGeometry through  RenderBox::repaintLayerRectsForImage. 

...and so we have a huge, super-nasty infinite loop. Please message me if you'd like more information or discuss solutions. I'm eager to think further about this.

[Fady Samuel]

Forgot to add: one RenderBox will see that the SVG hasn't "changed" and so it doesn't do anything. The other will see that the SVG has changed its size to that of the other RenderBox, and so the cycle repeats to reset SVGSVGElement.

(In reply to comment #4)
> (In reply to comment #3)
> > Created an attachment (id=87605) [details] [details]
> > Infinite Relayout Loop reduction
> > 
> > Sorry for the delay. I had a hard time deciding whether this was a platform-specific bug or a general bug, and what exactly was causing it. It seems to be an SVG bug. I will describe the issue in detail in comments.
> 
> So here's what's going on:
> 
> From a high level here, we have two divs (two RenderBoxes) that have the same SVG as a background image. The background layer for each box has a StyleCachedImage proxy object, to a single *SHARED* CachedImage object. This, in turn, has access to an SVGImage object which creates a *fake* page containing an SVGDocument, and SVGSVGElement root element. 
> 
> When a RenderBox is in the process of laying out, at some point RenderBoxModelObject::calculateFillTileSize (called from RenderBox::calculateBackgroundImageGeometry) is called. Inside that method, image->setImageContainerSize(..) is called. This ultimately updates m_containerSize in SVGSVGElement (which is shared by the two boxes in this example) to the size of one of the containers. 
> 
> When SVGImage::draw is called to paint the background, it resizes the FrameView of the fake page to fit the SVGSVGElement (which is sized according to the other container). This triggers a FrameView::layout(). Ultimately, through FrameView::layout(), we reach ScrollView::repaintContentRectangle(..)which calls it's hostWindow's ChromeClient (which is an SVGImageChromeClient) to "invalidateContentsAndWindow", which then calls CachedImage::changedInRect(..). 
> 
> This causes the CachedImage to inform its observers (the two RenderBoxes in this case), that the imageChanged (RenderBox::imageChanged)which eventually calls RenderBox::calculateBackgroundImageGeometry through  RenderBox::repaintLayerRectsForImage. 
> 
> ...and so we have a huge, super-nasty infinite loop. Please message me if you'd like more information or discuss solutions. I'm eager to think further about this.

[Fady Samuel]

Created attachment 88102 [details]
Patch

[Nate Chapin]

Comment on attachment 88102 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88102&action=review

I don't know this code well enough to know for sure that this is the right solution, though it looks fine to me.

Just a couple nits :)

> Source/WebCore/svg/graphics/SVGImage.cpp:-77
> -        if (m_image && m_image->imageObserver())
> -            m_image->imageObserver()->changedInRect(m_image, r);

Can we remove this override of invalidateContentsAndWindow()?

Also, I believe this is the only place changedInRect() is called, so we should probably remove that as well.

[Eric Seidel (no email)]

So what cases will this break?  Do we have repaint tests which show your change?

[Fady Samuel]

Created attachment 88110 [details]
Patch

[Eric Seidel (no email)]

Looking at annotate tells me this code came from bug 22981.

[Simon Fraser (smfr)]

I'd prefer to fix the actual bug rather than regress SVG image invalidation.

[Darin Adler]

Comment on attachment 88110 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88110&action=review

> Source/WebCore/ChangeLog:17
> +        This fix is a short term workaround that gets rid of the infinite loop by
> +        getting rid of the code that informs clients of a change in the SVG.

Can’t we fix it instead?

[Nikolas Zimmermann]

Comment on attachment 88110 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88110&action=review

Darin is right, this is not a good approach, can you investigate on how to actually fix it? Maybe Simon can help.
I'm unfamiliar with the SVGImage code.

> LayoutTests/svg/css/up-arrow.svg:2
> +<!-- Generator: Adobe Illustrator 14.0.0, SVG Export Plug-In . SVG Version: 6.00 Build 43363)  -->

Please remove this line.

> LayoutTests/svg/css/up-arrow.svg:3
> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">

You can also omit this line.

> LayoutTests/svg/css/up-arrow.svg:4
> +<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px" viewBox="10 10 30 30">

id, xmns:xlink, x, y are not needed please remove.

> LayoutTests/svg/css/up-arrow.svg:5
> +<g>

This <g> is superfluous, you can remove it.

[Fady Samuel]

Thanks. I'm looking into a proper fix now. :)

(In reply to comment #13)
> (From update of attachment 88110 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88110&action=review
> 
> Darin is right, this is not a good approach, can you investigate on how to actually fix it? Maybe Simon can help.
> I'm unfamiliar with the SVGImage code.
> 
> > LayoutTests/svg/css/up-arrow.svg:2
> > +<!-- Generator: Adobe Illustrator 14.0.0, SVG Export Plug-In . SVG Version: 6.00 Build 43363)  -->
> 
> Please remove this line.
> 
> > LayoutTests/svg/css/up-arrow.svg:3
> > +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
> 
> You can also omit this line.
> 
> > LayoutTests/svg/css/up-arrow.svg:4
> > +<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px" viewBox="10 10 30 30">
> 
> id, xmns:xlink, x, y are not needed please remove.
> 
> > LayoutTests/svg/css/up-arrow.svg:5
> > +<g>
> 
> This <g> is superfluous, you can remove it.

[Nikolas Zimmermann]

I ran into the same issue while developing the CSS 2.1 background-intrinsic-* support.
A workaround is to disable caching for SVGImages. I'm looking into a proper fix as well.