Bug 78315 - REGRESSION(99539): Infinite repaint loop with SVGImage and deferred repaint timers
Summary: REGRESSION(99539): Infinite repaint loop with SVGImage and deferred repaint t...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords: InRadar
Depends on: 79707 79840 80732 81010 82052
Blocks: 82232
  Show dependency treegraph
 
Reported: 2012-02-09 19:11 PST by Tim Horton
Modified: 2012-04-06 23:36 PDT (History)
10 users (show)

See Also:


Attachments
repro (1.13 KB, application/zip)
2012-02-09 19:12 PST, Tim Horton
no flags Details
experimental patch (for ews) (2.02 KB, patch)
2012-02-15 18:41 PST, Tim Horton
no flags Details | Formatted Diff | Diff
patch (4.68 KB, patch)
2012-02-16 10:24 PST, Tim Horton
zimmermann: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
revised patch (4.84 KB, patch)
2012-02-16 17:49 PST, Tim Horton
dino: review+
Details | Formatted Diff | Diff
revised patch, cancelling all painting during layout inside drawSVGToImageBuffer (8.79 KB, patch)
2012-03-07 17:57 PST, Tim Horton
zimmermann: review+
Details | Formatted Diff | Diff
patch (third time's a charm); this time, prevent deferred timers from even being started (11.23 KB, patch)
2012-03-12 11:45 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (11.78 KB, patch)
2012-03-19 03:11 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (2.73 KB, patch)
2012-04-05 08:05 PDT, Stephen Chenney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2012-02-09 19:11:47 PST
<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
Comment 1 Tim Horton 2012-02-09 19:12:28 PST
Created attachment 126433 [details]
repro
Comment 2 Nikolas Zimmermann 2012-02-15 01:58:31 PST
(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?
Comment 3 Tim Horton 2012-02-15 14:27:39 PST
(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?
Comment 4 Nikolas Zimmermann 2012-02-15 14:52:06 PST
(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?
Comment 5 Tim Horton 2012-02-15 18:41:53 PST
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.
Comment 6 Nikolas Zimmermann 2012-02-16 03:56:54 PST
(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.
Comment 7 Tim Horton 2012-02-16 10:24:58 PST
Created attachment 127403 [details]
patch
Comment 8 Nikolas Zimmermann 2012-02-16 11:20:18 PST
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.
Comment 9 WebKit Review Bot 2012-02-16 12:40:21 PST
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
Comment 10 Tim Horton 2012-02-16 17:49:18 PST
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.
Comment 11 Tim Horton 2012-02-24 11:41:56 PST
Landed in http://trac.webkit.org/changeset/108834
Comment 12 Nikolas Zimmermann 2012-02-28 00:37:15 PST
Tim, this broke Chromium/Mac 10.6, according to ennes observation in bug 79707.
Can you revisit this?
Comment 13 Tim Horton 2012-02-28 00:39:22 PST
(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?
Comment 14 Tim Horton 2012-03-05 15:15:20 PST
(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>*)
Comment 15 Tim Horton 2012-03-05 15:38:48 PST
> 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.
Comment 16 Nikolas Zimmermann 2012-03-06 06:29:30 PST
(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.
Comment 17 Tim Horton 2012-03-06 12:55:40 PST
(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)
Comment 18 Tim Horton 2012-03-06 13:17:08 PST
(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.
Comment 19 Nikolas Zimmermann 2012-03-07 00:48:42 PST
(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)...
Comment 20 Tim Horton 2012-03-07 17:57:13 PST
Created attachment 130740 [details]
revised patch, cancelling all painting during layout inside drawSVGToImageBuffer
Comment 21 Nikolas Zimmermann 2012-03-09 02:37:38 PST
Comment on attachment 130740 [details]
revised patch, cancelling all painting during layout inside drawSVGToImageBuffer

Looks great to me!
Comment 22 Tim Horton 2012-03-09 11:22:27 PST
Re-landed in http://trac.webkit.org/changeset/110309
Comment 23 Tim Horton 2012-03-09 15:40:24 PST
Re-rolled out.
Comment 24 Tim Horton 2012-03-10 22:48:14 PST
(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...
Comment 25 Adam Barth 2012-03-10 22:58:55 PST
The crashes went away when we rolled out the patch.
Comment 26 Tim Horton 2012-03-10 23:00:05 PST
(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 :-\
Comment 27 Tim Horton 2012-03-10 23:05:44 PST
(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.
Comment 28 Tim Horton 2012-03-12 11:45:51 PDT
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.
Comment 29 Nikolas Zimmermann 2012-03-12 13:09:01 PDT
Comment on attachment 131367 [details]
patch (third time's a charm); this time, prevent deferred timers from even being started

Looks great to me.
Comment 30 Nikolas Zimmermann 2012-03-12 13:09:35 PDT
Comment on attachment 131367 [details]
patch (third time's a charm); this time, prevent deferred timers from even being started

Looks great to me.
Comment 31 Tim Horton 2012-03-12 13:27:20 PDT
Landed as http://trac.webkit.org/changeset/110469
Comment 32 Hajime Morrita 2012-03-13 01:29:51 PDT
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.
Comment 33 Hajime Morrita 2012-03-13 06:50:17 PDT
Got some stacktraces https://gist.github.com/2028708
Comment 34 Tim Horton 2012-03-13 07:30:30 PDT
(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.
Comment 35 Nikolas Zimmermann 2012-03-13 07:57:36 PDT
(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.
Comment 36 Tim Horton 2012-03-13 08:55:34 PDT
(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.
Comment 37 Simon Fraser (smfr) 2012-03-13 09:06:56 PDT
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.
Comment 38 Tim Horton 2012-03-13 09:47:31 PDT
(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!).
Comment 39 Hajime Morrita 2012-03-14 20:44:06 PDT
Reopening because this one was reverted at r110581.
I'm investigating.
Comment 40 Hajime Morrita 2012-03-19 03:11:19 PDT
Created attachment 132567 [details]
Patch
Comment 41 Hajime Morrita 2012-03-19 03:13:11 PDT
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.
Comment 42 Nikolas Zimmermann 2012-03-19 07:24:49 PDT
(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.
Comment 43 Hajime Morrita 2012-03-20 18:20:08 PDT
Committed r111480: <http://trac.webkit.org/changeset/111480>
Comment 44 Hajime Morrita 2012-03-21 22:19:56 PDT
Comment on attachment 132567 [details]
Patch

Landed at r111480. Closing.
Comment 45 Tim Horton 2012-03-21 22:23:48 PDT
(In reply to comment #44)
> (From update of attachment 132567 [details])
> Landed at r111480. Closing.

Yay! Thanks for your help, Hajime!
Comment 46 Stephen Chenney 2012-04-05 08:05:55 PDT
Reopening to attach new patch.
Comment 47 Stephen Chenney 2012-04-05 08:05:59 PDT
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.
Comment 48 Stephen Chenney 2012-04-05 08:25:32 PDT
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.
Comment 49 Dimitri Glazkov (Google) 2012-04-05 08:44:05 PDT
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.
Comment 50 Stephen Chenney 2012-04-05 08:51:29 PDT
Comment on attachment 135825 [details]
Patch

Clearing flags on attachment: 135825

Committed r113323: <http://trac.webkit.org/changeset/113323>
Comment 51 Stephen Chenney 2012-04-05 08:51:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 52 Nikolas Zimmermann 2012-04-06 23:36:37 PDT
(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!