Bug 70678 – Crash in WebCore::RenderTableSection::addChild due to assert failure

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Dimitris Apostolou]

Created attachment 112086 [details]
Crash log.

r

Reproducibility: always

Steps:
Go to http://dictionary.reference.com/browse/hem

What happened:
Infinite loop, assert failure and crash.

ASSERTION FAILED: row->isAnonymous()
/Users/rex/WebKit/Source/WebCore/rendering/RenderTableSection.cpp(123) : virtual void WebCore::RenderTableSection::addChild(WebCore::RenderObject *, WebCore::RenderObject *)
1   0x1054c81e8 WebCore::RenderTableSection::addChild(WebCore::RenderObject*, WebCore::RenderObject*)
2   0x105247e20 WebCore::NodeRendererFactory::createRendererIfNeeded()
3   0x10522cad9 WebCore::Node::createRendererIfNeeded()
4   0x104885a78 WebCore::Element::attach()
5   0x1044867d6 WebCore::Node::reattach()
6   0x1048863f3 WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
7   0x104886baf WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
8   0x104886baf WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
9   0x104886baf WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
10  0x104886baf WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
11  0x104886baf WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
12  0x104886baf WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
13  0x104886baf WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
14  0x104886baf WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
15  0x104886baf WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
16  0x104886baf WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
17  0x104886baf WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
18  0x104886baf WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
19  0x104886baf WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
20  0x104886baf WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
21  0x104886baf WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
22  0x104886baf WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
23  0x104886baf WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
24  0x1046b74e1 WebCore::Document::recalcStyle(WebCore::Node::StyleChange)
25  0x1046b8559 WebCore::Document::updateStyleIfNeeded()
26  0x1046c2cff WebCore::Document::finishedParsing()
27  0x104af8eff WebCore::HTMLTreeBuilder::finished()
28  0x104a449e9 WebCore::HTMLDocumentParser::end()
29  0x104a4398c WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd()
30  0x104a43787 WebCore::HTMLDocumentParser::prepareToStopParsing()
31  0x104a441a1 WebCore::HTMLDocumentParser::endIfDelayed()

Expected result:
No infinite loop, no assert failure, no crash.

[Dimitris Apostolou]

r98157

[Fady Samuel]

I'm seeing this bug in Chromium on Linux as well.

[Abhishek Arya]

(In reply to comment #2)
> I'm seeing this bug in Chromium on Linux as well.

We would need a reduction to see what is going on. But as i understand, we should probably need to change that assert into a hard check, and not adding it as a child when previous sibling is not anonymous. We should just continue forward in that function and let it get shoved into a new anonymous block. note that you would need fixes in rendertable, rendertablesection, rendertablerow and rendertablecell. (same code exists there, check out bug 69536.)

[Fady Samuel]

Created attachment 112224 [details]
Reduction: Inserting span between two rows triggers an ASSERT failure

[Abhishek Arya]

Nice, so now you need to see whether the span has to go in its own new anonymous block or it has to go as a last child of row-1. Please compare with firefox, opera renderings.

[Abhishek Arya]

(In reply to comment #5)
> Nice, so now you need to see whether the span has to go in its own new anonymous block or it has to go as a last child of row-1. Please compare with firefox, opera renderings.

Add this to testcase. 	  spanElement.appendChild(document.createTextNode("Some text.")); 

You will see the wrong rendering. You need to change that assert into a hard check. We need to add the child into its own new anonymous row.

[Fady Samuel]

(In reply to comment #6)
> (In reply to comment #5)
> > Nice, so now you need to see whether the span has to go in its own new anonymous block or it has to go as a last child of row-1. Please compare with firefox, opera renderings.
> 
> Add this to testcase.       spanElement.appendChild(document.createTextNode("Some text.")); 
> 
> You will see the wrong rendering. You need to change that assert into a hard check. We need to add the child into its own new anonymous row.

Yup, Firefox inserts the text as a row between row-1, and row-2.

[Fady Samuel]

Created attachment 112230 [details]
Improved Reduction

[Abhishek Arya]

(In reply to comment #8)
> Created an attachment (id=112230) [details]
> Improved Reduction

Perfect, do remember to fix all the three places

Source/WebCore/rendering/RenderTable.cpp (view diffs)
Source/WebCore/rendering/RenderTableRow.cpp (view diffs)
Source/WebCore/rendering/RenderTableSection.cpp (view diffs)

[Fady Samuel]

(In reply to comment #9)
> (In reply to comment #8)
> > Created an attachment (id=112230) [details] [details]
> > Improved Reduction
> 
> Perfect, do remember to fix all the three places
> 
> Source/WebCore/rendering/RenderTable.cpp (view diffs)
> Source/WebCore/rendering/RenderTableRow.cpp (view diffs)
> Source/WebCore/rendering/RenderTableSection.cpp (view diffs)

Yup, will do, I probably needs tests for all three cases right? Three layout tests.

[Abhishek Arya]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Created an attachment (id=112230) [details] [details] [details]
> > > Improved Reduction
> > 
> > Perfect, do remember to fix all the three places
> > 
> > Source/WebCore/rendering/RenderTable.cpp (view diffs)
> > Source/WebCore/rendering/RenderTableRow.cpp (view diffs)
> > Source/WebCore/rendering/RenderTableSection.cpp (view diffs)
> 
> Yup, will do, I probably needs tests for all three cases right? Three layout tests.

Yeah. You can add all the cases in one test too.

[Fady Samuel]

Created attachment 112265 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 112265 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112265&action=review

> Source/WebCore/ChangeLog:13
> +        we need to create a new anonymous Section/Row/Cell respectively, insteading failing an

"insteading" -> instead of

[Fady Samuel]

Created attachment 112267 [details]
Patch

[Fady Samuel]

(In reply to comment #13)
> (From update of attachment 112265 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112265&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        we need to create a new anonymous Section/Row/Cell respectively, insteading failing an
> 
> "insteading" -> instead of

Oops, fixed.

[Dave Hyatt]

Comment on attachment 112267 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 112267 [details]
Patch

Clearing flags on attachment: 112267

Committed r98372: <http://trac.webkit.org/changeset/98372>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.