Bug 57052 - Infinite layout loop with two images sharing the same SVG image source
Summary: Infinite layout loop with two images sharing the same SVG image source
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-24 14:06 PDT by Fady Samuel
Modified: 2011-07-05 02:00 PDT (History)
9 users (show)

See Also:


Attachments
Infinite Relayout Loop reduction (918 bytes, application/x-compressed-tar)
2011-03-30 13:34 PDT, Fady Samuel
no flags Details
Patch (11.97 KB, patch)
2011-04-04 12:44 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (13.77 KB, patch)
2011-04-04 13:15 PDT, Fady Samuel
zimmermann: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fady Samuel 2011-03-24 14:06:48 PDT
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");
Comment 1 Fady Samuel 2011-03-24 14:08:10 PDT
(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);
Comment 2 Alexey Proskuryakov 2011-03-24 17:40:58 PDT
There is no test case attached at the moment, please do attach it.
Comment 3 Fady Samuel 2011-03-30 13:34:27 PDT
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.
Comment 4 Fady Samuel 2011-03-30 13:54:28 PDT
(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.
Comment 5 Fady Samuel 2011-03-30 14:03:01 PDT
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.
Comment 6 Fady Samuel 2011-04-04 12:44:15 PDT
Created attachment 88102 [details]
Patch
Comment 7 Nate Chapin 2011-04-04 12:49:17 PDT
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.
Comment 8 Eric Seidel (no email) 2011-04-04 13:03:35 PDT
So what cases will this break?  Do we have repaint tests which show your change?
Comment 9 Fady Samuel 2011-04-04 13:15:09 PDT
Created attachment 88110 [details]
Patch
Comment 10 Eric Seidel (no email) 2011-04-04 13:28:44 PDT
Looking at annotate tells me this code came from bug 22981.
Comment 11 Simon Fraser (smfr) 2011-04-04 13:35:39 PDT
I'd prefer to fix the actual bug rather than regress SVG image invalidation.
Comment 12 Darin Adler 2011-04-04 17:08:49 PDT
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?
Comment 13 Nikolas Zimmermann 2011-04-05 05:52:34 PDT
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.
Comment 14 Fady Samuel 2011-04-05 08:46:20 PDT
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.
Comment 15 Nikolas Zimmermann 2011-07-05 02:00:15 PDT
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.