REGRESSION(99539): Infinite repaint loop with SVGImage and deferred repaint timers

• Component: SVG
• Status: RESOLVED FIXED

[Tim Horton]

<rdar://problem/10651634>

Turning on deferred repaint timers exposes an issue in SVGImage/SVGImageChromeClient which gets WebKit into a state of infinitely repainting.

Steps to Reproduce:

0. Turn on layer borders, to make the repainting clear.
1. Switch on deferred repaint timers in JavaScriptCore/wtf/Platform.h (#define ENABLE_REPAINT_THROTTLING 1)
2. Open the attached (extremely simple) test case.

The result is that after the deferred repaint timer fires, SVGImageChromeClient ends up invalidating the SVGImageCache, and we repaint the image, causing another deferred repaint timer to be started, and so on.

Niko, my question for you: why do we invalidate the image in this situation? Where do you think it's best to break the loop? Antti is of the belief that we should break it somewhere between SVGImageChromeClient and SVGImageCache, but neither of us are sure where to do so!

Here's a partial backtrace of the interesting bit:

#0  WebCore::SVGImageCache::imageContentChanged (this=0x7f9f8f00bf20) at /Volumes/SSD/src/WebKit/OpenSource/Source/WebCore/svg/graphics/SVGImageCache.cpp:80
#1  0x000000010e680326 in WebCore::CachedImage::changedInRect (this=0x7f9f8b820800, image=0x7f9f8f00d3e0, rect=@0x7fff6c4c7448) at /Volumes/SSD/src/WebKit/OpenSource/Source/WebCore/loader/cache/CachedImage.cpp:462
#2  0x000000010e680377 in non-virtual thunk to WebCore::CachedImage::changedInRect(WebCore::Image const*, WebCore::IntRect const&) () at /Volumes/SSD/src/WebKit/OpenSource/Source/WebCore/loader/cache/CachedImage.cpp:467
#3  0x000000010f953cff in WebCore::SVGImageChromeClient::invalidateContentsAndRootView (this=0x7f9f8f00d530, r=@0x7fff6c4c7448) at /Volumes/SSD/src/WebKit/OpenSource/Source/WebCore/svg/graphics/SVGImage.cpp:80
#4  0x000000010e6b9a25 in WebCore::Chrome::invalidateContentsAndRootView (this=0x7f9f8f00c030, updateRect=@0x7fff6c4c7448, immediate=false) at /Volumes/SSD/src/WebKit/OpenSource/Source/WebCore/page/Chrome.cpp:86
#5  0x000000010f814935 in WebCore::ScrollView::repaintContentRectangle (this=0x7f9f8f010ce0, rect=@0x7f9f910010b0, now=false) at /Volumes/SSD/src/WebKit/OpenSource/Source/WebCore/platform/ScrollView.cpp:959
#6  0x000000010ebd7227 in WebCore::FrameView::doDeferredRepaints (this=0x7f9f8f010ce0) at /Volumes/SSD/src/WebKit/OpenSource/Source/WebCore/page/FrameView.cpp:1907
#7  0x000000010ebcf2b9 in WebCore::FrameView::deferredRepaintTimerFired (this=0x7f9f8f010ce0) at /Volumes/SSD/src/WebKit/OpenSource/Source/WebCore/page/FrameView.cpp:1950

[Tim Horton]

Created attachment 126433 [details]
repro

[Nikolas Zimmermann]

(In reply to comment #0)
> The result is that after the deferred repaint timer fires, SVGImageChromeClient ends up invalidating the SVGImageCache, and we repaint the image, causing another deferred repaint timer to be started, and so on.
Ouch this is evil. When the SVG embedded document repaint timer fires, we should trigger a repaint of the host document. This is not the problematic part, right? It all happens in the SVG documents FrameView itself? (
 
> Niko, my question for you: why do we invalidate the image in this situation? Where do you think it's best to break the loop? Antti is of the belief that we should break it somewhere between SVGImageChromeClient and SVGImageCache, but neither of us are sure where to do so!

Looking at:

void SVGImageCache::imageContentChanged()
{
    ImageDataMap::iterator end = m_imageDataMap.end();
    for (ImageDataMap::iterator it = m_imageDataMap.begin(); it != end; ++it)
        it->second.imageNeedsUpdate = true;

    // Start redrawing dirty images with a timer, as imageContentChanged() may be called
    // by the FrameView of the SVGImage which is currently in FrameView::layout().
    if (!m_redrawTimer.isActive())
        m_redrawTimer.startOneShot(0);

the redraw timer was invented, to avoid redrawing from within layout - here FrameView::layout() is not the root of the stack trace (at least it seems so, correct me if I'm wrong, but the deferred repaint timer should always fire after FrameView::layout() finished).

Hm, as quick test within an ENABLE_REPAINT_THROTTLING enabled build, could you try to just remove the m_redrawTimer, and copy the content of redrawTimerFired into imageContentChanged -- does that fix the bug?

[Tim Horton]

(In reply to comment #2)
> (In reply to comment #0)
> > The result is that after the deferred repaint timer fires, SVGImageChromeClient ends up invalidating the SVGImageCache, and we repaint the image, causing another deferred repaint timer to be started, and so on.
> Ouch this is evil. When the SVG embedded document repaint timer fires, we should trigger a repaint of the host document. This is not the problematic part, right? It all happens in the SVG documents FrameView itself? (
> 
> > Niko, my question for you: why do we invalidate the image in this situation? Where do you think it's best to break the loop? Antti is of the belief that we should break it somewhere between SVGImageChromeClient and SVGImageCache, but neither of us are sure where to do so!
> 
> Looking at:
> 
> void SVGImageCache::imageContentChanged()
> {
>     ImageDataMap::iterator end = m_imageDataMap.end();
>     for (ImageDataMap::iterator it = m_imageDataMap.begin(); it != end; ++it)
>         it->second.imageNeedsUpdate = true;
> 
>     // Start redrawing dirty images with a timer, as imageContentChanged() may be called
>     // by the FrameView of the SVGImage which is currently in FrameView::layout().
>     if (!m_redrawTimer.isActive())
>         m_redrawTimer.startOneShot(0);
> 
> the redraw timer was invented, to avoid redrawing from within layout - here FrameView::layout() is not the root of the stack trace (at least it seems so, correct me if I'm wrong, but the deferred repaint timer should always fire after FrameView::layout() finished).
> 
> Hm, as quick test within an ENABLE_REPAINT_THROTTLING enabled build, could you try to just remove the m_redrawTimer, and copy the content of redrawTimerFired into imageContentChanged -- does that fix the bug?

*Excellent*! That does it... so, is there a better way to prevent repainting from inside of layout?

[Nikolas Zimmermann]

(In reply to comment #3)
> *Excellent*! That does it... so, is there a better way to prevent repainting from inside of layout?
Phew! Glad I still knew what's going on there :-) Anyhow, my assumption that this is only ever called from within FrameView::layout, doesn't hold - so that needs to be fixed. Maybe as simple as checking for frameView->needsLayout() - if it returns false, don't start our timer, but redraw immediately.

Does that work?

[Tim Horton]

Created attachment 127291 [details]
experimental patch (for ews)

This fixes it for me, but I'm not sure it's appropriate (I see no obvious way to get to the frameview without plumbing it through in an ugly way, so I went with redraw-immediately-if-we're-painting, using the painting check from CachedImage). Uploading here to check chromium EWS and see what it says.

[Nikolas Zimmermann]

(In reply to comment #5)
> Created an attachment (id=127291) [details]
> experimental patch (for ews)
> 
> This fixes it for me, but I'm not sure it's appropriate (I see no obvious way to get to the frameview without plumbing it through in an ugly way, so I went with redraw-immediately-if-we're-painting, using the painting check from CachedImage). Uploading here to check chromium EWS and see what it says.

I'm not sure if the patch is perfect as well never used the currentPaintTimeStamp logic so far, anyhow it might be easier to just grab a FrameView. SVGImageCache has acccess to the SVGImage. You could add a FrameView* accessor to SVGImage and use it from SVGImageCache, no?
The SVGImage has the Page, which has the Frame & FrameView.

[Tim Horton]

Created attachment 127403 [details]
patch

[Nikolas Zimmermann]

Comment on attachment 127403 [details]
patch

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

r=me, but please add null-checks first.

> Source/WebCore/svg/graphics/SVGImage.cpp:260
> +    return m_page->mainFrame()->view();

You should check for a null m_page,, as hasRelativeWidth etc. does.

> Source/WebCore/svg/graphics/SVGImage.h:57
> +    FrameView* frameView();

Should be const, no?

> Source/WebCore/svg/graphics/SVGImageCache.cpp:87
> +    if (m_svgImage->frameView()->needsLayout() && !m_redrawTimer.isActive())

This needs a null-check as well, m_page could be null, if not, you'l need assertions.

[WebKit Review Bot]

Comment on attachment 127403 [details]
patch

Attachment 127403 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11539200

New failing tests:
svg/as-image/animated-svg-as-image-no-fixed-intrinsic-size.html

[Tim Horton]

Created attachment 127484 [details]
revised patch

The logic was slightly wrong; before, with the !timer.isActive() inside the same condition as needsLayout(), if the timer was active and we were in layout, we would (bad!) perform an immediate repaint. Uploading in hopes that the Chromium EWS is happier with this, but this does fix it locally.

[Tim Horton]

Landed in http://trac.webkit.org/changeset/108834

[Nikolas Zimmermann]

Tim, this broke Chromium/Mac 10.6, according to ennes observation in bug 79707.
Can you revisit this?

[Tim Horton]

(In reply to comment #12)
> Tim, this broke Chromium/Mac 10.6, according to ennes observation in bug 79707.
> Can you revisit this?

Kinda bizarre that it would only break that platform! I'll take a look, but it's not a configuration I have readily available. Do you have any thoughts?

[Tim Horton]

(In reply to comment #13)
> (In reply to comment #12)
> > Tim, this broke Chromium/Mac 10.6, according to ennes observation in bug 79707.
> > Can you revisit this?
> 
> Kinda bizarre that it would only break that platform! I'll take a look, but it's not a configuration I have readily available. Do you have any thoughts?

I can reproduce on Mac (though only inside DRT).

This isn't going to work as-is (obviously, since it got rolled out) because we end up modifying m_repaintRects while inside the loop in doDeferredRepaints, because drawSVGToImageBuffer ends up doing layout...

1   0x10936f104 WebCore::FrameView::repaintContentRectangle(WebCore::IntRect const&, bool)
2   0x109f89309 WebCore::RenderView::repaintViewRectangle(WebCore::IntRect const&, bool)
3   0x109e93ba1 WebCore::RenderObject::repaintUsingContainer(WebCore::RenderBoxModelObject*, WebCore::IntRect const&, bool)
4   0x109e93d2d WebCore::RenderObject::repaint(bool)
5   0x10936b4fd WebCore::FrameView::layout(bool)
6   0x10a19e7f6 WebCore::SVGImage::draw(WebCore::GraphicsContext*, WebCore::FloatRect const&, WebCore::FloatRect const&, WebCore::ColorSpace, WebCore::CompositeOperator)
7   0x10a19e424 WebCore::SVGImage::drawSVGToImageBuffer(WebCore::ImageBuffer*, WebCore::IntSize const&, float, WebCore::SVGImage::ShouldClearBuffer)
8   0x10a1a431a WebCore::SVGImageCache::redraw()
9   0x10a1a4191 WebCore::SVGImageCache::imageContentChanged()
10  0x108dcdc56 WebCore::CachedImage::changedInRect(WebCore::Image const*, WebCore::IntRect const&)
11  0x108dcdca7 non-virtual thunk to WebCore::CachedImage::changedInRect(WebCore::Image const*, WebCore::IntRect const&)
12  0x10a1a1aff WebCore::SVGImageChromeClient::invalidateContentsAndRootView(WebCore::IntRect const&, bool)
13  0x108e150f5 WebCore::Chrome::invalidateContentsAndRootView(WebCore::IntRect const&, bool)
14  0x10a045055 WebCore::ScrollView::repaintContentRectangle(WebCore::IntRect const&, bool)
15  0x10936f99a WebCore::FrameView::doDeferredRepaints()
16  0x109367949 WebCore::FrameView::deferredRepaintTimerFired(WebCore::Timer<WebCore::FrameView>*)

[Tim Horton]

> This isn't going to work as-is (obviously, since it got rolled out) because we end up modifying m_repaintRects while inside the loop in doDeferredRepaints, because drawSVGToImageBuffer ends up doing layout...

The layout occurs because of the resize:

SVGImage::drawSVGToImageBuffer    frame->view()->resize(this->size());

From other fixes to similar problems in this area in the past, I assume the correct solution is to ensure that the layout (and the resize) happen before we're painting at all.

[Nikolas Zimmermann]

(In reply to comment #15)
> SVGImage::drawSVGToImageBuffer    frame->view()->resize(this->size());
> 
> From other fixes to similar problems in this area in the past, I assume the correct solution is to ensure that the layout (and the resize) happen before we're painting at all.
drawSVGToImageBuffer layouts the SVG document, and paints it while the host document is painting. This is potentially okay, when done right. Currently drawSVGToImageBuffer overrides the imageObserver to zero, so that any frame view resize calls etc. that follow, don't cause calls to SVGImageChromeClient::invalidateContentsAndRootView(), as that calls back to the host document, which is currently painting.

But I've realized this doensn't prevent the SVG documents FrameView to schedule relayouts/repaints, and that's the problem - we don't want that at all.

Currently drawSVGToImageBuffer works like this:
- Override image observer, so that no one notices what we're doing with the SVG document (in theory)
- Resize to desired target size and zoom factor (this information comes from the SVGImageCache) - utilizing SVGImage::draw(), which calls frameView->layout().
- Draw SVG root object to ImageBuffer (SVGImage::draw, calling frameView->paint()).
- Reset zoom factor and size to initial size
- Restore image observer.

Basically we need to be able to snapshot the document at a specific size & zoom level, but then restore the original document as-is. We can't leave the cached document mutated in any way. Consider two SVG documents which both embed foo.svg - if document 'A' wants to draw the SVG Image into a 300x300 target <div> element as background-image, it would resize the SVG document to 300x300, paint that. Now what if document 'B' already embedded the embedded SVG before with another size? It would now get redrawn as well using another size.....

When animations are running its even harder to get this right, if resources are shared.

To summarize: I think the drawSVGToImageBuffer approach is right in general, but still has some bugs. 
You coudl try to call frame->view()->beginDefferedRepaints() after the setImageObserver(0) call, end endDeferredRepaints() after the restoring of it, and see if that affects anything.

[Tim Horton]

(In reply to comment #16)
> To summarize: I think the drawSVGToImageBuffer approach is right in general, but still has some bugs. 
> You coudl try to call frame->view()->beginDefferedRepaints() after the setImageObserver(0) call, end endDeferredRepaints() after the restoring of it, and see if that affects anything.

That will just result in queueing all the repaints-we-don't-want up and firing them at the end :-D (experimental results agree)

[Tim Horton]

(In reply to comment #17)
> (In reply to comment #16)
> > To summarize: I think the drawSVGToImageBuffer approach is right in general, but still has some bugs. 
> > You coudl try to call frame->view()->beginDefferedRepaints() after the setImageObserver(0) call, end endDeferredRepaints() after the restoring of it, and see if that affects anything.
> 
> That will just result in queueing all the repaints-we-don't-want up and firing them at the end :-D (experimental results agree)

Whereas adding a way to explicitly prevent FrameView from firing any repaints works fine. Not sure that's actually a reasonable thing to do, though.

[Nikolas Zimmermann]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > To summarize: I think the drawSVGToImageBuffer approach is right in general, but still has some bugs. 
> > > You coudl try to call frame->view()->beginDefferedRepaints() after the setImageObserver(0) call, end endDeferredRepaints() after the restoring of it, and see if that affects anything.
> > 
> > That will just result in queueing all the repaints-we-don't-want up and firing them at the end :-D (experimental results agree)
> 
> Whereas adding a way to explicitly prevent FrameView from firing any repaints works fine. Not sure that's actually a reasonable thing to do, though.
Ah yeah, that's true - just deffering won't help, we need to clear these repaints.
I think its reasonable, as SVGImage is really special. We want to force layout, paint, and reset layout to the old state w/o any side effects (aka. triggering repainting or anything in the host document)...

[Tim Horton]

Created attachment 130740 [details]
revised patch, cancelling all painting during layout inside drawSVGToImageBuffer

[Nikolas Zimmermann]

Comment on attachment 130740 [details]
revised patch, cancelling all painting during layout inside drawSVGToImageBuffer

Looks great to me!

[Tim Horton]

Re-landed in http://trac.webkit.org/changeset/110309

[Tim Horton]

Re-rolled out.

[Tim Horton]

(In reply to comment #23)
> Re-rolled out.

I can't reproduce any crashes or unexpected failures at all related to this patch with a cr-mac-lion or mac-lion build...

[Adam Barth]

The crashes went away when we rolled out the patch.

[Tim Horton]

(In reply to comment #25)
> The crashes went away when we rolled out the patch.

Indeed; I'll find a SL machine, I guess. It would be really nice if the bot crash logs worked :-\

[Tim Horton]

(In reply to comment #26)
> (In reply to comment #25)
> > The crashes went away when we rolled out the patch.
> 
> Indeed; I'll find a SL machine, I guess. It would be really nice if the bot crash logs worked :-\

Or not, I can make it reproduce by turning on deferred repaints. They must only be on on SL by default? (I was never able to find out where/why you guys were turning them on last time I looked). Will investigate, anyway.

[Tim Horton]

Created attachment 131367 [details]
patch (third time's a charm); this time, prevent deferred timers from even being started

This patch passes on mac-lion and cr-mac-lion (in both cases, with deferred repaint timers forced on). That said, before this fix I was easily able to make the failures flaky by running a different subset of tests, since they're timing related.

[Nikolas Zimmermann]

Comment on attachment 131367 [details]
patch (third time's a charm); this time, prevent deferred timers from even being started

Looks great to me.

[Nikolas Zimmermann]

Comment on attachment 131367 [details]
patch (third time's a charm); this time, prevent deferred timers from even being started

Looks great to me.

[Tim Horton]

Landed as http://trac.webkit.org/changeset/110469

[Hajime Morrita]

This crashes on cr-mac (again?)...

----
Regressions: Unexpected DumpRenderTree crashes : (5)
  svg/as-background-image/svg-as-background-6.html = CRASH
  svg/as-image/svg-nested.html = CRASH
  svg/custom/fill-opacity-rgba.svg = CRASH
  tables/mozilla_expected_failures/marvin/table_overflow_td_align_right.html = CRASH
  transitions/interrupt-transform-transition.html = CRASH
----
http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.6%20%28dbg%29/builds/8809
The range under suspicion is from r110481 to r110466, which includes this one.
And the crash list is highly related svg images...
And unfortunately the log didn't leave any stack traces. 

WildFox@ told me that this is tested against even cr-mac.
So I'm hesitant to revert this blindly. But I don't want to leave this as is.

[Hajime Morrita]

Got some stacktraces https://gist.github.com/2028708

[Tim Horton]

(In reply to comment #33)
> Got some stacktraces https://gist.github.com/2028708

Some look related; I don't understand why the assertion failures are so intermittent. I'll take a look right away.

[Nikolas Zimmermann]

(In reply to comment #34)
> Some look related; I don't understand why the assertion failures are so intermittent. I'll take a look right away.
The rest are mine from bug 12437.

[Tim Horton]

(In reply to comment #34)
> (In reply to comment #33)
> > Got some stacktraces https://gist.github.com/2028708
> 
> Some look related; I don't understand why the assertion failures are so intermittent. I'll take a look right away.

I can't make this reproduce at all on cr-mac-lion with deferred repaint timers forced on; you guys can roll it out if it's still failing, but I'm at a loss for how to fix it this time.

[Simon Fraser (smfr)]

I'd rather we avoid rolling this out again. Tim has done due diligence in trying to debug this in cr-mac, and I think it's time to get some help from Chromium people.

[Tim Horton]

(In reply to comment #37)
> I'd rather we avoid rolling this out again. Tim has done due diligence in trying to debug this in cr-mac, and I think it's time to get some help from Chromium people.

Or Niko, since r99539 was his (though he's been a great help all along!).

[Hajime Morrita]

Reopening because this one was reverted at r110581.
I'm investigating.

[Hajime Morrita]

Created attachment 132567 [details]
Patch

[Hajime Morrita]

Comment on attachment 131367 [details]
patch (third time's a charm); this time, prevent deferred timers from even being started

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

Hi Zimmerman, could you rubberstamp? I'd like to see if my fix works.
It works on my local mac-chromium.

> Source/WebCore/page/FrameView.cpp:-1853
> -        return;

Found that this deletion caused the assertion failure.
I recovered this line on the updated patch.

[Nikolas Zimmermann]

(In reply to comment #41)
> (From update of attachment 131367 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131367&action=review
> 
> Hi Zimmerman, could you rubberstamp? I'd like to see if my fix works.
> It works on my local mac-chromium.
> 
> > Source/WebCore/page/FrameView.cpp:-1853
> > -        return;
> 
> Found that this deletion caused the assertion failure.
> I recovered this line on the updated patch.


Hey Hajime,

sure, let's give it a try, rs=me.

[Hajime Morrita]

Committed r111480: <http://trac.webkit.org/changeset/111480>

[Hajime Morrita]

Comment on attachment 132567 [details]
Patch

Landed at r111480. Closing.

[Tim Horton]

(In reply to comment #44)
> (From update of attachment 132567 [details])
> Landed at r111480. Closing.

Yay! Thanks for your help, Hajime!

[Stephen Chenney]

Reopening to attach new patch.

[Stephen Chenney]

Created attachment 135825 [details]
Patch

This problem has been a nightmare. The fix as currently in trunk causes crashes on MacOS for Chromium tests, and probably also causes flaky results on Release builds. This patch addresses the remaining problems, and after tens of thousands of test runs on a MacOS 10.6 machine there are no signs of crashes. The previous crash rate was about 1 in 20. There may still be issues with the repaint timer firing in the _next_ layout test, but I'll deal with that separately.

[Stephen Chenney]

I can also shed some light on why this is flaky. That is, why the crashes only happen rarely (although I have no idea why only on Mac platforms).

DRT execution traces looking at layout calls and requests for repaint show that non-crash runs execute very linearly, in the sense that there is an initial layout, a resize or two, another layout and we're done. The runs that crash, for some reason, have a layout timer that fires and causes SVG layout to fire which makes many many more calls requesting repaint of various rects. This in turn gets the repaint timer into the mix and the interactions between the timers causes problems.

I'm guessing that the underlying issue is how fast some content comes off disk, or how the OS timers behave, or how loaded the machine is, or ...

In some cases we are also seeing the SVGImageCache repaint timer fire at the very beginning of a test. We find this odd and need to investigate further. It does, however, explain why this crash can be hit even when there is no SVG content in the test case.

[Dimitri Glazkov (Google)]

Comment on attachment 135825 [details]
Patch

The whole thing of even needing to fire a timer to redraw sounds bad, but this doesn't make it worse.

[Stephen Chenney]

Comment on attachment 135825 [details]
Patch

Clearing flags on attachment: 135825

Committed r113323: <http://trac.webkit.org/changeset/113323>

[Stephen Chenney]

All reviewed patches have been landed.  Closing bug.

[Nikolas Zimmermann]

(In reply to comment #49)
> (From update of attachment 135825 [details])
> The whole thing of even needing to fire a timer to redraw sounds bad, but this doesn't make it worse.
Your intuition is correct, it's indeed weird on first sight, but there were good arguments for it, see the bug which initially introduced SVGImageCache.

Stephen, thanks so much for tackling this!