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
Created attachment 193090 [details] Patch
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.
(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 on attachment 193090 [details] Patch Clearing flags on attachment: 193090 Committed r145864: <http://trac.webkit.org/changeset/145864>
All reviewed patches have been landed. Closing bug.
(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.
Adam, Raf -- sounds like a <template> bug.
(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.
Re-opened since this is blocked by bug 112408
Created attachment 193240 [details] Patch
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 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.
Created attachment 193257 [details] Patch
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 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.
Created attachment 193741 [details] Patch
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.
Adam, would you take a look at my new patch?
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 on attachment 193741 [details] Patch Exiting early after 30 failures. 33517 tests run.
Created attachment 194195 [details] Patch
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 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).
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 on attachment 194195 [details] Patch Well. let me r+ this.
Comment on attachment 194195 [details] Patch Clearing flags on attachment: 194195 Committed r146562: <http://trac.webkit.org/changeset/146562>