Bug 124128

Created attachment 216534 [details] testcase

Created attachment 216556 [details] Patch V1 (test)

Comment on attachment 216556 [details] Patch V1 (test) Attachment 216556 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/22908330 New failing tests: js/basic-set.html mathml/presentation/mixing.html

Created attachment 216558 [details] Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Comment on attachment 216556 [details] Patch V1 (test) Attachment 216556 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/22699394 New failing tests: js/basic-set.html mathml/presentation/mixing.html

Created attachment 216559 [details] Archive of layout-test-results from webkit-ews-07 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Comment on attachment 216556 [details] Patch V1 (test) Attachment 216556 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/22908370 New failing tests: js/basic-set.html mathml/presentation/mixing.html

Created attachment 216562 [details] Archive of layout-test-results from webkit-ews-03 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-03 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Mixing arbitrary markup with MathML was introducing a case where non-MathML nodes were being interpreted as MathML. This could pose a security risk. Based on the language in http://www.w3.org/TR/MathML/chapter6.html#world-int-combine-other, it seemed like allowing arbitrary markup inside MathML documents was not intended to work. Indeed, the MathML validator at W3C (http://validator.w3.org) rejects example with non-MathML markup inside the MathML document. For these reasons, I turned off the ability to host non-MathML inside a document as part of the security fix https://bugs.webkit.org/show_bug.cgi?id=121728

(In reply to comment #9) > Mixing arbitrary markup with MathML was introducing a case where non-MathML nodes were being interpreted as MathML. This could pose a security risk. > > Based on the language in http://www.w3.org/TR/MathML/chapter6.html#world-int-combine-other, it seemed like allowing arbitrary markup inside MathML documents was not intended to work. Indeed, the MathML validator at W3C (http://validator.w3.org) rejects example with non-MathML markup inside the MathML document. > > For these reasons, I turned off the ability to host non-MathML inside a document as part of the security fix https://bugs.webkit.org/show_bug.cgi?id=121728 Chapter 6 mentions "Extending the schema in this way is easily achieved using the Relax NG schema described in Appendix A Parsing MathML, it may be as simple as including the MathML schema whilst overriding the content model of mtext:" In particular, the HTML5 validator modifies the RelaxNG schema to allow foreign content in <mtext> elements. However, my testcase is not correct, I think only inline elements are allowed (I don't know the exact list). At least it will validate if you replace <p> by <span> in the test case. Allowing foreign content is an important feature for native MathML (for example to allow features like in the Mozilla demos or http://www.youtube.com/watch?v=imIwtZ-mNcc or "SVG in equation" in http://golem.ph.utexas.edu/wiki/instiki/show/Sandbox) Also, this is needed to implement the <semantics> element, otherwise we can not display alternate <annotation-xml> elements. Finally, there are a couple of unit tests that will fail or become incorrect if we are not able to use HTML in MathML. mtext and annotation-xml are implemented in Gecko and there is a similar feature in SVG with the foreignObject (which is implemented in WebKit), so I guess there is a way to allow that in a secured way? I suspect it should be handled on the HTML5 parser side, to do the correct namespace mapping.

Created attachment 217147 [details] testcase namespace Here is another testcase to check the namespaces. It seems that the elements are correctly mapped to SVG or HTML, not MathML so the corresponding renderers should be used.

(In reply to comment #10) > In particular, the HTML5 validator modifies the RelaxNG schema to allow foreign content in <mtext> elements. However, my testcase is not correct, I think only inline elements are allowed (I don't know the exact list). At least it will validate if you replace <p> by <span> in the test case. For the record, here is the HTML5 RelaxNG schema https://bitbucket.org/validator/validator/src/5ee4172d2929787d5b78519c2035e62b503eee6c/schema/xhtml5-svg-mathml.rnc?at=default#cl-31 so <mtext> can contain "common.elem.phrasing", which includes "svg", "math" and other HTML elements.

The HTML5 (In reply to comment #9) > Mixing arbitrary markup with MathML was introducing a case where non-MathML nodes were being interpreted as MathML. This could pose a security risk. > The HTML5 spec is explicit that this should be allowed. Phrasing content is allowed in the token elements mo, mn, ms, mi, mtext and annotation-xml if it has encoding attribute text/html or application/xhtml+xml. http://www.w3.org/html/wg/drafts/html/CR/syntax.html#parsing (or the equivalent in the whatwg version) A node is a MathML text integration point if it is one of the following elements: An mi element in the MathML namespace An mo element in the MathML namespace An mn element in the MathML namespace An ms element in the MathML namespace An mtext element in the MathML namespace exactly what that implies is scatted through the parser state description but basically it means html is allowed there.

(In reply to comment #13) > The HTML5 (In reply to comment #9) > > Mixing arbitrary markup with MathML was introducing a case where non-MathML nodes were being interpreted as MathML. This could pose a security risk. > > > > The HTML5 spec is explicit that this should be allowed. Someone queried off list whether I meant that a security bug should not be fixed. Sorry if I gave that impression: that is not what I meant. I just meant to point out that the points at which mathml and html can be nested is specified in detail in the normative html(5) parsing spec and not just in the non normative schema as used in the validator.nu based validators (as is often the case the validator is in fact stricter than the implementation requirements and declares invalid constructs that are defined to parse without error, in particular it tries to guide authors to use mtext or annotation-xml as the integration points for nesting html, but the parser spec allows any of the token elements, such as mi).

(In reply to comment #14) Maybe this is a good time for me to introduce David Carlisle, if any of you don't know him. He's been the chief editor of the w3c MathML spec for many years, so obviously knows all the spec issues extremely well. He's also been happy to clarify any such issues whenever anyone at webkit wants. Fred Wang also knows the MathML spec extremely well (I believe he's also on the w3c MathML committee), but Carlisle is really the #1 authority in the world, can give history and rationale, etc., is a great guy, and wants to help if he can. I encourage you all to cc him on any MathML bug pages without hesitation. (Fred is obviously very good to cc also.)

My understanding is that the parser allowed non-MathML elements to be inserted arbitrarily inside MathML and these elements were incorrectly interpreted as MathML elements, thus causing potential security issues. If the parser conforms with the HTML5 spec and correctly interprets the foreign elements at the MathML integration points (token elements and <annotation-xml>) then it won't be a problem any more. Note that this situation happens in SVG's <foreignObject> too and I already restored the possibility to insert foreign content in <annotation-xml> (bug 100626). I plan to work on this bug later since I'd like the refactoring and code clean up of token elements (bug 124838) to be finished first (if one reviewer could take a look at the patch on that bug, that would be helpful btw).

Created attachment 226426 [details] Patch This patch restores foreign content in mtext/mn/mi for phrasing content: http://www.w3.org/TR/html5/dom.html#phrasing-content-1 The case of mo is a bit tedious, so it is not handled here. Anyway, the HTML5 validator currently only allows foreign content in <mtext> so that's what people are going to use in practice. I also tried to restrict the content of annotation-xml to Flow content + <html> as indicated in the RelaxNG schema, but that breaks some tests relying on foreign <mtext> and it is a bit difficult to keep perfect rendering matching otherwise. So I finally didn't change what annotation-xml accepts. http://www.w3.org/TR/html5/dom.html#flow-content. This should hopefully make mspace-units pass again.

Created attachment 226488 [details] Patch

Comment on attachment 226488 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226488&action=review Seems OK, but might need some additional refinement to be completely correct. > Source/WebCore/mathml/MathMLTextElement.cpp:92 > + return (e.isMathMLElement() && e.hasTagName(MathMLNames::mathTag)) || (e.isSVGElement() && e.hasTagName(SVGNames::svgTag)) || (e.isHTMLElement() && (e.hasTagName(HTMLNames::aTag) || e.hasTagName(HTMLNames::abbrTag) || e.hasTagName(HTMLNames::areaTag) || e.hasTagName(HTMLNames::audioTag) || e.hasTagName(HTMLNames::bTag) || e.hasTagName(HTMLNames::bdiTag) || e.hasTagName(HTMLNames::bdoTag) || e.hasTagName(HTMLNames::brTag) || e.hasTagName(HTMLNames::buttonTag) || e.hasTagName(HTMLNames::canvasTag) || e.hasTagName(HTMLNames::citeTag) || e.hasTagName(HTMLNames::codeTag) || e.hasTagName(HTMLNames::datalistTag) || e.hasTagName(HTMLNames::delTag) || e.hasTagName(HTMLNames::dfnTag) || e.hasTagName(HTMLNames::emTag) || e.hasTagName(HTMLNames::embedTag) || e.hasTagName(HTMLNames::iTag) || e.hasTagName(HTMLNames::iframeTag) || e.hasTagName(HTMLNames::imgTag) || e.hasTagName(HTMLNames::inputTag) || e.hasTagName(HTMLNames::insTag) || e.hasTagName(HTMLNames::kbdTag) || e.hasTagName(HTMLNames::keygenTag) || e.hasTagName(HTMLNames::labelTag) || e.hasTagName(HTMLNames::mapTag) || e.hasTagName(HTMLNames::markTag) || e.hasTagName(HTMLNames::meterTag) || e.hasTagName(HTMLNames::noscriptTag) || e.hasTagName(HTMLNames::objectTag) || e.hasTagName(HTMLNames::outputTag) || e.hasTagName(HTMLNames::progressTag) || e.hasTagName(HTMLNames::qTag) || e.hasTagName(HTMLNames::rubyTag) || e.hasTagName(HTMLNames::sTag) || e.hasTagName(HTMLNames::sampTag) || e.hasTagName(HTMLNames::scriptTag) || e.hasTagName(HTMLNames::selectTag) || e.hasTagName(HTMLNames::smallTag) || e.hasTagName(HTMLNames::spanTag) || e.hasTagName(HTMLNames::strongTag) || e.hasTagName(HTMLNames::subTag) || e.hasTagName(HTMLNames::supTag) || e.hasTagName(HTMLNames::templateTag) || e.hasTagName(HTMLNames::textareaTag) || e.hasTagName(HTMLNames::uTag) || e.hasTagName(HTMLNames::varTag) || e.hasTagName(HTMLNames::videoTag) || e.hasTagName(HTMLNames::wbrTag))); Writing this all out on one line is messy. I recommend separate sections for MathML, SVG, and HTML. Also, could use hasLocalName here for better efficiency, although that will change once my patch for bug 130090 lands. Once that is in, we’ll want to use hasTagName, except that we’ll want to cast the element reference to the more specific element type. > Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:65 > + m_isTextOnly = !element().firstElementChild(); What this implements is “has no element descendants”, which is not exactly “is text only”. It would be better to name this consistent with the implementation unless the two are truly precisely equivalent. I’m also concerned that we are making a decision here based on DOM tree rather than rendering tree. What if there’s an element child that is styled "display: none"? > Source/WebCore/rendering/mathml/RenderMathMLToken.h:57 > virtual void updateStyle(); > + bool m_isTextOnly; I would suggest a blank line between the function members and the data member.

Comment on attachment 226488 [details] Patch Attachment 226488 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5350057318547456 New failing tests: mathml/wbr-in-mroot-crash.html

Created attachment 226644 [details] Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Created attachment 226717 [details] Patch This one applies on top of the patch for bug 130090.

(In reply to comment #19) > > Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:65 > > + m_isTextOnly = !element().firstElementChild(); > > What this implements is “has no element descendants”, which is not exactly “is text only”. It would be better to name this consistent with the implementation unless the two are truly precisely equivalent. > > I’m also concerned that we are making a decision here based on DOM tree rather than rendering tree. What if there’s an element child that is styled "display: none"? Since RenderObject::isRenderElement() is defined by "return !isText()", I thought that this was equivalent. It's true however that relying on the DOM may not be a good idea. I initially tried to rely on the render objects. But the problem is that in addition to the anonymous RenderMathMLBlock wrapper created by RenderMathMLToken, there is another RenderBlock child created somewhere else (I'm not sure where it comes from). So I have to check that there is not any element in toRenderElement(toRenderElement(firstChild())->firstChild()) rather than in the RenderMathMLBlock wrapper. This assumption on the structure of renderer hierarchy seemed a bit fragile, that's why I switched to the DOM-based check. I have restored the verification based on the renderer objects in the current patch, but I'm not sure what's best.

Comment on attachment 226717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226717&action=review > Source/WebCore/ChangeLog:8 > + This restores foreign content in <mtext>, <mn> and <mi> elements, but only when it is "phrasing content" as defined per the HTML5 specification. Other token elements are not handled here. This change makes mspace-units.html works again on Linux. ... This change makes mspace-units.html ""work"" again on Linux... > Source/WebCore/mathml/MathMLTextElement.cpp:99 > + auto& svgElement = toSVGElement(element); it doesn't seem necessary to convert to an SVG element. I think this could be written as if (element.isSVG) return element.hasTagName(svgTag); ditto for MathMLElement above > Source/WebCore/mathml/MathMLTextElement.cpp:160 > +bool MathMLTextElement::childShouldCreateRenderer(const Node& child) const extraneous space added > Source/WebCore/mathml/MathMLTextElement.cpp:164 > + // FIXME: phrasing content should be accepted in <mo> elements too. bug number? > Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:69 > + const auto& block = toRenderElement(toRenderElement(firstChild())->firstChild()); are we always guaranteed to have a grandchild for this element? > Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:70 > + m_hasChildElement = childrenOfType<RenderElement>(*block).first(); is this asking if there's a great-grandchild of this object?

(In reply to comment #24) > (From update of attachment 226717 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=226717&action=review > > Source/WebCore/mathml/MathMLTextElement.cpp:99 > > + auto& svgElement = toSVGElement(element); > > it doesn't seem necessary to convert to an SVG element. > I think this could be written as > > if (element.isSVG) > return element.hasTagName(svgTag); > > ditto for MathMLElement above > Perhaps I misunderstood what Darin said in comment 19, but I thought that after bug 130090 we want to cast to SVG/MathML/HTML for better efficiency. > > Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:69 > > + const auto& block = toRenderElement(toRenderElement(firstChild())->firstChild()); > > are we always guaranteed to have a grandchild for this element? > > > Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:70 > > + m_hasChildElement = childrenOfType<RenderElement>(*block).first(); > > is this asking if there's a great-grandchild of this object? Yes, I don't like these two. See the discussion in comment 19 and comment 23.

(In reply to comment #25) > (In reply to comment #24) > > (From update of attachment 226717 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=226717&action=review > > > Source/WebCore/mathml/MathMLTextElement.cpp:99 > > > + auto& svgElement = toSVGElement(element); > > > > it doesn't seem necessary to convert to an SVG element. > > I think this could be written as > > > > if (element.isSVG) > > return element.hasTagName(svgTag); > > > > ditto for MathMLElement above > > > > Perhaps I misunderstood what Darin said in comment 19, but I thought that after bug 130090 we want to cast to SVG/MathML/HTML for better efficiency. Ok, we'll have to wait for Darin's patch to land for the gains to be noticed I suppose. > > > > Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:69 > > > + const auto& block = toRenderElement(toRenderElement(firstChild())->firstChild()); > > > > are we always guaranteed to have a grandchild for this element? > > > > > Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:70 > > > + m_hasChildElement = childrenOfType<RenderElement>(*block).first(); > > > > is this asking if there's a great-grandchild of this object? > > Yes, I don't like these two. See the discussion in comment 19 and comment 23. Ok thanks

(In reply to comment #26) Thanks for the review, Chris. @Darin: can you double check the patch? Especially the comments above about the cast and how to determine m_hasChildElement?

Comment on attachment 226717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226717&action=review > Source/WebCore/mathml/MathMLTextElement.cpp:91 > + auto& element = toElement(node); There’s no need to have this local variable element. Checking isXXXElement is just as efficient directly on a node. But also no harm, so lets leave it like this. >>>> Source/WebCore/mathml/MathMLTextElement.cpp:99 >>>> + auto& svgElement = toSVGElement(element); >>> >>> it doesn't seem necessary to convert to an SVG element. >>> I think this could be written as >>> >>> if (element.isSVG) >>> return element.hasTagName(svgTag); >>> >>> ditto for MathMLElement above >> >> Perhaps I misunderstood what Darin said in comment 19, but I thought that after bug 130090 we want to cast to SVG/MathML/HTML for better efficiency. > > Ok, we'll have to wait for Darin's patch to land for the gains to be noticed I suppose. That’s right. The efficiency gain will happen when I get my patch in, which should be in a few days or weeks. Without the cast, each call to hasTagName will redo the isSVGElement test. It’s possible the compiler will be smart enough to optimize all those checks out, but I think it’s better not to rely on it. >> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:70 >> + m_hasChildElement = false; >> + if (!isEmpty()) { >> + const auto& block = toRenderElement(toRenderElement(firstChild())->firstChild()); >> + m_hasChildElement = childrenOfType<RenderElement>(*block).first(); > > is this asking if there's a great-grandchild of this object? I agree that the render tree version of "has child element" is sloppy, depending on exactly how many elements we should ignore. And I think it’s really unfortunate that we don’t know exactly why there are two levels of RenderElement here. And also that this mysterious code doesn’t have any comments. But it’s still better than the DOM code, since the DOM code will definitely fail tests where CSS makes a DOM node not render. There should be a comment explaining why we dig down two levels here, even if that comment has to admit that one of the two levels has a cause not known to you at the time you write the comment. I think the data member should not be named m_hasChildElement since that’s not mechanically true. RenderMathMLToken will always have a child that is a RenderElement, so having m_hasChildElement false is confusing. We need to be more precise to avoid confusing future programmers. Event "m_containsElement" would be better as long as we comment on what “contains” means.

Committed r165702: <http://trac.webkit.org/changeset/165702>

Comment on attachment 226717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226717&action=review > Source/WebCore/mathml/MathMLTextElement.cpp:78 > + ASSERT(hasTagName(MathMLNames::miTag) || hasTagName(MathMLNames::mnTag) || hasTagName(MathMLNames::msTag) || hasTagName(MathMLNames::mtextTag)); Looks to me like this assertion is wrong, or mathtags.in is wrong. annotationTag is missing, and msTag won’t be encountered here. This is causing regression tests to crash.

Comment on attachment 226717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226717&action=review >> Source/WebCore/mathml/MathMLTextElement.cpp:78 >> + ASSERT(hasTagName(MathMLNames::miTag) || hasTagName(MathMLNames::mnTag) || hasTagName(MathMLNames::msTag) || hasTagName(MathMLNames::mtextTag)); > > Looks to me like this assertion is wrong, or mathtags.in is wrong. annotationTag is missing, and msTag won’t be encountered here. This is causing regression tests to crash. Not sure what to do short of rolling out the patch.

(In reply to comment #31) > (From update of attachment 226717 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=226717&action=review > > >> Source/WebCore/mathml/MathMLTextElement.cpp:78 > >> + ASSERT(hasTagName(MathMLNames::miTag) || hasTagName(MathMLNames::mnTag) || hasTagName(MathMLNames::msTag) || hasTagName(MathMLNames::mtextTag)); > > > > Looks to me like this assertion is wrong, or mathtags.in is wrong. annotationTag is missing, and msTag won’t be encountered here. This is causing regression tests to crash. > > Not sure what to do short of rolling out the patch. The msTag is on purpose. <ms> is not implemented yet, but will be allowed as an insertion point and so should create the RenderMathMLToken. I don't think that will cause an assertion. The missing <annotation> is a mistake. The element can only contain text but is not a token element (that's why I didn't think about it). So that one should continue to return MathMLElement::createElementRenderer(std::move(style))

Created attachment 226883 [details] Fix <annotation>

Comment on attachment 226883 [details] Fix <annotation> View in context: https://bugs.webkit.org/attachment.cgi?id=226883&action=review Patch looks fine, but I don’t think our test coverage is sufficient. There are multiple changes here that I believe are untested. > Source/WebCore/ChangeLog:8 > + No new tests. No point in saying "no new tests". We should delete this line if we have nothing to say about tests. If there is a good reason we don’t need tests for this patch even though the patch fixes a bug, then please give that reason in the change log.

(In reply to comment #34) > (From update of attachment 226883 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=226883&action=review > > Patch looks fine, but I don’t think our test coverage is sufficient. There are multiple changes here that I believe are untested. I think only the changes to <annotation> were not tested by the initial patch (even if other tests detected the wrong assert). I'll add tests later, but feel free to take the patch now if the ASSERT crashes are currently problematic.

This change caused assertion failures: <http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r165703%20(3374)/results.html>. Frédéric, will you have a chance to look into this soon?

(In reply to comment #36) > This change caused assertion failures: <http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r165703%20(3374)/results.html>. > > Frédéric, will you have a chance to look into this soon? We can commit attachment 226883 [details] and that should fix the assert. I'll open a follow-up bugs to add some tests as suggested by Darin.

> We can commit attachment 226883 [details] and that should fix the assert. Please do. Commit queue won't land it, because the bug is marked resolved/fixed.

Committed r165739: <http://trac.webkit.org/changeset/165739>

There is still a crash occurring in the mathml/wbr-in-mroot-crash.html test since the original commit: http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r165745%20(16869)/results.html

Filed https://bugs.webkit.org/show_bug.cgi?id=130353.