Bug 112487

Created attachment 193410 [details] Patch

Comment on attachment 193410 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193410&action=review > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:968 > if (!m_tree.currentStackItem()->hasLocalName(token->name())) Not a big deal since it's "just" a parse error, but seems like hasTagName() would be better here.

Comment on attachment 193410 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193410&action=review >> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:968 >> if (!m_tree.currentStackItem()->hasLocalName(token->name())) > > Not a big deal since it's "just" a parse error, but seems like hasTagName() would be better here. At this point, we know that |token| is an HTMLTemplate element, otherwise we wouldn't be here. I see what you are getting at, but it seems fine to me that HTML parser insertion modes can make the assumption that the current token is HTML. The danger we are fixing her is that the steps for processing foreignContent may have put non-HTML elements on the stack, so we have to be careful when examining it.

Comment on attachment 193410 [details] Patch Ok. Please make sure we stay in sync with the spec if this issue isn't resolved the way we expect.

Comment on attachment 193410 [details] Patch Clearing flags on attachment: 193410 Committed r146028: <http://trac.webkit.org/changeset/146028>

All reviewed patches have been landed. Closing bug.

Comment on attachment 193410 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193410&action=review >>> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:968 >>> if (!m_tree.currentStackItem()->hasLocalName(token->name())) >> >> Not a big deal since it's "just" a parse error, but seems like hasTagName() would be better here. > > At this point, we know that |token| is an HTMLTemplate element, otherwise we wouldn't be here. I see what you are getting at, but it seems fine to me that HTML parser insertion modes can make the assumption that the current token is HTML. The danger we are fixing her is that the steps for processing foreignContent may have put non-HTML elements on the stack, so we have to be careful when examining it. I'm not sure how you can make the assumption that because |token| is being treated as HTML, that currentStackItem is also HTML. Perhaps because this is processTemplateEndTag? If it were a start tag, then you definitely couldn't make that assumption (since, e.g., <mtext> in MathML will cause the parser to treat tokens as HTML, and in that case, the currentStackItem will have a MathML namespace URI, though of course it won't have a <template> localName). Anyway, again, just a parse error, but I think we'll want to tighten up _all_ these callsites.

Comment on attachment 193410 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193410&action=review >>>> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:968 >>>> if (!m_tree.currentStackItem()->hasLocalName(token->name())) >>> >>> Not a big deal since it's "just" a parse error, but seems like hasTagName() would be better here. >> >> At this point, we know that |token| is an HTMLTemplate element, otherwise we wouldn't be here. I see what you are getting at, but it seems fine to me that HTML parser insertion modes can make the assumption that the current token is HTML. The danger we are fixing her is that the steps for processing foreignContent may have put non-HTML elements on the stack, so we have to be careful when examining it. > > I'm not sure how you can make the assumption that because |token| is being treated as HTML, that currentStackItem is also HTML. Perhaps because this is processTemplateEndTag? If it were a start tag, then you definitely couldn't make that assumption (since, e.g., <mtext> in MathML will cause the parser to treat tokens as HTML, and in that case, the currentStackItem will have a MathML namespace URI, though of course it won't have a <template> localName). > > Anyway, again, just a parse error, but I think we'll want to tighten up _all_ these callsites. You are correct. Using hasTagName(templateTag)