Bug 6796

Summary: Reproducible CSS Crasher (infinite recursion adding/removing scrollbar)
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: Layout and RenderingAssignee: Beth Dakin <bdakin>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, hyatt, joost, mitz, mjs, samuel.sidler, webkitBugzilla
Priority: P1 Keywords: HasReduction, InRadar
Version: 417.x   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
Simple Test Case (causes crash)
none
Reduced Testcase
none
First attempt at halting infinte recursion
hyatt: review-
New patch
hyatt: review-
Single-bit guard around layoutBlock() call
none
no this one! hyatt: review+

Description Eric Seidel (no email) 2006-01-25 11:03:13 PST
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>
Comment 1 Eric Seidel (no email) 2006-01-25 11:03:38 PST
Created attachment 5952 [details]
Simple Test Case (causes crash)
Comment 2 Eric Seidel (no email) 2006-01-25 11:04:06 PST
This is in Radar as <rdar://problem/4406377>
Comment 3 Eric Seidel (no email) 2006-01-25 11:11:04 PST
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)
Comment 4 Joost de Valk (AlthA) 2006-01-25 11:23:33 PST
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.
Comment 5 mitz 2006-01-25 15:08:57 PST
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.
Comment 6 Eric Seidel (no email) 2006-01-25 15:11:13 PST
We need some hyatt here.
Comment 7 mitz 2006-01-25 15:55:01 PST
My last comment was wrong. Checking with Firefox, it looks like there shouldn't be a vertical scrollbar in the first place.
Comment 8 Darin Adler 2006-03-07 09:01:10 PST
*** Bug 7631 has been marked as a duplicate of this bug. ***
Comment 9 Darin Adler 2006-03-07 09:01:47 PST
Another test case in bug 7631. Lets make sure both of these test cases are fixed.
Comment 10 Beth Dakin 2006-03-10 20:10:36 PST
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?
Comment 11 Maciej Stachowiak 2006-03-11 02:09:13 PST
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.
Comment 12 Dave Hyatt 2006-03-13 14:20:51 PST
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.
Comment 13 Beth Dakin 2006-03-13 16:34:42 PST
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!
Comment 14 Darin Adler 2006-03-13 16:58:27 PST
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.
Comment 15 Dave Hyatt 2006-03-13 17:13:35 PST
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).
Comment 16 Dave Hyatt 2006-03-13 17:17:15 PST
Comment on attachment 7053 [details]
New patch

Yeah, Darin is right.  I have an idea for how this can be adjusted.
Comment 17 Dave Hyatt 2006-03-13 17:20:46 PST
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.
Comment 18 Beth Dakin 2006-03-14 14:00:58 PST
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.
Comment 19 Beth Dakin 2006-03-14 14:15:40 PST
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!
Comment 20 Dave Hyatt 2006-03-14 14:18:58 PST
Comment on attachment 7063 [details]
no this one!

r=me, be sure to file a followup bug about possibly adjusting min/maxwidth.
Comment 21 Beth Dakin 2006-03-14 16:03:57 PST
I filed http://bugzilla.opendarwin.org/show_bug.cgi?id=7768 to represent the min/max_width problem.
Comment 22 Marcus Pallinger 2006-04-26 15:42:20 PDT
Has this been merged into the Safari-2-0 branch, given that this crash also occurs in Safari 2.0.3?
Comment 23 mitz 2007-02-19 23:16:55 PST
*** Bug 12679 has been marked as a duplicate of this bug. ***
Comment 24 mitz 2007-04-28 22:31:17 PDT
*** Bug 13536 has been marked as a duplicate of this bug. ***