Bug 6796 – Reproducible CSS Crasher (infinite recursion adding/removing scrollbar)

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Eric Seidel (no email)]

Simple reduction attached.

Also here:

<style>
body{
    font-size:11px;
    line-height:127%
}
div{
    float:left;
    overflow:auto
}
div label{
    float:left;
    font-size:110%
}
div span{
    float:right;
}
</style> 
<div>
    <label>foo</label>
    <span>bar</span>
</div>

[Eric Seidel (no email)]

Created attachment 5952 [details]
Simple Test Case (causes crash)

[Eric Seidel (no email)]

This is in Radar as <rdar://problem/4406377>

[Eric Seidel (no email)]

Crash Report:

It's a recursive loop.  We're blowing out the stack.

Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_INVALID_ADDRESS (0x0001) at 0xbf7ffff0

Thread 0 Crashed:
0   com.apple.CoreFoundation 	0x90740b80 CFRetain + 24
1   com.apple.CoreFoundation 	0x907405c0 _CFArrayReplaceValues + 372
2   com.apple.CoreFoundation 	0x90740438 CFArrayAppendValue + 200
3   com.apple.AppKit         	0x93695748 -[NSView _addSubview:] + 124
4   com.apple.AppKit         	0x936949f0 -[NSView _setSuperview:] + 320
5   com.apple.AppKit         	0x936954f0 -[NSView addSubview:] + 352
6   com.apple.WebKit         	0x0127bac4 -[WebHTMLView addSubview:] + 96 (WebHTMLView.m:2237)
7   com.apple.WebCore        	0x01d025ec QWidget::addToSuperview(NSView*) + 532 (KWQWidget.mm:530)
8   com.apple.WebCore        	0x01cdfdb8 QScrollView::addChild(QWidget*, int, int) + 528 (KWQScrollView.mm:360)
9   com.apple.WebCore        	0x02009d00 WebCore::RenderLayer::setHasVerticalScrollbar(bool) + 196 (render_layer.cpp:743)
10  com.apple.WebCore        	0x0200a1b8 WebCore::RenderLayer::updateScrollInfoAfterLayout() + 856 (render_layer.cpp:883)
11  com.apple.WebCore        	0x01fd3b6c WebCore::RenderBlock::layoutBlock(bool) + 3648 (RenderBlock.cpp:546)
12  com.apple.WebCore        	0x0200a2cc WebCore::RenderLayer::updateScrollInfoAfterLayout() + 1132 (render_layer.cpp:893)
13  com.apple.WebCore        	0x01fd3b6c WebCore::RenderBlock::layoutBlock(bool) + 3648 (RenderBlock.cpp:546)
14  com.apple.WebCore        	0x0200a2cc WebCore::RenderLayer::updateScrollInfoAfterLayout() + 1132 (render_layer.cpp:893)
15  com.apple.WebCore        	0x01fd3b6c WebCore::RenderBlock::layoutBlock(bool) + 3648 (RenderBlock.cpp:546)
16  com.apple.WebCore        	0x0200a2cc WebCore::RenderLayer::updateScrollInfoAfterLayout() + 1132 (render_layer.cpp:893)
17  com.apple.WebCore        	0x01fd3b6c WebCore::RenderBlock::layoutBlock(bool) + 3648 (RenderBlock.cpp:546)
18  com.apple.WebCore        	0x0200a2cc WebCore::RenderLayer::updateScrollInfoAfterLayout() + 1132 (render_layer.cpp:893)
19  com.apple.WebCore        	0x01fd3b6c WebCore::RenderBlock::layoutBlock(bool) + 3648 (RenderBlock.cpp:546)
20  com.apple.WebCore        	0x0200a2cc WebCore::RenderLayer::updateScrollInfoAfterLayout() + 1132 (render_layer.cpp:893)
21  com.apple.WebCore        	0x01fd3b6c WebCore::RenderBlock::layoutBlock(bool) + 3648 (RenderBlock.cpp:546)
22  com.apple.WebCore        	0x0200a2cc WebCore::RenderLayer::updateScrollInfoAfterLayout() + 1132 (render_layer.cpp:893)

[Joost de Valk (AlthA)]

Created attachment 5953 [details]
Reduced Testcase

Reduced it a bit more. Reducing the line-height percentage or font-size more, keeps the crash from occurring.

The <label>, which is a float, has height 19 and overflow height 20.

The <div> grows to contain only 19 pixels, at which point it decides that it needs a vertical scrollbar. That leaves not enough room for the <span>, which makes it move to the next line, but then, the float's height discrepancy is no longer an issue, the <div> no longer needs that scrollbar, the <span> can move up again, and on and on it goes...

I think the problem is right there at the beginning, when the <div> uses the FloatingObject's endY, which is based on height, rather than something that takes overflow height into account.

Still no idea what to fix, let alone how.

[Eric Seidel (no email)]

We need some hyatt here.

My last comment was wrong. Checking with Firefox, it looks like there shouldn't be a vertical scrollbar in the first place.

[Darin Adler]

*** Bug 7631 has been marked as a duplicate of this bug. ***

[Darin Adler]

Another test case in bug 7631. Lets make sure both of these test cases are fixed.

[Beth Dakin]

Created attachment 7001 [details]
First attempt at halting infinte recursion

I am not sure if this is exactly right, but it fixes the test case and doesn't break any layout tests. Thoughts?

[Maciej Stachowiak]

Looks good to me. Make sure to try both test cases, and to add them as layout tests. I'll give hyatt a chance to give it the once over.

[Dave Hyatt]

Comment on attachment 7001 [details]
First attempt at halting infinte recursion

Move the variables into RenderLayer and try to keep as much of the scrolling logic there as you can.

I'd try to make the min/max width modification only be done if the infinite recursion case was hit.  I think it would also be fine to simply not have this code in an initial check-in as well.

[Beth Dakin]

Created attachment 7053 [details]
New patch

After talking with Hyatt, I decided not to mess with the min and max width in this check-in. Here is a new patch!

[Darin Adler]

Comment on attachment 7053 [details]
New patch

There's one major thing I don't understand: When does anything ever reset sawHorizontalBar or sawVerticalBar back to false? What happens if the content get smaller and doesn't need a horizontal scrollbar any more? Does the layer get destroyed and re-created in that case?

+    bool sawHorizontalBar() { return m_sawHorizontalBar; }
+    bool sawVerticalBar() { return m_sawVerticalBar; }

Those should be const member functions.

+    if (needHorizontalBar)
+        setSawHorizontalBar(needHorizontalBar);

Could just be setSawHorizontalBar(true);

+    if (needVerticalBar)
+        setSawVerticalBar(needVerticalBar);

Same here.

[Dave Hyatt]

Comment on attachment 7053 [details]
New patch

r=me.  Make sure to run layout tests before landing (and to add a layout test for this problem).

[Dave Hyatt]

Comment on attachment 7053 [details]
New patch

Yeah, Darin is right.  I have an idea for how this can be adjusted.

[Dave Hyatt]

I think you can put a guard around the spot in RenderLayer that does a re-layout when overflow is AUTO.  A single bit re-entrancy guard on the layer should do it.

[Beth Dakin]

Created attachment 7062 [details]
Single-bit guard around layoutBlock() call

Like this, Dave? In the specific test case for this bug, this patch prevents the crash but yields a nasty layout. "foobar" is on one line and a scroll bar is drawn over most of the "bar" as if "foo" and "bar" were stacked. It is worth noting, though, that some of the other sites that we know of that crash in this same way (like the one in the bug that is marked as a dup of this bugzilla) no longer crash with this patch and lay out just fine.

[Beth Dakin]

Created attachment 7063 [details]
no this one!

Okay, newer patch with some Hyatt-comments from IRC taken into account. This one doesn't yield a nasty layout either. yay!

[Dave Hyatt]

Comment on attachment 7063 [details]
no this one!

r=me, be sure to file a followup bug about possibly adjusting min/maxwidth.

[Beth Dakin]

I filed http://bugzilla.opendarwin.org/show_bug.cgi?id=7768 to represent the min/max_width problem.

[Marcus Pallinger]

Has this been merged into the Safari-2-0 branch, given that this crash also occurs in Safari 2.0.3?

*** Bug 12679 has been marked as a duplicate of this bug. ***

*** Bug 13536 has been marked as a duplicate of this bug. ***