Bug 112328 - XMLDocumentParser doesn't parse <template> correctly.
Summary: XMLDocumentParser doesn't parse <template> correctly.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Takashi Sakamoto
URL:
Keywords:
Depends on: 112408
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-14 01:36 PDT by Takashi Sakamoto
Modified: 2013-03-21 22:24 PDT (History)
9 users (show)

See Also:


Attachments
Patch (4.32 KB, patch)
2013-03-14 02:02 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (5.00 KB, patch)
2013-03-14 23:26 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (4.89 KB, patch)
2013-03-15 01:23 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (3.96 KB, patch)
2013-03-18 23:15 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (4.58 KB, patch)
2013-03-21 00:13 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Takashi Sakamoto 2013-03-14 01:36:59 PDT
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
Comment 1 Takashi Sakamoto 2013-03-14 02:02:35 PDT
Created attachment 193090 [details]
Patch
Comment 2 Dimitri Glazkov (Google) 2013-03-14 10:17:28 PDT
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.
Comment 3 Takashi Sakamoto 2013-03-14 18:47:52 PDT
(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.
Comment 4 WebKit Review Bot 2013-03-14 18:53:12 PDT
Comment on attachment 193090 [details]
Patch

Clearing flags on attachment: 193090

Committed r145864: <http://trac.webkit.org/changeset/145864>
Comment 5 WebKit Review Bot 2013-03-14 18:53:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Takashi Sakamoto 2013-03-14 21:17:42 PDT
(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.
Comment 7 Dimitri Glazkov (Google) 2013-03-14 21:19:49 PDT
Adam, Raf -- sounds like a <template> bug.
Comment 8 Takashi Sakamoto 2013-03-14 22:51:24 PDT
(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.
Comment 9 WebKit Review Bot 2013-03-14 22:53:30 PDT
Re-opened since this is blocked by bug 112408
Comment 10 Takashi Sakamoto 2013-03-14 23:26:58 PDT
Created attachment 193240 [details]
Patch
Comment 11 Takashi Sakamoto 2013-03-14 23:32:59 PDT
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.
Comment 12 Hajime Morrita 2013-03-15 00:22:06 PDT
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.
Comment 13 Takashi Sakamoto 2013-03-15 01:23:26 PDT
Created attachment 193257 [details]
Patch
Comment 14 Takashi Sakamoto 2013-03-15 01:24:25 PDT
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.
Comment 15 Adam Klein 2013-03-18 10:43:49 PDT
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.
Comment 16 Takashi Sakamoto 2013-03-18 23:15:19 PDT
Created attachment 193741 [details]
Patch
Comment 17 Takashi Sakamoto 2013-03-18 23:22:30 PDT
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.
Comment 18 Takashi Sakamoto 2013-03-19 03:18:29 PDT
Adam, would you take a look at my new patch?
Comment 19 Adam Klein 2013-03-19 11:57:44 PDT
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.
Comment 20 Adam Barth 2013-03-20 15:48:24 PDT
Comment on attachment 193741 [details]
Patch

Exiting early after 30 failures. 33517 tests run.
Comment 21 Takashi Sakamoto 2013-03-21 00:13:27 PDT
Created attachment 194195 [details]
Patch
Comment 22 Takashi Sakamoto 2013-03-21 00:18:04 PDT
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.
Comment 23 Adam Klein 2013-03-21 09:25:17 PDT
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).
Comment 24 Takashi Sakamoto 2013-03-21 19:36:07 PDT
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).
Comment 25 Hajime Morrita 2013-03-21 20:16:55 PDT
Comment on attachment 194195 [details]
Patch

Well. let me r+ this.
Comment 26 WebKit Review Bot 2013-03-21 22:24:39 PDT
Comment on attachment 194195 [details]
Patch

Clearing flags on attachment: 194195

Committed r146562: <http://trac.webkit.org/changeset/146562>
Comment 27 WebKit Review Bot 2013-03-21 22:24:44 PDT
All reviewed patches have been landed.  Closing bug.