Bug 20765 - Website crashes on load due to messy HTML in search form
Summary: Website crashes on load due to messy HTML in search form
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P1 Critical
Assignee: Beth Dakin
URL: http://outpost10f.com
Keywords: HasReduction, InRadar
: 24247 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-09-10 08:14 PDT by Alexis Deveria
Modified: 2009-04-17 11:09 PDT (History)
5 users (show)

See Also:


Attachments
Minimum HTML needed to cause crash (64 bytes, text/html)
2008-09-10 08:22 PDT, Alexis Deveria
no flags Details
Patch (39.74 KB, patch)
2009-04-16 13:40 PDT, Beth Dakin
hyatt: review+
Details | Formatted Diff | Diff
patch to clean up some loose ends (6.85 KB, patch)
2009-04-17 06:24 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
better version of "loose ends" patch (6.38 KB, patch)
2009-04-17 06:48 PDT, Darin Adler
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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