Bug 80773 – MathML crash in WebCore::Node::previousSibling()

• Component: MathML
• Status: RESOLVED FIXED

[Jacky Jiang]

Program terminated with signal 11, Segmentation fault.
#0  0x7d0f95f0 in WebCore::Node::previousSibling (this=0x0)
    at /Users/zhajiang/dev/webkit/Source/WebCore/dom/Node.h:152
152         Node* previousSibling() const { return m_previous; }
(gdb) bt
#0  0x7d0f95f0 in WebCore::Node::previousSibling (this=0x0)
    at /Users/zhajiang/dev/webkit/Source/WebCore/dom/Node.h:152
#1  0x7d1a58e0 in WebCore::Element::previousElementSibling (this=0x0)
    at /Users/zhajiang/dev/webkit/Source/WebCore/dom/Element.cpp:1844
#2  0x7e0c359c in WebCore::RenderMathMLSubSup::addChild (this=0x775a6170, child=0x7759f7f0, beforeChild=0x0)
    at /Users/zhajiang/dev/webkit/Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:70
#3  0x7d73cf48 in WebCore::RenderObject::addChild (this=0x775a6170, newChild=0x773c8498, beforeChild=0x0)
    at /Users/zhajiang/dev/webkit/Source/WebCore/rendering/RenderObject.cpp:319
#4  0x7d638cec in WebCore::RenderBlock::addChildIgnoringAnonymousColumnBlocks (this=0x775a6170, newChild=0x773c8498, 
    beforeChild=0x0) at /Users/zhajiang/dev/webkit/Source/WebCore/rendering/RenderBlock.cpp:778
#5  0x7d638f64 in WebCore::RenderBlock::addChildIgnoringContinuation (this=0x775a6170, newChild=0x773c8498, 
    beforeChild=0x0) at /Users/zhajiang/dev/webkit/Source/WebCore/rendering/RenderBlock.cpp:796
#6  0x7d638e7c in WebCore::RenderBlock::addChild (this=0x775a6170, newChild=0x773c8498, beforeChild=0x0)
    at /Users/zhajiang/dev/webkit/Source/WebCore/rendering/RenderBlock.cpp:789
#7  0x7e0c3b20 in WebCore::RenderMathMLSubSup::addChild (this=0x775a6170, child=0x773c8498, beforeChild=0x0)
    at /Users/zhajiang/dev/webkit/Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:111
#8  0x7d1db890 in WebCore::NodeRendererFactory::createRendererIfNeeded (this=0x779cf434)
    at /Users/zhajiang/dev/webkit/Source/WebCore/dom/NodeRenderingContext.cpp:350
#9  0x7d1bd740 in WebCore::Node::createRendererIfNeeded (this=0x55cc9f70)
    at /Users/zhajiang/dev/webkit/Source/WebCore/dom/Node.cpp:1484
#10 0x7d1a1bf4 in WebCore::Element::attach (this=0x55cc9f70)
    at /Users/zhajiang/dev/webkit/Source/WebCore/dom/Element.cpp:1025
#11 0x7dfa6548 in WebCore::HTMLConstructionSite::attach<WebCore::Element> (this=0x785f8fe4, rawParent=0x55cc94c8, 
    prpChild=...) at /Users/zhajiang/dev/webkit/Source/WebCore/html/parser/HTMLConstructionSite.cpp:111
#12 0x7dfa45e0 in WebCore::HTMLConstructionSite::attachToCurrent (this=0x785f8fe4, child=...)
    at /Users/zhajiang/dev/webkit/Source/WebCore/html/parser/HTMLConstructionSite.cpp:263
#13 0x7dfa4f64 in WebCore::HTMLConstructionSite::insertForeignElement (this=0x785f8fe4, token=..., namespaceURI=...)
    at /Users/zhajiang/dev/webkit/Source/WebCore/html/parser/HTMLConstructionSite.cpp:330
#14 0x7d363e58 in WebCore::HTMLTreeBuilder::processStartTag (this=0x785f8fd0, token=...)
    at /Users/zhajiang/dev/webkit/Source/WebCore/html/parser/HTMLTreeBuilder.cpp:1532
#15 0x7d35cbf8 in WebCore::HTMLTreeBuilder::processToken (this=0x785f8fd0, token=...)
    at /Users/zhajiang/dev/webkit/Source/WebCore/html/parser/HTMLTreeBuilder.cpp:481
#16 0x7d35ca8c in WebCore::HTMLTreeBuilder::constructTreeFromAtomicToken (this=0x785f8fd0, token=...)
    at /Users/zhajiang/dev/webkit/Source/WebCore/html/parser/HTMLTreeBuilder.cpp:462
#17 0x7d35c9a4 in WebCore::HTMLTreeBuilder::constructTreeFromToken (this=0x785f8fd0, rawToken=...)
    at /Users/zhajiang/dev/webkit/Source/WebCore/html/parser/HTMLTreeBuilder.cpp:452
#18 0x7d342294 in WebCore::HTMLDocumentParser::pumpTokenizer (this=0x772590a8, 
---Type <return> to continue, or q <return> to quit---
    mode=WebCore::HTMLDocumentParser::AllowYield)
    at /Users/zhajiang/dev/webkit/Source/WebCore/html/parser/HTMLDocumentParser.cpp:278
#19 0x7d341b40 in WebCore::HTMLDocumentParser::pumpTokenizerIfPossible (this=0x772590a8, 
    mode=WebCore::HTMLDocumentParser::AllowYield)
    at /Users/zhajiang/dev/webkit/Source/WebCore/html/parser/HTMLDocumentParser.cpp:177
#20 0x7d3429b8 in WebCore::HTMLDocumentParser::append (this=0x772590a8, source=...)
    at /Users/zhajiang/dev/webkit/Source/WebCore/html/parser/HTMLDocumentParser.cpp:370
#21 0x7df1aec8 in WebCore::DecodedDataDocumentParser::appendBytes (this=0x772590a8, writer=0x773a7b8c, 
    data=0x77ca0218 "\"true\" displaystyle=\"false\" displaystyle=\"false\" background=\"transparent\"><msqrt><mroot></mroot></msqrt></mstyle><mmultiscripts><merror>x</merror><mtable><mroot></mroot><mfrac></mfrac></mtable><msup><"..., 
    length=2819) at /Users/zhajiang/dev/webkit/Source/WebCore/dom/DecodedDataDocumentParser.cpp:50
#22 0x7d3f8610 in WebCore::DocumentWriter::addData (this=0x773a7b8c, 
    bytes=0x77ca0218 "\"true\" displaystyle=\"false\" displaystyle=\"false\" background=\"transparent\"><msqrt><mroot></mroot></msqrt></mstyle><mmultiscripts><merror>x</merror><mtable><mroot></mroot><mfrac></mfrac></mtable><msup><"..., 
    length=2819) at /Users/zhajiang/dev/webkit/Source/WebCore/loader/DocumentWriter.cpp:206
#23 0x7cfb88e0 in WebCore::FrameLoaderClientBlackBerry::receivedData (this=0x78efe378, 
    data=0x77ca0218 "\"true\" displaystyle=\"false\" displaystyle=\"false\" background=\"transparent\"><msqrt><mroot></mroot></msqrt></mstyle><mmultiscripts><merror>x</merror><mtable><mroot></mroot><mfrac></mfrac></mtable><msup><"..., 
    length=2819, textEncoding=...)
    at /Users/zhajiang/dev/webkit/Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:376
#24 0x7cfb82c4 in WebCore::FrameLoaderClientBlackBerry::committedLoad (this=0x78efe378, loader=0x773a7b00, 
    data=0x77ca0218 "\"true\" displaystyle=\"false\" displaystyle=\"false\" background=\"transparent\"><msqrt><mroot></mroot></msqrt></mstyle><mmultiscripts><merror>x</merror><mtable><mroot></mroot><mfrac></mfrac></mtable><msup><"..., 
    length=2819)
    at /Users/zhajiang/dev/webkit/Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:313
#25 0x7d3eac2c in WebCore::DocumentLoader::commitLoad (this=0x773a7b00, 
    data=0x77ca0218 "\"true\" displaystyle=\"false\" displaystyle=\"false\" background=\"transparent\"><msqrt><mroot></mroot></msqrt></mstyle><mmultiscripts><merror>x</merror><mtable><mroot></mroot><mfrac></mfrac></mtable><msup><"..., 
    length=2819) at /Users/zhajiang/dev/webkit/Source/WebCore/loader/DocumentLoader.cpp:303
#26 0x7d3eae7c in WebCore::DocumentLoader::receivedData (this=0x773a7b00, 
    data=0x77ca0218 "\"true\" displaystyle=\"false\" displaystyle=\"false\" background=\"transparent\"><msqrt><mroot></mroot></msqrt></mstyle><mmultiscripts><merror>x</merror><mtable><mroot></mroot><mfrac></mfrac></mtable><msup><"..., 
    length=2819) at /Users/zhajiang/dev/webkit/Source/WebCore/loader/DocumentLoader.cpp:329
#27 0x7d4221a0 in WebCore::MainResourceLoader::addData (this=0x77638e38, 
    data=0x77ca0218 "\"true\" displaystyle=\"false\" displaystyle=\"false\" background=\"transparent\"><msqrt><mroot></mroot></msqrt></mstyle><mmultiscripts><merror>x</merror><mtable><mroot></mroot><mfrac></mfrac></mtable><msup><"..., 
    length=2819, allAtOnce=false) at /Users/zhajiang/dev/webkit/Source/WebCore/loader/MainResourceLoader.cpp:171
---Type <return> to continue, or q <return> to quit---
#28 0x7d43831c in WebCore::ResourceLoader::didReceiveData (this=0x77638e38, 
    data=0x77ca0218 "\"true\" displaystyle=\"false\" displaystyle=\"false\" background=\"transparent\"><msqrt><mroot></mroot></msqrt></mstyle><mmultiscripts><merror>x</merror><mtable><mroot></mroot><mfrac></mfrac></mtable><msup><"..., 
    length=2819, encodedDataLength=2819, allAtOnce=false)
    at /Users/zhajiang/dev/webkit/Source/WebCore/loader/ResourceLoader.cpp:302
#29 0x7d42399c in WebCore::MainResourceLoader::didReceiveData (this=0x77638e38, 
    data=0x77ca0218 "\"true\" displaystyle=\"false\" displaystyle=\"false\" background=\"transparent\"><msqrt><mroot></mroot></msqrt></mstyle><mmultiscripts><merror>x</merror><mtable><mroot></mroot><mfrac></mfrac></mtable><msup><"..., 
    length=2819, encodedDataLength=2819, allAtOnce=false)
    at /Users/zhajiang/dev/webkit/Source/WebCore/loader/MainResourceLoader.cpp:473
#30 0x7d438d3c in WebCore::ResourceLoader::didReceiveData (this=0x77638e38, 
    data=0x77ca0218 "\"true\" displaystyle=\"false\" displaystyle=\"false\" background=\"transparent\"><msqrt><mroot></mroot></msqrt></mstyle><mmultiscripts><merror>x</merror><mtable><mroot></mroot><mfrac></mfrac></mtable><msup><"..., 
    length=2819, encodedDataLength=2819) at /Users/zhajiang/dev/webkit/Source/WebCore/loader/ResourceLoader.cpp:456
#31 0x7e33c784 in WebCore::NetworkJob::handleNotifyDataReceived (this=0x775b81e0, 
    buf=0x77ca0218 "\"true\" displaystyle=\"false\" displaystyle=\"false\" background=\"transparent\"><msqrt><mroot></mroot></msqrt></mstyle><mmultiscripts><merror>x</merror><mtable><mroot></mroot><mfrac></mfrac></mtable><msup><"..., 
    len=2819) at /Users/zhajiang/dev/webkit/Source/WebCore/platform/network/blackberry/NetworkJob.cpp:429
#32 0x7e33c440 in WebCore::NetworkJob::notifyDataReceivedPlain (this=0x775b81e0, 
    buf=0x77ca0218 "\"true\" displaystyle=\"false\" displaystyle=\"false\" background=\"transparent\"><msqrt><mroot></mroot></msqrt></mstyle><mmultiscripts><merror>x</merror><mtable><mroot></mroot><mfrac></mfrac></mtable><msup><"..., 
    len=2819) at /Users/zhajiang/dev/webkit/Source/WebCore/platform/network/blackberry/NetworkJob.cpp:381
#33 0x7e341ef8 in WebCore::NetworkJob::notifyDataReceived (this=0x775b81e0, buffer=0x77493870)
    at /Users/zhajiang/dev/webkit/Source/WebCore/platform/network/blackberry/NetworkJob.h:91
#34 0x7960eeb0 in BlackBerry::Platform::FilterStream::notifyDataReceived (this=0x550e87e0, buffer=0x77493870)
    at /Users/zhajiang/dev/platform/blackberryplatform/network/FilterStream.cpp:108
#35 0x79614c04 in BlackBerry::Platform::MultipartStream::notifyDataReceived (this=0x550e87e0, buffer=0x77493870)
    at /Users/zhajiang/dev/platform/blackberryplatform/network/MultipartStream.cpp:154
#36 0x7960eeb0 in BlackBerry::Platform::FilterStream::notifyDataReceived (this=0x78d2a5c0, buffer=0x77493870)
    at /Users/zhajiang/dev/platform/blackberryplatform/network/FilterStream.cpp:108
#37 0x79643fac in BlackBerry::Platform::RSSFilterStream::notifyDataReceived (this=0x78d2a5c0, buffer=0x77493870)
    at /Users/zhajiang/dev/platform/blackberryplatform/rss/RSSFilterStream.cpp:696
#38 0x7960eeb0 in BlackBerry::Platform::FilterStream::notifyDataReceived (this=0x56242db0, buffer=0x77493870)
    at /Users/zhajiang/dev/platform/blackberryplatform/network/FilterStream.cpp:108
#39 0x795edd7c in BlackBerry::Platform::ContentSnifferStream::notifyDataReceived (this=0x56242db0, buffer=0x77493870)
    at /Users/zhajiang/dev/platform/blackberryplatform/network/ContentSnifferStream.cpp:126
#40 0x7960eeb0 in BlackBerry::Platform::FilterStream::notifyDataReceived (this=0x3c3de610, buffer=0x77493870)
---Type <return> to continue, or q <return> to quit---
    at /Users/zhajiang/dev/platform/blackberryplatform/network/FilterStream.cpp:108
#41 0x795e3dcc in BlackBerry::Platform::CacheStream::notifyDataReceived (this=0x3c3de610, buffer=0x77493870)
    at /Users/zhajiang/dev/platform/blackberryplatform/network/CacheStream.cpp:357
#42 0x7960eeb0 in BlackBerry::Platform::FilterStream::notifyDataReceived (this=0x31121a70, buffer=0x77493870)
    at /Users/zhajiang/dev/platform/blackberryplatform/network/FilterStream.cpp:108
#43 0x796250d0 in BlackBerry::Platform::NetworkStream::notifyDataReceived (this=0x31121a70, buffer=0x77493870)
    at /Users/zhajiang/dev/platform/blackberryplatform/network/NetworkStream.cpp:292
#44 0x78be46e4 in WebKitThread::invokeNotifyDataReceived (this=0x7b5580c0, handle=40, buffer=0x77493870)
...

[Jacky Jiang]

This was introduced by https://bugs.webkit.org/show_bug.cgi?id=62098 and the changset http://trac.webkit.org/changeset/89273.

Anonymous child make the childElement null and crash WebKit.
void RenderMathMLSubSup::addChild(RenderObject* child, RenderObject* beforeChild)
{
    // Note: The RenderMathMLBlock only allows element children to be added.
    Element* childElement = toElement(child->node());

    if (!childElement->previousElementSibling()) {

  .....
}

[Jacky Jiang]

Created attachment 131201 [details]
The test case

This crash was also verified on QT port.

[Jacky Jiang]

Created attachment 131277 [details]
The patch

[Julien Chaffraix]

Comment on attachment 131277 [details]
The patch

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

> Source/WebCore/rendering/RenderObject.cpp:272
> +    if (!strcmp(renderName(), "RenderMathMLSubSup"))

First we *never* do a strcmp against renderName() and use a boolean function to return this value (guarded by ENABLE(MATHML) in this case). Here this is a hack and will not work if mtd is inserted below any other elements.

I would rather see a function introduced on RenderObject that returns whether the table should be inlined or not. This will be needed for https://bugs.webkit.org/show_bug.cgi?id=53144 AFAICT.

You will still have tons of issues (any table part <td>, <tr>, ... would wrongly be wrapped in an inline table) but at least it's more correct.

[Jacky Jiang]

Comment on attachment 131277 [details]
The patch

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

>> Source/WebCore/rendering/RenderObject.cpp:272
>> +    if (!strcmp(renderName(), "RenderMathMLSubSup"))
> 
> First we *never* do a strcmp against renderName() and use a boolean function to return this value (guarded by ENABLE(MATHML) in this case). Here this is a hack and will not work if mtd is inserted below any other elements.
> 
> I would rather see a function introduced on RenderObject that returns whether the table should be inlined or not. This will be needed for https://bugs.webkit.org/show_bug.cgi?id=53144 AFAICT.
> 
> You will still have tons of issues (any table part <td>, <tr>, ... would wrongly be wrapped in an inline table) but at least it's more correct.

Will update the patch.

[Jacky Jiang]

Created attachment 131416 [details]
Patch

Update the patch based on Julien's review.

[Jacky Jiang]

BTW, mlabeledtr of mtable is not needed for the layout test case, as it runs successfully.

[Julien Chaffraix]

Comment on attachment 131416 [details]
Patch

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

> Source/WebCore/rendering/RenderObject.h:852
> +    virtual bool needsInlineTable() const { return false; }

For the record, I think this is going in the right direction, though the function should not be virtual if MATHML is disabled. There are some corner cases that won't work with this change like <td> or <tr> would be wrapped into an anonymous inline-table and I don't think this is right.

As this seems orthogonal to the crash fix, I would rather see this in a new bug to discuss the proper testing.

> LayoutTests/mathml/msub-anonymous-child-render-crash.html:18
> +    <msub>
> +        <mi></mi>
> +        <mtr></mtr>
> +    </msub>
> +    <msub>
> +        <mi></mi>
> +        <mtd></mtd>
> +    </msub>

It would be neat to add testing for msup and msubsup too. That would validate that all the branches are not crashing.

[Jacky Jiang]

Comment on attachment 131416 [details]
Patch

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

>> Source/WebCore/rendering/RenderObject.h:852
>> +    virtual bool needsInlineTable() const { return false; }
> 
> For the record, I think this is going in the right direction, though the function should not be virtual if MATHML is disabled. There are some corner cases that won't work with this change like <td> or <tr> would be wrapped into an anonymous inline-table and I don't think this is right.
> 
> As this seems orthogonal to the crash fix, I would rather see this in a new bug to discuss the proper testing.

Why would "<td> or <tr>" doesn't work?  I think <td> or <tr>  is not allowed in MathML,  only <mtr> or <mtd> is allowed.  I would create a new bug for the layout issue to separate it.

>> LayoutTests/mathml/msub-anonymous-child-render-crash.html:18
>> +    </msub>
> 
> It would be neat to add testing for msup and msubsup too. That would validate that all the branches are not crashing.

msubsup wouldn't crash in this case, should I still add the test cases for msubsup?

[Julien Chaffraix]

Comment on attachment 131416 [details]
Patch

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

>>> Source/WebCore/rendering/RenderObject.h:852
>>> +    virtual bool needsInlineTable() const { return false; }
>> 
>> For the record, I think this is going in the right direction, though the function should not be virtual if MATHML is disabled. There are some corner cases that won't work with this change like <td> or <tr> would be wrapped into an anonymous inline-table and I don't think this is right.
>> 
>> As this seems orthogonal to the crash fix, I would rather see this in a new bug to discuss the proper testing.
> 
> Why would "<td> or <tr>" doesn't work?  I think <td> or <tr>  is not allowed in MathML,  only <mtr> or <mtd> is allowed.  I would create a new bug for the layout issue to separate it.

AFAICT nothing prevents us from adding some td and tr into a MathML tree (provided we do some namespace magic as we are speaking XHTML here). At least, RenderMathMLBlock::isChildAllowed doesn't prevent adding a RenderTableCell to the tree. If that doesn't work, MathML can be inlined in an HTML document.

Now because of your check, the td would get an anonymous inline-table which is *arguably* wrong as <msub> is considered to be a block. That is at least worth having some test covering whichever behavior we pick.

>>> LayoutTests/mathml/msub-anonymous-child-render-crash.html:18
>>> +    </msub>
>> 
>> It would be neat to add testing for msup and msubsup too. That would validate that all the branches are not crashing.
> 
> msubsup wouldn't crash in this case, should I still add the test cases for msubsup?

Yes, please.

[Jacky Jiang]

Comment on attachment 131416 [details]
Patch

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

>>>> Source/WebCore/rendering/RenderObject.h:852
>>>> +    virtual bool needsInlineTable() const { return false; }
>>> 
>>> AFAICT nothing prevents us from adding some td and tr into a MathML tree (provided we do some namespace magic as we are speaking XHTML here). At least, RenderMathMLBlock::isChildAllowed doesn't prevent adding a RenderTableCell to the tree. If that doesn't work, MathML can be inlined in an HTML document.
>>> 
>>> Now because of your check, the td would get an anonymous inline-table which is *arguably* wrong as <msub> is considered to be a block. That is at least worth having some test covering whichever behavior we pick.
>> 
>> Why would "<td> or <tr>" doesn't work?  I think <td> or <tr>  is not allowed in MathML,  only <mtr> or <mtd> is allowed.  I would create a new bug for the layout issue to separate it.
> 
> For the record, I think this is going in the right direction, though the function should not be virtual if MATHML is disabled. There are some corner cases that won't work with this change like <td> or <tr> would be wrapped into an anonymous inline-table and I don't think this is right.
> 
> As this seems orthogonal to the crash fix, I would rather see this in a new bug to discuss the proper testing.

That makes sense, will update the patch. Thanks!

[Jacky Jiang]

Created attachment 131454 [details]
Patch

Update the patch.

[Jacky Jiang]

https://bugs.webkit.org/show_bug.cgi?id=80909 has been created to track the rendering problem of this bug.

[Yong Li]

Comment on attachment 131454 [details]
Patch

Will the new created renders be leaked?

[Julien Chaffraix]

Comment on attachment 131454 [details]
Patch

> Will the new created renders be leaked?

That's a good point. The current code assumes that it will be inserted in the tree. If it is not, the pointer is leaked.

[Jacky Jiang]

(In reply to comment #15)
> (From update of attachment 131454 [details])
> > Will the new created renders be leaked?
> 
> That's a good point. The current code assumes that it will be inserted in the tree. If it is not, the pointer is leaked.
How would the node not be inserted to the tree? We don't have any return value of addChild() to make sure we have inserted the node successfully or not for all of the rendering not just this case I think.

[Jacky Jiang]

(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 131454 [details] [details])
> > > Will the new created renders be leaked?
> > 
> > That's a good point. The current code assumes that it will be inserted in the tree. If it is not, the pointer is leaked.
> How would the node not be inserted to the tree? We don't have any return value of addChild() to make sure we have inserted the node successfully or not for all of the rendering not just this case I think.

I would think it behaves as a regular anonymous render and gets destroyed when the parent render destroyed. However, for msub, msup and msubsup, we always wrap the child in a new container (not just this case), we may still keep the containers when their children get removed by DOM manipulation which I am not sure, I think this is the common possible issue which https://bugs.webkit.org/show_bug.cgi?id=57897 was trying to fix, but never get that patch in, we may need better design of the rendering. But in this case, I would just fix the crash.

[Jacky Jiang]

Created attachment 131620 [details]
html-anonymous-render-child

Just made a layout test for a normal anonymous render of html(Not the mathml), these two lines will be commented out at the beginning "var msub = document.getElementById("mydiv"); msub.removeChild(msub.childNodes[1]);", then put it back and compare the layout test results. Will attach the diff to support my opinion.

[Jacky Jiang]

Created attachment 131624 [details]
the pretty-diff

The diff shows that the anonymous render "RenderBlock (anonymous)" is still in the render tree
after the DOM manipulation to remove the child element "text run at (0,0)" of the anonymous render.
This shows that mathml is as normal as the common html that if the anonymous render is still in the render tree after DOM manipulation. 
Which might be the common problem or just by design which I am not sure. So, I think for mathml, anonymous render is fine as well.

[Julien Chaffraix]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > (From update of attachment 131454 [details] [details] [details])
> > > > Will the new created renders be leaked?
> > > 
> > > That's a good point. The current code assumes that it will be inserted in the tree. If it is not, the pointer is leaked.
> > How would the node not be inserted to the tree? We don't have any return value of addChild() to make sure we have inserted the node successfully or not for all of the rendering not just this case I think.
> 
> I would think it behaves as a regular anonymous render and gets destroyed when the parent render destroyed. 

I don't think you are following the issue. If you don't call addChild for the anonymous renderer, it has *no* parent. Thus it will never be cleaned and you leak your whole anonymous subtree. The test case you just pasted don't show anything.

You should see the issue by looking at the tree dump of your crash fix. There should be some missing anonymous RenderTable. You can also visualize that in your text output by putting some text in your <mtd> or <mtr> (there should be some missing).

[Jacky Jiang]

(In reply to comment #20)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > (In reply to comment #15)
> > > > (From update of attachment 131454 [details] [details] [details] [details])
> > > > > Will the new created renders be leaked?
> > > > 
> > > > That's a good point. The current code assumes that it will be inserted in the tree. If it is not, the pointer is leaked.
> > > How would the node not be inserted to the tree? We don't have any return value of addChild() to make sure we have inserted the node successfully or not for all of the rendering not just this case I think.
> > 
> > I would think it behaves as a regular anonymous render and gets destroyed when the parent render destroyed. 
> 
> I don't think you are following the issue. If you don't call addChild for the anonymous render, it has *no* parent. Thus it will never be cleaned and you leak your whole anonymous subtree. The test case you just pasted don't show anything.
> 
> You should see the issue by looking at the tree dump of your crash fix. There should be some missing anonymous RenderTable. You can also visualize that in your text output by putting some text in your <mtd> or <mtr> (there should be some missing).
The test just attached was about the DOM manipulation case of the anonymous render.
OK, do you mean dump the render tree and see if all of the anonymous renders of <mtd> or <mtr> are there or not?  And also add pixel test result to the patch?   I can't imagine how there is a case that the anonymous render is not inserted to the tree though.

[Jacky Jiang]

Created attachment 131649 [details]
dump render tree of the test case of the patch

[Jacky Jiang]

(In reply to comment #22)
> Created an attachment (id=131649) [details]
> dump render tree of the test case of the patch

This is the dump render tree of the test case in the requested review patch. 
This shows that all of the anonymous renders of <mtr> <mtd> are attached to the render tree.
Is this dump render tree expected file what you want to add to patch?  Hopefully I got what you mean.

[Jacky Jiang]

Created attachment 131684 [details]
Patch

Update the patch with the dump render tree to make sure that anonymous render's and their children are attched to the render tree.

[Dave Barton]

(In reply to comment #20)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > (In reply to comment #15)
> > > > (From update of attachment 131454 [details] [details] [details] [details])
> > > > > Will the new created renders be leaked?
> > > > 
> > > > That's a good point. The current code assumes that it will be inserted in the tree. If it is not, the pointer is leaked.
> > > How would the node not be inserted to the tree? We don't have any return value of addChild() to make sure we have inserted the node successfully or not for all of the rendering not just this case I think.
> > 
> > I would think it behaves as a regular anonymous render and gets destroyed when the parent render destroyed. 
> 
> I don't think you are following the issue. If you don't call addChild for the anonymous renderer, it has *no* parent. Thus it will never be cleaned and you leak your whole anonymous subtree. The test case you just pasted don't show anything.
> 
> You should see the issue by looking at the tree dump of your crash fix. There should be some missing anonymous RenderTable. You can also visualize that in your text output by putting some text in your <mtd> or <mtr> (there should be some missing).

I agree with Jacky. The original bug is that for an <msub> or <msup>, if a table part (<mtd>/etc.) is inserted as the second child, then RenderMathMLSubSup::addChild() at old line 114 will call RenderMathMLBlock::addChild(), which is really RenderObject::addChild(), to do the insert. RenderObject::addChild() will create an anonymous table renderer in this case, and then call RenderMathMLSubSup::addChild() again to insert it. This is the only way an anonymous renderer gets created. The only bug is that RenderMathMLSubSup::addChild() doesn't check for childElement being NULL in this case. With the patch's check, the code will run correctly, and the anonymous renderer will get inserted when RenderMathMLBlock::addChild() at old line 114 again gets executed. So addChild() indeed gets called on the anonymous renderer.

[Jacky Jiang]

(In reply to comment #25)
> (In reply to comment #20)
> > (In reply to comment #17)
> > > (In reply to comment #16)
> > > > (In reply to comment #15)
> > > > > (From update of attachment 131454 [details] [details] [details] [details] [details])
> > > > > > Will the new created renders be leaked?
> > > > > 
> > > > > That's a good point. The current code assumes that it will be inserted in the tree. If it is not, the pointer is leaked.
> > > > How would the node not be inserted to the tree? We don't have any return value of addChild() to make sure we have inserted the node successfully or not for all of the rendering not just this case I think.
> > > 
> > > I would think it behaves as a regular anonymous render and gets destroyed when the parent render destroyed. 
> > 
> > I don't think you are following the issue. If you don't call addChild for the anonymous renderer, it has *no* parent. Thus it will never be cleaned and you leak your whole anonymous subtree. The test case you just pasted don't show anything.
> > 
> > You should see the issue by looking at the tree dump of your crash fix. There should be some missing anonymous RenderTable. You can also visualize that in your text output by putting some text in your <mtd> or <mtr> (there should be some missing).
> 
> I agree with Jacky. The original bug is that for an <msub> or <msup>, if a table part (<mtd>/etc.) is inserted as the second child, then RenderMathMLSubSup::addChild() at old line 114 will call RenderMathMLBlock::addChild(), which is really RenderObject::addChild(), to do the insert. RenderObject::addChild() will create an anonymous table renderer in this case, and then call RenderMathMLSubSup::addChild() again to insert it. This is the only way an anonymous renderer gets created. The only bug is that RenderMathMLSubSup::addChild() doesn't check for childElement being NULL in this case. With the patch's check, the code will run correctly, and the anonymous renderer will get inserted when RenderMathMLBlock::addChild() at old line 114 again gets executed. So addChild() indeed gets called on the anonymous renderer.
Exactly^_^, thanks for the agreement.

[Julien Chaffraix]

Comment on attachment 131684 [details]
Patch

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

> I agree with Jacky.

Yes, now I see that I just overlook the ASSERT. Thanks Dave & Jacky for correcting me.

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:100
> +            if (!childElement)
> +                return;

If this ever get reached, we will leak |child| and was what prompted my comment about leaking. The alternative is crashing so I guess it's OK.

> LayoutTests/mathml/msub-anonymous-child-render-crash.html:7
> +        <mi>X</mi>
> +        <mtr>3</mtr>

Adding text unfortunately here makes the render tree dump platform specific due to font difference. This can be alleviated but IMHO it doesn't add much as it was caused by me being confused.

Sorry to get you chase wild rabbits here but could you just revert it to dumpAsText?

[Jacky Jiang]

Comment on attachment 131684 [details]
Patch

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

>> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:100
>> +                return;
> 
> If this ever get reached, we will leak |child| and was what prompted my comment about leaking. The alternative is crashing so I guess it's OK.

Yeah,  the ASSERT would help although it shouldn't ever get into this path.

>> LayoutTests/mathml/msub-anonymous-child-render-crash.html:7
>> +        <mtr>3</mtr>
> 
> Adding text unfortunately here makes the render tree dump platform specific due to font difference. This can be alleviated but IMHO it doesn't add much as it was caused by me being confused.
> 
> Sorry to get you chase wild rabbits here but could you just revert it to dumpAsText?

No problem, I will revert it to dumpAsText and remove the text.

[Julien Chaffraix]

Comment on attachment 131684 [details]
Patch

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

>>> LayoutTests/mathml/msub-anonymous-child-render-crash.html:7
>>> +        <mtr>3</mtr>
>> 
>> Adding text unfortunately here makes the render tree dump platform specific due to font difference. This can be alleviated but IMHO it doesn't add much as it was caused by me being confused.
>> 
>> Sorry to get you chase wild rabbits here but could you just revert it to dumpAsText?
> 
> No problem, I will revert it to dumpAsText and remove the text.

You can keep the text (even maybe should as it makes the text output nicer with no downside). It's only if you do a tree dump (like in this patch) that the text is making your output platform specific.

[Jacky Jiang]

(In reply to comment #29)
> (From update of attachment 131684 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131684&action=review
> 
> >>> LayoutTests/mathml/msub-anonymous-child-render-crash.html:7
> >>> +        <mtr>3</mtr>
> >> 
> >> Adding text unfortunately here makes the render tree dump platform specific due to font difference. This can be alleviated but IMHO it doesn't add much as it was caused by me being confused.
> >> 
> >> Sorry to get you chase wild rabbits here but could you just revert it to dumpAsText?
> > 
> > No problem, I will revert it to dumpAsText and remove the text.
> 
> You can keep the text (even maybe should as it makes the text output nicer with no downside). It's only if you do a tree dump (like in this patch) that the text is making your output platform specific.
OK, I will keep the text and dump it.  The layout of the test should be improved in the new created follow up patch as the style would be different that we are not inline the anonymous table.

[Jacky Jiang]

Created attachment 131717 [details]
Patch

Revert to the previois dumpAsText patch as suggested by Julien.

[Julien Chaffraix]

Comment on attachment 131717 [details]
Patch

r=me

[Jacky Jiang]

(In reply to comment #32)
> (From update of attachment 131717 [details])
> r=me
Thanks a lot for your time to review this ^_^.

[WebKit Review Bot]

Comment on attachment 131717 [details]
Patch

Clearing flags on attachment: 131717

Committed r110640: <http://trac.webkit.org/changeset/110640>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.