Bug 20765 – Website crashes on load due to messy HTML in search form

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Alexis Deveria]

Crashes in latest nightly Webkit, as well as Safari 3.1 and Chrome 0.2 on OSX 10.5, Windows XP and presumably other OSes.

This is the minimal HTML I've found required to replicate the crash:

<table><tr><b><form style="display:inline"></b><td>a</td><td><p>

I have contacted the website and suggested they remove the <b> tags, since they don't serve any purpose in this case anyway.

[Alexis Deveria]

Created attachment 23319 [details]
Minimum HTML needed to cause crash

[Mark Rowe (bdash)]

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0xbbadbeef
0x03559cdd in WebCore::RenderContainer::appendChildNode (this=0x1d00b97c, newChild=0x1cb66e9c, fullAppend=true) at WebCore/rendering/RenderContainer.cpp:417
417	    ASSERT(!isBlockFlow() || (!newChild->isTableSection() && !newChild->isTableRow() && !newChild->isTableCell()));
(gdb) bt
#0  0x03559cdd in WebCore::RenderContainer::appendChildNode (this=0x1d00b97c, newChild=0x1cb66e9c, fullAppend=true) at WebCore/rendering/RenderContainer.cpp:417
#1  0x035752bc in WebCore::RenderInline::splitFlow (this=0x1d00b34c, beforeChild=0x0, newBlockBox=0x1d00b88c, newChild=0x1d00b7ec, oldCont=0x0) at WebCore/rendering/RenderInline.cpp:255
#2  0x035755a2 in WebCore::RenderInline::addChildToFlow (this=0x1d00b34c, newChild=0x1d00b7ec, beforeChild=0x0) at WebCore/rendering/RenderInline.cpp:122

[Mark Rowe (bdash)]

<rdar://problem/6210633>

[Alexis Deveria]

The website ( http://outpost10f.com/ ) has now removed the buggy HTML, so it won't crash any more.

[Beth Dakin]

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

[Beth Dakin]

Created attachment 29547 [details]
Patch

[Dave Hyatt]

Comment on attachment 29547 [details]
Patch

r=me

[Beth Dakin]

Fixed with r42586.

[Scott Violet]

Thanks Beth!

[Darin Adler]

Created attachment 29575 [details]
patch to clean up some loose ends

[Darin Adler]

Created attachment 29576 [details]
better version of "loose ends" patch

[Dave Hyatt]

Comment on attachment 29576 [details]
better version of "loose ends" patch

I don't understand why it's ok to remove this line:

bool wrapInAnonymousSection = !child->isPositioned();

and replace it with false.

[Darin Adler]

(In reply to comment #12)
> (From update of attachment 29576 [details] [review])
> I don't understand why it's ok to remove this line:
> 
> bool wrapInAnonymousSection = !child->isPositioned();
> 
> and replace it with false.

Because every single code path after that sets wrapInAnonymousSection to either true or false; that value is ignored and my patch doesn't change behavior at all.

If we need correct handling of positioned elements we need to fix the code to not do that any more. And write test cases so it doesn't break again.

Also, the other tables renderers that do wrapping similarly don't check isPositioned.

[Dave Hyatt]

Comment on attachment 29576 [details]
better version of "loose ends" patch

r=me