Bug 112328 – XMLDocumentParser doesn't parse <template> correctly.

• Component: CSS
• Status: RESOLVED FIXED

[Takashi Sakamoto]

Fuzzer	 Inferno_twister
Job Type	Linux_asan_chrome_mp
Crash type	UNKNOWN
Crash address	0x000000000018
Crash state	- crash stack -
WebCore::CSSDefaultStyleSheets::ensureDefaultStyleSheetsForElement
WebCore::StyleResolver::styleForElement
WebCore::Document::styleForElementIgnoringPendingStylesheets

https://cluster-fuzz.appspot.com/testcase?key=169974990

[Takashi Sakamoto]

Created attachment 193090 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 193090 [details]
Patch

I am sort of surprised that style resolving machinery is even invoked inside of a template. We probably shouldn't do that at all.

[Takashi Sakamoto]

(In reply to comment #2)
> (From update of attachment 193090 [details])
> I am sort of surprised that style resolving machinery is even invoked inside of a template. We probably shouldn't do that at all.

I see. I will investigate why invoked inside of a template.

[WebKit Review Bot]

Comment on attachment 193090 [details]
Patch

Clearing flags on attachment: 193090

Committed r145864: <http://trac.webkit.org/changeset/145864>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Takashi Sakamoto]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 193090 [details] [details])
> > I am sort of surprised that style resolving machinery is even invoked inside of a template. We probably shouldn't do that at all.
> 
> I see. I will investigate why invoked inside of a template.

I found that <object> in <template> is attached while parsing XHTML. I think, this is XMLDocumentParser's bug. (While parsing HTML, the <object> is not attached.)

Once <object> is attached, <title>'s childrenChanged will invoke computedStyle.
So the crash will occur.

[Dimitri Glazkov (Google)]

Adam, Raf -- sounds like a <template> bug.

[Takashi Sakamoto]

(In reply to comment #7)
> Adam, Raf -- sounds like a <template> bug.

I found how to fix this issue.
So I'm now reverting my previous patch and will update a new patch.

[WebKit Review Bot]

Re-opened since this is blocked by bug 112408

[Takashi Sakamoto]

Created attachment 193240 [details]
Patch

[Takashi Sakamoto]

I found the followings:

Using HTML:
- children of <template> are appended to the <template>'s content.
- children of <template> are not attached after parsing.

Using XHTML:
- children of <template> are children of the <template>.
- children of <template> are attached.

So if <title> in <template> is attached, HTMLTitleElement::childrenChanged invoke style resolving code... crash.

[Hajime Morrita]

Comment on attachment 193240 [details]
Patch

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

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:845
> +#endif

Could you minimize difference between #if and #else paths? Diverging behavior here smells like another source of confusion.

[Takashi Sakamoto]

Created attachment 193257 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 193240 [details]
Patch

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

Thank you for reviewing.

>> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:845
>> +#endif
> 
> Could you minimize difference between #if and #else paths? Diverging behavior here smells like another source of confusion.

I see. Done.

[Adam Klein]

Comment on attachment 193257 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        Implemented the logic in executeTask in HTMLConstructionSite.cpp.

I don't think you need to copy as much logic from HTMLConstructionSite as you have below. Note that in the XHTML parser, the template content is itself pushed onto the stack, which makes the code much simpler (and doesn't require fixing up parent later).

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:826
> +        parent = toHTMLTemplateElement(m_currentNode)->content();

I don't think you need this block at all, see below.

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:844
> +#if ENABLE(TEMPLATE_ELEMENT)

Any reason to #if-guard this? It seems like checking that the parent is attached is always more correct...

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:845
> +        && newElement->parentNode() && parent->attached()

Can't this just be "newElement->parentNode() && newElement->parentNode()->attached()"? I see that the HTML parser calls attached() on something other than parentNode(), but that seems likely just a typo in the HTML parser that needn't be duplicated here.

[Takashi Sakamoto]

Created attachment 193741 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 193257 [details]
Patch

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

Thank you for reviewing.

I missed pushCurrentNode updates m_currentNode and m_currentNode will be never <template>.

>> Source/WebCore/ChangeLog:15
>> +        Implemented the logic in executeTask in HTMLConstructionSite.cpp.
> 
> I don't think you need to copy as much logic from HTMLConstructionSite as you have below. Note that in the XHTML parser, the template content is itself pushed onto the stack, which makes the code much simpler (and doesn't require fixing up parent later).

I see. Yeah, I don't need to copy. What I have to do is just checking whether m_currentNode is attached or not.

>> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:826
>> +        parent = toHTMLTemplateElement(m_currentNode)->content();
> 
> I don't think you need this block at all, see below.

Removed.

>> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:844
>> +#if ENABLE(TEMPLATE_ELEMENT)
> 
> Any reason to #if-guard this? It seems like checking that the parent is attached is always more correct...

I removed #if-guard and replaced parent->attached() with m_currentNode->attached().

>> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:845
>> +        && newElement->parentNode() && parent->attached()
> 
> Can't this just be "newElement->parentNode() && newElement->parentNode()->attached()"? I see that the HTML parser calls attached() on something other than parentNode(), but that seems likely just a typo in the HTML parser that needn't be duplicated here.

I think, we can only check whether m_currentNode is attached or not.

[Takashi Sakamoto]

Adam, would you take a look at my new patch?

[Adam Klein]

Comment on attachment 193741 [details]
Patch

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

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:833
> +    if (m_view && m_currentNode->attached() && !newElement->attached())

I don't think this will work (and I'm surprised it doesn't break more tests, though glancing at EWS it seems that it does break a good number). At this point, in all cases except for <template>, m_currentNode and newElement point to the same object, since pushCurrentNode() mutates m_currentNode.

[Adam Barth]

Comment on attachment 193741 [details]
Patch

Exiting early after 30 failures. 33517 tests run.

[Takashi Sakamoto]

Created attachment 194195 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 193741 [details]
Patch

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

Thank you for reviewing.

>> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:833
>> +    if (m_view && m_currentNode->attached() && !newElement->attached())
> 
> I don't think this will work (and I'm surprised it doesn't break more tests, though glancing at EWS it seems that it does break a good number). At this point, in all cases except for <template>, m_currentNode and newElement point to the same object, since pushCurrentNode() mutates m_currentNode.

Thanks, Adam. You are right. I have to copy m_currentNode's value to some local variable. I fixed this.
I also checked the reason why only 30 tests failed... I found that nodes were attached in Element::recalcStyle. i.e.

#9  0x0000000000d5dec0 in WebCore::Node::reattach (this=0x206985b1a0c0)
#10 0x00000000010cc45c in WebCore::Element::recalcStyle (this=0x206985b1a0c0, 
#11 0x000000000106ff82 in WebCore::Document::recalcStyle (this=0x2069859c4020, 
#12 0x0000000001074e77 in WebCore::Document::styleResolverChanged (
#13 0x00000000028a04f3 in WebCore::XMLDocumentParser::end (this=0x206985c4cca0)

I think, this hid my mistake.

[Adam Klein]

Comment on attachment 194195 [details]
Patch

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

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:839
> +    if (m_view && currentNode->attached() && !newElement->attached())

Okay, this seems fine to me. You could have just used newElement->parentNode(), but the two should be equivalent (since parserAppendChild shouldn't dispatch any synchronous events).

[Takashi Sakamoto]

Thank you, Adam.

(In reply to comment #23)
> (From update of attachment 194195 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194195&action=review
> 
> > Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:839
> > +    if (m_view && currentNode->attached() && !newElement->attached())
> 
> Okay, this seems fine to me. You could have just used newElement->parentNode(), but the two should be equivalent (since parserAppendChild shouldn't dispatch any synchronous events).

[Hajime Morrita]

Comment on attachment 194195 [details]
Patch

Well. let me r+ this.

[WebKit Review Bot]

Comment on attachment 194195 [details]
Patch

Clearing flags on attachment: 194195

Committed r146562: <http://trac.webkit.org/changeset/146562>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.