Bug 20765

Summary:

Website crashes on load due to messy HTML in search form

Product:

WebKit

Reporter:

Alexis Deveria <adeveria>

Component:

DOM

Assignee:

Beth Dakin <bdakin>

Status:

RESOLVED FIXED

Severity:

Critical

CC:

bdakin, mitz, sky, webkit, zwarich

Priority:

Keywords:

HasReduction, InRadar

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

OS X 10.5

URL:

http://outpost10f.com

Attachments:

Description	Flags
Minimum HTML needed to cause crash	none
Patch	hyatt: review+
patch to clean up some loose ends	none
better version of "loose ends" patch	hyatt: review+

Description Alexis Deveria 2008-09-10 08:14:54 PDT

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.

Comment 1 Alexis Deveria 2008-09-10 08:22:36 PDT

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

Comment 2 Mark Rowe (bdash) 2008-09-10 12:51:43 PDT

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

Comment 3 Mark Rowe (bdash) 2008-09-10 12:58:39 PDT

<rdar://problem/6210633>

Comment 4 Alexis Deveria 2008-09-11 06:02:20 PDT

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

Comment 5 Beth Dakin 2009-04-16 13:22:10 PDT

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

Comment 6 Beth Dakin 2009-04-16 13:40:30 PDT

Created attachment 29547 [details]
Patch

Comment 7 Dave Hyatt 2009-04-16 13:45:14 PDT

Comment on attachment 29547 [details]
Patch

r=me

Comment 8 Beth Dakin 2009-04-16 13:48:53 PDT

Fixed with r42586.

Comment 9 Scott Violet 2009-04-16 13:51:53 PDT

Thanks Beth!

Comment 10 Darin Adler 2009-04-17 06:24:29 PDT

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

Comment 11 Darin Adler 2009-04-17 06:48:21 PDT

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

Comment 12 Dave Hyatt 2009-04-17 10:21:42 PDT

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.

Comment 13 Darin Adler 2009-04-17 11:01:15 PDT

(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.

Comment 14 Dave Hyatt 2009-04-17 11:09:55 PDT

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

r=me