iExploder(#460): Assertion failure in RenderObject::drawBorder()

[Alexey Proskuryakov]

run-iexploder-tests 460 (or just open the attached test case)

ASSERTION FAILED: x2 >= x1 (/Users/ap/WebKit/WebCore/rendering/RenderObject.cpp:974 void WebCore::RenderObject::drawBorder(WebCore::GraphicsContext*, int, int, int, int, WebCore::RenderObject::BorderSide, WebCore::Color, const WebCore::Color&, WebCore::EBorderStyle, int, int, bool))

Thread 0 Crashed:
0   com.apple.WebCore        	0x019bf550 WebCore::RenderObject::drawBorder(WebCore::GraphicsContext*, int, int, int, int, WebCore::RenderObject::BorderSide, WebCore::Color, WebCore::Color const&, WebCore::EBorderStyle, int, int, bool) + 6496 (RenderObject.cpp:974)
1   com.apple.WebCore        	0x019bed10 WebCore::RenderObject::drawBorder(WebCore::GraphicsContext*, int, int, int, int, WebCore::RenderObject::BorderSide, WebCore::Color, WebCore::Color const&, WebCore::EBorderStyle, int, int, bool) + 4384 (RenderObject.cpp:944)
2   com.apple.WebCore        	0x0198da28 WebCore::RenderFieldset::paintBorderMinusLegend(WebCore::GraphicsContext*, int, int, int, int, WebCore::RenderStyle const*, int, int) + 760 (render_form.cpp:446)
3   com.apple.WebCore        	0x0198e37c WebCore::RenderFieldset::paintBoxDecorations(WebCore::RenderObject::PaintInfo&, int, int) + 948 (render_form.cpp:420)
4   com.apple.WebCore        	0x01969978 WebCore::RenderBlock::paintObject(WebCore::RenderObject::PaintInfo&, int, int) + 276 (RenderBlock.cpp:1317)
5   com.apple.WebCore        	0x0195f5a4 WebCore::RenderBlock::paint(WebCore::RenderObject::PaintInfo&, int, int) + 616 (RenderBlock.cpp:1254)
6   com.apple.WebCore        	0x0195fa38 WebCore::RenderBlock::paintChildren(WebCore::RenderObject::PaintInfo&, int, int) + 812 (RenderBlock.cpp:1279)
7   com.apple.WebCore        	0x01969a44 WebCore::RenderBlock::paintObject(WebCore::RenderObject::PaintInfo&, int, int) + 480 (RenderBlock.cpp:1335)

...

[Alexey Proskuryakov]

Created attachment 7764 [details]
test case

Created attachment 7769 [details]
Reduced test case

[Alexey Proskuryakov]

Created attachment 10495 [details]
proposed fix

First attempt.

[Dave Hyatt]

Comment on attachment 10495 [details]
proposed fix

You can't manipulate width like this after the horizontal margins have been calculated.

I think a better way to patch this would be to do it in calcMinMaxWidth instead.

[Dave Hyatt]

Just subclass and then ensure that the fieldset is big enough for its m_minWidth to contain the legend's m_minWidth (and the same for m_maxWidth).

[Dave Hyatt]

Although I would also verify that this is actually what should be done.  WinIE cheats remember and treats the CSS width like a CSS min-width, so of course it grows the fieldset.  What does Firefox do?

[Alexey Proskuryakov]

(In reply to comment #4)
> I think a better way to patch this would be to do it in calcMinMaxWidth
> instead.

I have tried overriding calcMinMaxWidth in RenderFieldset, but setting m_minWidth and m_maxWidth there doesn't fix the issue - they seem to have no effect on the fieldset width.

void RenderFieldset::calcMinMaxWidth()
{
    ASSERT(!minMaxKnown());

    // a large value, just to test if it has any effect at all
    m_minWidth = 600;
    m_maxWidth = 600;

    setMinMaxKnown();
}

(In reply to comment #6)
> What does Firefox do?

Firefox does grow the fieldset, although in RTL case it still grows it to the right (making a horizontal scroll bar appear on the page). WinIE rendering in RTL case is just broken (the fieldset grows, but its top border doesn't leave enough space for the larger legend).

Created attachment 12142 [details]
Patch

The only good thing I can say about this patch is that it probably works.

calcMinMaxWidth needs to be patched. I could have copied and thinned down RenderBlock's implementation instead of tail-patching it. Not sure which way is better.

Then there is actual width calculation. This patch pretends that there's a CSS min-width protecting the legend. It takes care of the normal, inline and positioned cases. I looked into alternative approaches but all seemed to require turning more methods into virtual methods and/or copying large amount of code from RenderBox to RenderFieldset.

Any thoughts?

[Darin Adler]

Comment on attachment 12142 [details]
Patch

+const Length RenderBox::styleMinWidth(int containerWidth) const

Return value should just be Length, not "const Length".

[Dave Hyatt]

Comment on attachment 12142 [details]
Patch

I don't much like the styleMinWidth aspect of this.  I do like the change to calcMinMaxWidth in RenderFieldSet.cpp.

Could you patch calcWidth instead, after minwidth is checked (but before margin calculations)?

Seems like there you could have something like...

if (sizesToMinIntrinsicWidth())
    m_width = max(m_width, minWidth());

And then clean up the margin calculations by no longer treating width as auto if you had to grow m_width (so that calcHorizontalMargins gets called).

For positioning you could do something similar.  If sizesToMinIntrinsicWidth(), then you could do an additional calcAbsoluteHorizontalValues passing in a Fixed length that is m_minWidth.

Comment on attachment 12142 [details]
Patch

I'll try to do something along the lines of what Hyatt suggested.

(In reply to comment #10)
> Could you patch calcWidth instead, after minwidth is checked (but before margin
> calculations)?

I was almost done doing this when I realized it wouldn't work. It would make the fieldset stretch to fit the minimal intrinsic width of *any* of its children, not only the legend.

But now I checked and it looks like that's exactly how Firefox behaves, so I guess I'll just do that.

Created attachment 12256 [details]
Patch

No layout test regressions.

[Dave Hyatt]

Comment on attachment 12256 [details]
Patch

r=me

[Mark Rowe (bdash)]

Landed in r18788.

[Dave Hyatt]

This patch caused a regression.

[David Kilzer (:ddkilzer)]

(In reply to comment #16)
> This patch caused a regression.

Bug 13654.