Accept HTML and MathML namespaces as valid requiredExtensions

[Frédéric Wang (:fredw)]

The SVGTiny12 spec suggests support for HTML and MathML languages in <foreignObject>. It also shows the use of "http://www.w3.org/1999/xhtml" and "http://www.w3.org/1998/Math/MathML" as requiredExtensions strings:

http://www.w3.org/TR/SVGTiny12/extend.html#ForeignObjectExamples

Firefox has supported these languages in SVG and recognized these requiredExtensions values since at least 2008/2009:

https://bugzilla.mozilla.org/show_bug.cgi?id=449746

The inclusion of foreign object is useful to mix diagrams and formulas and has been discussed e.g. in MathJax mailing lists.

Here are reftests from the MathJax test suite which are currently failing in Chrome/Safari:

http://devel.mathjax.org/testing/testsuite/Configuration/Contexts/svg-1a.html
http://devel.mathjax.org/testing/testsuite/Configuration/Contexts/svg-1-ref.html

http://devel.mathjax.org/testing/testsuite/Configuration/Contexts/svg-1b.html
http://devel.mathjax.org/testing/testsuite/Configuration/Contexts/svg-1-ref.html

See also
http://www.w3.org/Math/Documents/Notes/graphics.xml#mathml-in-svg-guidelines

[Frédéric Wang (:fredw)]

I might try to work on this, since that's similar to my work for bug 120058.

SVGSwitchElement::childShouldCreateRenderer contains a comment

// FIXME: This function does not do what the comment below implies it does.
// It will create a renderer for any valid SVG element children, not just the first one.

But it seems to me that what the code does is correct.

[Frédéric Wang (:fredw)]

Created attachment 211617 [details]
testcase

WebKit should render the square root of n using MathML, but it renders the <text> fallback instead.

[Philip Rogers]

The current MathML implementation has inherent security flaws that will likely prevent it from being shipped in a major browser (see https://code.google.com/p/chromium/issues/detail?id=152430#c40). Are any browsers currently shipping with WebKit's MathML enabled?

[Philip Rogers]

(In reply to comment #3)
> The current MathML implementation has inherent security flaws that will likely prevent it from being shipped in a major browser (see https://code.google.com/p/chromium/issues/detail?id=152430#c40). Are any browsers currently shipping with WebKit's MathML enabled?

I was mistaken--Safari ships the WebKit MathML implementation. Please ignore my noise :)

[Frédéric Wang (:fredw)]

I don't think "inherent security flaws" is quite correct. To my knowledge, it's mainly a design issue with the MathML implementation violating some assumptions from other parts of the code. At least nobody (even Google engineers) has been able to provide a test case demonstrating security problem and the only thing I found was a performance issue that could make the browser hangs a few seconds ; something which I think is categorized as the lowest security level by Google.

Anyway the solution for this bug is obvious: just uses the #ifdef preprocessing rules to add/remove the MathML namespace from authorized requiredExtensions values. If the patch is imported to Chromium, the #ifdef code can just be dropped.

[Frédéric Wang (:fredw)]

Created attachment 220437 [details]
Patch V1

[chris fleizach]

Comment on attachment 220437 [details]
Patch V1

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

> Source/WebCore/svg/SVGTests.cpp:128
> +    unsigned extensionsSize = m_requiredExtensions.value.size();

size() returns a size_t

> Source/WebCore/svg/SVGTests.cpp:131
> +        if (value.isEmpty() || !hasExtension(value))

I don't think you need to check if the value is empty, since hasExtension will return false for an empty string

[Frédéric Wang (:fredw)]

(In reply to comment #7)
> > Source/WebCore/svg/SVGTests.cpp:128
> > +    unsigned extensionsSize = m_requiredExtensions.value.size();
> 
> size() returns a size_t
> 
> > Source/WebCore/svg/SVGTests.cpp:131
> > +        if (value.isEmpty() || !hasExtension(value))
> 
> I don't think you need to check if the value is empty, since hasExtension will return false for an empty string

Thanks for the feedback. I think you are right for the isEmpty. Note that I copied the existing code in SVGTests::isValid() for features and systemLanguage. Should I apply the same changes to these other attributes?

[chris fleizach]

(In reply to comment #8)
> (In reply to comment #7)
> > > Source/WebCore/svg/SVGTests.cpp:128
> > > +    unsigned extensionsSize = m_requiredExtensions.value.size();
> > 
> > size() returns a size_t
> > 
> > > Source/WebCore/svg/SVGTests.cpp:131
> > > +        if (value.isEmpty() || !hasExtension(value))
> > 
> > I don't think you need to check if the value is empty, since hasExtension will return false for an empty string
> 
> Thanks for the feedback. I think you are right for the isEmpty. Note that I copied the existing code in SVGTests::isValid() for features and systemLanguage. Should I apply the same changes to these other attributes?

Sure

[Frédéric Wang (:fredw)]

Created attachment 220438 [details]
Patch V2

So as I read it, the isEmpty is still necessary for the features attribute.

[Dirk Schulze]

Comment on attachment 220438 [details]
Patch V2

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

Patch looks good in general. r- because of a missing test for SVGDOM.

> Source/WebCore/svg/SVGSwitchElement.cpp:55
> +    // We create a renderer for the first valid SVG element child.
> +    // FIXME: The renderer must be updated after dynamic change of the requiredFeatures, requiredExtensions and systemLanguage attributes (http://wkbug/122297).

Yes, it looks like this is correct. The link is not valid. Even if it would, it references something different (MathML related). Please open a bug with a test case (if you haven't done so yet) and use the link to this bug instead.

> Source/WebCore/svg/SVGTests.cpp:102
> +bool SVGTests::hasExtension(const String& extension) const

This is used by SVGDOM as well. However, you do not have one JS test to check the correct behavior. Could you add those?

> Source/WebCore/svg/SVGTests.cpp:104
> +    // We recognize XHTML and MathML, as implemented in Gecko and suggested in the SVG Tiny recommendation (http://www.w3.org/TR/SVGTiny12/struct.html#RequiredExtensionsAttribute).

We do not implement SVG 1.2 Tiny. But the normative text between SVG 1.1 and SVG 1.2 Tiny is basically the same. Just use http://www.w3.org/TR/SVG11/struct.html#RequiredExtensionsAttribute

> Source/WebCore/svg/SVGTests.cpp:109
> +#if ENABLE(MATHML)
> +    return extension == HTMLNames::xhtmlNamespaceURI || extension == MathMLNames::mathmlNamespaceURI;
> +#else
> +    return extension == HTMLNames::xhtmlNamespaceURI;
> +#endif

I wonder, even though if it would be foolish of an author to do that, if we shouldn't check for the SVG namespace as well.

> Source/WebCore/svg/SVGTests.cpp:117
>          if (value.isEmpty() || !DOMImplementation::hasFeature(value, String()))

To answer your question: value can be empty. However, it is not necessary to have the check though. It could be an optimization on the other hand. Just keep it.

> Source/WebCore/svg/SVGTests.cpp:129
> +    for (unsigned i = 0; i < extensionsSize; ++i) {

Well, this is inconsistent. If you change the other to size_t, this should be size_t as well. (Ditto on the other for-loops.) However, I am not sure if I agree with Chris. We always used to use unsigned.

[Frédéric Wang (:fredw)]

(In reply to comment #11)
> > Source/WebCore/svg/SVGTests.cpp:104
> We do not implement SVG 1.2 Tiny. But the normative text between SVG 1.1 and SVG 1.2 Tiny is basically the same. Just use http://www.w3.org/TR/SVG11/struct.html#RequiredExtensionsAttribute

The SVG11 version does not mention the XHTML/MathML examples, so it would not really relevant to point to that page...

> I wonder, even though if it would be foolish of an author to do that, if we shouldn't check for the SVG namespace as well.

I just checked the HTML5 validator schema and the allowed direct children are html, body, math and other HTML5 "inner flow" elements:

https://bitbucket.org/validator/validator/src/5ee4172d2929787d5b78519c2035e62b503eee6c/schema/xhtml5-svg-mathml.rnc?at=default#cl-17

SVG could still be inserted indirectly e.g. by using an intermediate HTML element... but it would be stupid for the author to ask the renderer if it supports SVG (otherwise why would it even be trying to understand the <switch>/<foreignObject> features or rendering the SVG document).

Well, I guess it is easy to add but I'm not really sure it is important... Gecko does not AFAIK.

[Frédéric Wang (:fredw)]

(In reply to comment #11)
> > Source/WebCore/svg/SVGTests.cpp:102
> > +bool SVGTests::hasExtension(const String& extension) const
> 
> This is used by SVGDOM as well. However, you do not have one JS test to check the correct behavior. Could you add those?
> 

I'm not sure I understand this. SVG DOM has

http://www.w3.org/TR/SVG/svgdom.html#FeatureStrings

for hasFeature but I don't see hasExtension. Perhaps you mean testing a private method but calling document.implementation.hasExtension returns undefined for me.

[Dirk Schulze]

(In reply to comment #12)
> (In reply to comment #11)
> > > Source/WebCore/svg/SVGTests.cpp:104
> > We do not implement SVG 1.2 Tiny. But the normative text between SVG 1.1 and SVG 1.2 Tiny is basically the same. Just use http://www.w3.org/TR/SVG11/struct.html#RequiredExtensionsAttribute
> 
> The SVG11 version does not mention the XHTML/MathML examples, so it would not really relevant to point to that page...

True, but it also doesn't matter since these are examples for URIs. In general, if there is a SVG1.1 version, we use the 1.1 version so that people don't get confused.

> 
> > I wonder, even though if it would be foolish of an author to do that, if we shouldn't check for the SVG namespace as well.
> 
> I just checked the HTML5 validator schema and the allowed direct children are html, body, math and other HTML5 "inner flow" elements:
> 
> https://bitbucket.org/validator/validator/src/5ee4172d2929787d5b78519c2035e62b503eee6c/schema/xhtml5-svg-mathml.rnc?at=default#cl-17
> 
> SVG could still be inserted indirectly e.g. by using an intermediate HTML element... but it would be stupid for the author to ask the renderer if it supports SVG (otherwise why would it even be trying to understand the <switch>/<foreignObject> features or rendering the SVG document).
> 
> Well, I guess it is easy to add but I'm not really sure it is important... Gecko does not AFAIK.

I assume the text could be interpreted as extension to SVG, in which case the SVG URI would not be part of it. Keep it as it is then.

[Dirk Schulze]

(In reply to comment #13)
> (In reply to comment #11)
> > > Source/WebCore/svg/SVGTests.cpp:102
> > > +bool SVGTests::hasExtension(const String& extension) const
> > 
> > This is used by SVGDOM as well. However, you do not have one JS test to check the correct behavior. Could you add those?
> > 
> 
> I'm not sure I understand this. SVG DOM has
> 
> http://www.w3.org/TR/SVG/svgdom.html#FeatureStrings
> 
> for hasFeature but I don't see hasExtension. Perhaps you mean testing a private method but calling document.implementation.hasExtension returns undefined for me.

Look at "Basic Data Types" and the interface for SVGTests: http://www.w3.org/TR/SVG11/types.html#InterfaceSVGTests

This has
   boolean hasExtension(in DOMString extension);

[Frédéric Wang (:fredw)]

Created attachment 220717 [details]
Patch V3

[Dirk Schulze]

Comment on attachment 220717 [details]
Patch V3

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

Patch looks great, just tiny change requests and the patch is good to go.

> LayoutTests/ChangeLog:5
> +

Please add a detailed description for Changelogs

> Source/WebCore/ChangeLog:12
> +        When HTML and MathML are used as foreign objects of an SVG image, it is important for Web authors to be able to specify a fallback content for SVG-only readers or browsers without MathML support. We rely on the requiredExtensions for that purpose and we use the XHTML/MathML namespaces as suggested in SVG Tiny 1.2 and implemented in Gecko.

The description comes right after "Reviewed by..." Please use line breaks.

> Source/WebCore/svg/SVGSwitchElement.cpp:55
> +    // FIXME: The renderer must be updated after dynamic change of the requiredFeatures, requiredExtensions and systemLanguage attributes (http://wkbug/74749).

Please change that to http://wkb.ug/74749

[Frédéric Wang (:fredw)]

Created attachment 220828 [details]
Patch (Final Version)

[Dirk Schulze]

Comment on attachment 220828 [details]
Patch (Final Version)

r=me

[WebKit Commit Bot]

Comment on attachment 220828 [details]
Patch (Final Version)

Clearing flags on attachment: 220828

Committed r161629: <http://trac.webkit.org/changeset/161629>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

svg/custom/conditional-processing-2.html is failing on Mac bots:

http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=svg%2Fcustom%2Fconditional-processing-2.html

[Alexey Proskuryakov]

Looks like EWS didn't catch that because it allows "flaky" tests, and this test appears to only fail when run for the first time, not when retried.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 126762

[Alexey Proskuryakov]

Rolling out. Sample diff: http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r161644%20(14912)/svg/custom/conditional-processing-2-diff.txt

I'm not sure what's going on with the tools here. WebKit2 bots seem to say that results are missing, and then that results are different on retry. But despite tools shortcomings, it seems clear that the test fails in some way, just not correctly reported.

[Alexey Proskuryakov]

More here: http://build.webkit.org/results/Apple%20Mavericks%20Release%20WK1%20(Tests)/r161647%20(2244)/results.html

[Dirk Schulze]

(In reply to comment #26)
> More here: http://build.webkit.org/results/Apple%20Mavericks%20Release%20WK1%20(Tests)/r161647%20(2244)/results.html

It might be better to change the painting tests to reference tests instead of DRT tests.
The JS tests can be pure js tests.

[Frédéric Wang (:fredw)]

Created attachment 220927 [details]
Patch with reftests

[Build Bot]

Comment on attachment 220927 [details]
Patch with reftests

Attachment 220927 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4757409243332608

New failing tests:
svg/custom/conditional-processing-2.html
fast/canvas/webgl/uniform-samplers-test.html

[Build Bot]

Created attachment 220929 [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

[Frédéric Wang (:fredw)]

Created attachment 220931 [details]
Patch with reftests

[Darin Adler]

Comment on attachment 220931 [details]
Patch with reftests

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

> Source/WebCore/svg/SVGSwitchElement.cpp:55
> +    // FIXME: The renderer must be updated after dynamic change of the requiredFeatures, requiredExtensions and systemLanguage attributes (http://wkb.ug/74749).

I don’t think it’s appropriate to use wkb.ug URLs in the WebKit source code. It’s neat how short they are, but we don’t want to use them here for the same rason we don’t use them in the change log.

> Source/WebCore/svg/SVGTests.cpp:32
> +#if ENABLE(MATHML)
> +#include "MathMLNames.h"
> +#endif

Conditional includes need to go in their own paragraph, rather than attempting to sort them in with the unconditional includes.

We should add this to the coding style guide; I looked and it’s not mentioned there!

> Source/WebCore/svg/SVGTests.cpp:133
> -    if (!m_requiredExtensions.value.isEmpty())
> -        return false;
> +    unsigned extensionsSize = m_requiredExtensions.value.size();
> +    for (unsigned i = 0; i < extensionsSize; ++i) {
> +        String value = m_requiredExtensions.value.at(i);
> +        if (!hasExtension(value))
> +            return false;
> +    }

This should be written like this:

    for (auto& extension : m_requiredExtensions.value) {
        if (!hasExtension(extension))
            return false;
    }

Lets not do the old style for loops in new code unless there is a reason.

[Frédéric Wang (:fredw)]

Created attachment 220972 [details]
Patch

[Frédéric Wang (:fredw)]

Can someone please commit the patch I uploaded last Sunday?

[WebKit Commit Bot]

Comment on attachment 220972 [details]
Patch

Rejecting attachment 220972 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 220972, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/6254836169310208

[WebKit Commit Bot]

Comment on attachment 220972 [details]
Patch

Clearing flags on attachment: 220972

Committed r162083: <http://trac.webkit.org/changeset/162083>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.