Summary: | Reproducible CSS Crasher (infinite recursion adding/removing scrollbar) | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||||||||||
Component: | Layout and Rendering | Assignee: | 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
Eric Seidel (no email)
2006-01-25 11:03:13 PST
Created attachment 5952 [details]
Simple Test Case (causes crash)
This is in Radar as <rdar://problem/4406377> 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) 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. 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. *** Bug 7631 has been marked as a duplicate of this bug. *** Another test case in bug 7631. Lets make sure both of these test cases are fixed. 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?
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 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.
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 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 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 on attachment 7053 [details]
New patch
Yeah, Darin is right. I have an idea for how this can be adjusted.
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. 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.
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 on attachment 7063 [details]
no this one!
r=me, be sure to file a followup bug about possibly adjusting min/maxwidth.
I filed http://bugzilla.opendarwin.org/show_bug.cgi?id=7768 to represent the min/max_width problem. 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. *** |