Bug 33230 – svg/W3C-SVG-1.1/animate-elem-30-t.svg crashing occasionally on Leopard Build Bot

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Eric Seidel (no email)]

svg/W3C-SVG-1.1/coords-coord-01-t.svg crashing occasionally on Leopard Build Bot

http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r52820%20(8894)/
Tests that caused the DumpRenderTree tool to crash:
svg/W3C-SVG-1.1/coords-coord-01-t.svg

http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r52818%20(8892)/
Tests that caused the DumpRenderTree tool to crash:
svg/W3C-SVG-1.1/animate-elem-30-t.svg

ASSERTION FAILED: !m_frame->view() || !m_frame->view()->needsLayout()
(/Volumes/Big/WebKit-BuildSlave/leopard-intel-debug/build/WebCore/editing/SelectionController.cpp:929 bool WebCore::SelectionController::recomputeCaretRect())

Both times:
Tests where results did not match expected results:
svg/W3C-SVG-1.1/filters-conv-01-f.svg

I wonder if this could be caused/revealed by http://trac.webkit.org/changeset/52778?

[Eric Seidel (no email)]

The filter failure is covered by bug 32294.

[Darin Adler]

Yes, this assertion is new.

We need a backtrace from the crash to figure out who is calling recomputeCaretRect without triggering layout.

[Eric Seidel (no email)]

Yet another case where bug 14861 would be useful.

[Nikolas Zimmermann]

svg/W3C-SVG-1.1/coords-coord-01-t.svg is not crashing, it was only the animate-elem-30-t.svg testcase. With the last SVG marker code rewrite, a SVGUseElement fix has been included, which may affect this bug (aka. fix it). Let's see if this ever happens again.

[Eric Seidel (no email)]

This happened again:
http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r52891%20(8951)/results.html

[Eric Seidel (no email)]

To be clear, specifically svg/W3C-SVG-1.1/animate-elem-30-t.svg crashed again on Leopard Debug.  See above link.

ASSERTION FAILED: !m_frame->view() || !m_frame->view()->needsLayout()
(/Volumes/Big/WebKit-BuildSlave/leopard-intel-debug/build/WebCore/editing/SelectionController.cpp:929 bool WebCore::SelectionController::recomputeCaretRect())

[Eric Seidel (no email)]

This is (not surprisingly) also crashing on the Snow Leopard Debug bot:
http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r52898%20(3248)/svg/W3C-SVG-1.1/animate-elem-30-t-stderr.txt

[Eric Seidel (no email)]

Saw this again:
http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r52896%20(8955)/results.html

This appears to be our most common crasher on the Leopard Bot.

I've not yet tried reproducing this locally.

[Eric Seidel (no email)]

Yup.  Worst failure on leopard bot right now:
http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r52902%20(8961)/results.html

[Eric Seidel (no email)]

run-webkit-tests --debug svg/W3C-SVG-1.1/animate-elem-30-t.svg

reproduces more than half the time for me.  you can run it with --iterations=100 to be sure to get a crash. :)

I've not bothered catching it in the debugger yet, but I have a crash report and will attach it shortly.

[Eric Seidel (no email)]

Created attachment 46025 [details]
crash log

I'm not sure the full stack trace is actually very helpful.  I suspect we'll have to catch this in the debugger to understand what's going on.

[Dirk Schulze]

I was not able to reproduce this crahs once on gtk :-(. Even with 100 iterations. Trying the debug build later.

[Nikolas Zimmermann]

I checked this again, it's definately not related to any <use> problem, it must be directly related to Darins patch, adding the new SelectionController assertions. Maybe they expose a bug in SVG text handling regarding selection state (aka. that we don't update anything correctly like HTML text does).
Darin, can you enlighten us, what may be triggering these assertions? Thanks in advance!

[Darin Adler]

(In reply to comment #13)
> Darin, can you enlighten us, what may be triggering these assertions? Thanks in
> advance!

The new assertions are in functions that assume layout. In general, functions can't get at the render tree unless they know layout has been performed.

In the past we were sloppy and imprecise about which functions would trigger layout if needed, and which required that layout already is done. This led to various crashes, even ones with security impact.

If the assertion fires, it means a function that depends on up-to-date layout was called when layout is needed but has not been done. There are two major ways to fix that:

    1) Look at the backtrace and figure out a higher level that can guarantee layout, usually by adding a call to Document::updateLayout. The design question is which functions depend on the caller to make sure the layout is up to date, and which take care of it themselves.

    2) Change the call with the assertion to perform layout instead of requiring it already be done. This can be tricky because there may be multiple call sites, and adding extra calls to layout can cause problems/crashes or slower performance.

I added a small number of assertions to the selection code, but generally we need more of these assertions in more places in the code.

The critical issue is figuring out who the caller is and what the backtrace is for these assertions. This probably requires a reproducible case, a bot that logs backtraces for crashes, or some inspired guesswork.

[Eric Seidel (no email)]

(In reply to comment #14)
> The critical issue is figuring out who the caller is and what the backtrace is
> for these assertions. This probably requires a reproducible case, a bot that
> logs backtraces for crashes, or some inspired guesswork.

The test case is reproducible, see comment 10.

A crash log including the full backtrace is attached, see comment 11 or attachment 46025 [details].

[Eric Seidel (no email)]

Is the problem that we're inside a layout already?
0   com.apple.WebCore             	0x05a578b3 WebCore::SelectionController::recomputeCaretRect() + 145 (SelectionController.cpp:929)
1   com.apple.WebCore             	0x0540c31b WebCore::Frame::selectionLayoutChanged() + 89 (Frame.cpp:642)
2   com.apple.WebCore             	0x054317eb WebCore::FrameView::layout(bool) + 2505 (FrameView.cpp:695)
3   com.apple.WebCore             	0x05431b41 WebCore::FrameView::forceLayout(bool) + 31 (FrameView.cpp:1809)
4   com.apple.WebKit              	0x00af4879 -[WebHTMLView layoutToMinimumPageWidth:maximumPageWidth:adjustingViewSize:] + 259 (WebHTMLView.mm:3012)
5   com.apple.WebKit              	0x00ae5128 -[WebHTMLView layout] + 68 (WebHTMLView.mm:3026)

[Darin Adler]

(In reply to comment #16)
> Is the problem that we're inside a layout already?
> 0   com.apple.WebCore                 0x05a578b3
> WebCore::SelectionController::recomputeCaretRect() + 145
> (SelectionController.cpp:929)
> 1   com.apple.WebCore                 0x0540c31b
> WebCore::Frame::selectionLayoutChanged() + 89 (Frame.cpp:642)
> 2   com.apple.WebCore                 0x054317eb
> WebCore::FrameView::layout(bool) + 2505 (FrameView.cpp:695)
> 3   com.apple.WebCore                 0x05431b41
> WebCore::FrameView::forceLayout(bool) + 31 (FrameView.cpp:1809)
> 4   com.apple.WebKit                  0x00af4879 -[WebHTMLView
> layoutToMinimumPageWidth:maximumPageWidth:adjustingViewSize:] + 259
> (WebHTMLView.mm:3012)
> 5   com.apple.WebKit                  0x00ae5128 -[WebHTMLView layout] + 68
> (WebHTMLView.mm:3026)

At that point in the layout process, needsLayout should already be false. I'm guessing that there's some situation where SVG code creates the need for a new layout while performing the current layout. That's a bug in SVG. But it may be hard to track down.

This does reflect a real bug. Inside recomputeCaretRect it's going to create a VisiblePosition and this will trigger a new layout! Extremely unpleasant.

I am not certain what the best thing to do is. We could remove the assertion for now to un-flakify the bots, which is sad for all the non-SVG cases and tests we could otherwise catch. We could add more targeted assertions to catch code trigger new layout during the layout process to try to pinpoint where the SVG code goes wrong.

[Eric Seidel (no email)]

I'm confused.  The call stack does not have SVG in it at all.  See the attached crash log.

[Nikolas Zimmermann]

Reran svg/W3C-SVG-1.1/animate-elem-30-t.svg with --repeat-each 100, can not reproduce it anymore. I've fixed the problem with the intermediate layouts during the <use> rewrite.

Actually Darins commented forced me to rewrite <use>, we shouldn't ever see crashes like this again :-)

[Darin Adler]

(In reply to comment #18)
> I'm confused. The call stack does not have SVG in it at all.

If the FrameView was doing a layout and during that, the code to lay out SVG elements triggered the need for additional layout, after that you could crash like this. The troublesome SVG code would have already run to completion so there would be no SVG code in the backtrace.

[Eric Seidel (no email)]

(In reply to comment #20)
> (In reply to comment #18)
> > I'm confused. The call stack does not have SVG in it at all.
> 
> If the FrameView was doing a layout and during that, the code to lay out SVG
> elements triggered the need for additional layout, after that you could crash
> like this. The troublesome SVG code would have already run to completion so
> there would be no SVG code in the backtrace.

So is it not safe to trigger additional layout during a layout()?

SVG does this:
http://trac.webkit.org/browser/trunk/WebCore/rendering/SVGRenderSupport.cpp#L253

[Darin Adler]

(In reply to comment #21)
> So is it not safe to trigger additional layout during a layout()?

Correct.

> SVG does this:
> http://trac.webkit.org/browser/trunk/WebCore/rendering/SVGRenderSupport.cpp#L253

Has always been incorrect and is a serious design problem.

[Nikolas Zimmermann]

(In reply to comment #22)
> (In reply to comment #21)
> > So is it not safe to trigger additional layout during a layout()?
> 
> Correct.
> 
> > SVG does this:
> > http://trac.webkit.org/browser/trunk/WebCore/rendering/SVGRenderSupport.cpp#L253
> 
> Has always been incorrect and is a serious design problem.

Huh?

Every RenderBlock calls child->layoutIfNeeded(), from it's own layout() method.
Where is the problem here? We're only layouting the subtree, no one retriggers a root layout from the middle of a layout.

[Darin Adler]

(In reply to comment #23)
> (In reply to comment #22)
> > > SVG does this:
> > > http://trac.webkit.org/browser/trunk/WebCore/rendering/SVGRenderSupport.cpp#L253
> > 
> > Has always been incorrect and is a serious design problem.
> 
> Huh?
> 
> Every RenderBlock calls child->layoutIfNeeded(), from it's own layout() method.
> Where is the problem here? We're only layouting the subtree, no one retriggers
> a root layout from the middle of a layout.

Sorry, I was in a rush did not look at the code and probably said the wrong thing.

It's wrong to trigger additional layout that will still not be complete at the end of the layout process. The code above is likely correct if it fits into the normal layout process.