398038 - <mpadded height="20 10"> triggers "###!!! ASSERTION: Unexpected Pseudo Unit"

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Loading the testcase triggers:

###!!! ASSERTION: Unexpected Pseudo Unit: '0', file /Users/jruderman/trunk/mozilla/layout/mathml/base/src/nsMathMLmpaddedFrame.cpp, line 331

Comment 1

[Vlad Sukhoy]

Attachment: fix

Take only the first of two distinct values.

Comment 2

[rbs]

I will rather prefer that we reject/ignore such an ill-formed attribute (otherwise we are feeding into the kind of legacy/problems that tag-soup HTML did).

Comment 3

[Vlad Sukhoy]

Attachment: v2

Rejects such attributes.

Comment 4

[rbs]

Comment on attachment 282926 [details] [diff] [review]
v2

What about just strengthening the if-test with this one-liner:

     // see if the input was just a CSS value
     number.Append(unit); // leave the sign out if it was there
-    if (ParseNumericValue(number, aCSSValue))
+    if (ParseNumericValue(number, aCSSValue) &&
+        aCSSValue.IsLengthUnit())
       return PR_TRUE;

Would that do the trick?

Comment 5

[Vlad Sukhoy]

Comment on attachment 282926 [details] [diff] [review]
v2

Yup, I was not sure about shared state modification, but it seems ok as far as I can see.. But still <math:mpadded height="10 0000000000em"/> is kind of peculiar, but if it is a bug, it is a different one, I guess.

Comment 6

[rbs]

Let's try to also fix the peculiar thing while we are it. It probably comes from the fact that height="10 em" (that is a space before the unit) is supported (by ParseNumericValue), which is desired. We are better off strengthening it so that it doesn't support some unintended leftover digits before the unit. Speaking of this I suspect that the peculiar behavior may also be happening with fixed-point numbers too, height="10 1.2em"...

Comment 7

[rbs]

On further thought, the following could do the trick because we only have to bother checking |unit| since it is the sole culprit. Just make sure that it doesn't regress bug 148326.

+    // catch any ill-formed unit that disguises as a numeric value - bug 398038
+    if (nsCRT::IsAsciiDigit(unit[0]) || unit[0] == '.') {
+      aSign = NS_MATHML_SIGN_INVALID;
+      return PR_FALSE;
+    }
+
     // see if the input was just a CSS value
     number.Append(unit); // leave the sign out if it was there
     if (ParseNumericValue(number, aCSSValue))
       return PR_TRUE;

Comment 8

[Vlad Sukhoy]

The remaining case of interest I can think of after that is "10 %width" which is  within the spec it appears, so comment 4 and comment 7 should do for this bug and peculiarities.

Comment 9

[rbs]

Comment 7 is enough because |gotPercent| will catch the '%' case and the unit will start right after it:
====================
  // skip any space after the number
  if (i < stringLength && nsCRT::IsAsciiSpace(aString[i]))
    i++;

  // see if this is a percentage-based value
  if (i < stringLength && aString[i] == '%') {
    i++;
    gotPercent = PR_TRUE;

    // skip any space after the '%' sign
    if (i < stringLength && nsCRT::IsAsciiSpace(aString[i]))
      i++;
  }

  // the remainder now should be a css-unit, or a pseudo-unit, or a named-space
  aString.Right(unit, stringLength - i);
==================

Comment 10

[Vlad Sukhoy]

Attachment: Comment 7 by rbs as a patch

Attached as a patch.

Comment 11

[Jonathan Hage]

The patch proposed still allows whitespaces after the sign

Comment 12

[Jonathan Hage]

nsMathMLmpaddedFrame::ParseAttribute allows whitespace between the sign and the number and between the number and the unit. nsMathMLFrame::ParseNumericValue allows whitespaces only between the sign and the number. So the two behaviors are inconsistent.
For example, in the testcase the bug comes from the fact that nsMathMLmpaddedFrame::ParseAttribute concatenates the two numbers, then it calls nsMathMLFrame::ParseNumericValue, consequently nsMathMLFrame::ParseNumericValue accepts the attribute's value.

In my opinion the best solution is the way of nsMathMLFrame::ParseNumericValue (i.e. allowing spaces only at the beginning and end of the attribute value). Note that the MathML rec says: "spaces  are not allowed within these values, but implementers may wish to ignore such spaces to maximize  backward compatibility."

Maybe we should also unify the code of the two functions. Therefore this bug will depend on Bug 411227.
Perhaps we should add a flag allow_pseudo_unit to use in nsMathMLMmpadded and implement the parsing of pseudo unit directly in nsMathMLElement::ParseNumericValue.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Unless there are very strong compat reasons here and a well-defined processing model for the spaces, I propose just not allowing the spaces...

Comment 14

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

The MathML REC says:

"Note that the examples in the Version 2 of the MathML specification showed spaces within the attribute values, suggesting that this was the intended format. Formally, spaces are not allowed within these values, but implementers may wish to ignore such spaces to maximize backward compatibility. "

Except in these former examples, I don't know if this syntax is really used. Why do we allow spaces in nsMathMLElement::ParseNumericValue? Is it consistent with what CSS does?

Comment 15

[Boris Zbarsky [:bzbarsky]]

CSS does not allow spaces between the number and the unit (or between the sign and the number, for that matter).

Comment 16

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

(In reply to comment #15)
> CSS does not allow spaces between the number and the unit (or between the
> sign and the number, for that matter).

And what about the spaces at the beginning and the end of the attribute value?

Comment 17

[Boris Zbarsky [:bzbarsky]]

CSS allows whitespace between any two tokens, typically, so something like:

  height: 5px ;

is perfectly fine.

Comment 18

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

(In reply to comment #17)
> CSS allows whitespace between any two tokens, typically, so something like:
> 
>   height: 5px ;
> 
> is perfectly fine.

OK, I meant when the values are used in attributes, such that height=" 5px "?

Comment 19

[Boris Zbarsky [:bzbarsky]]

If you mean in HTML, then HTML attributes that set lengths don't take units; they're always pixels.  So height=" 5px " is the same as height=" 5 happy unicorns", and both are the same as height="5".

If you're still talking about CSS, then I'm not sure what you're asking...

Comment 20

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

(In reply to comment #19)
> If you mean in HTML, then HTML attributes that set lengths don't take units;
> they're always pixels.  So height=" 5px " is the same as height=" 5 happy
> unicorns", and both are the same as height="5".
> 
> If you're still talking about CSS, then I'm not sure what you're asking...

Sorry, I'm mixing up with all the syntaxes. But for example, I think SVG shares styling attributes with CSS, and some SVG elements can have height="5px".

Comment 21

[Boris Zbarsky [:bzbarsky]]

We parse those SVG attributes with the CSS parser.  So they allow leading and trailing whitespace.

Comment 22

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

(In reply to comment #21)
> We parse those SVG attributes with the CSS parser.  So they allow leading
> and trailing whitespace.

OK, so Jonathan's proposal in comment 12 (allowing only leading/trainling whitespaces) seems the best option to me. 

The XML spec says:

"If the attribute type is not CDATA, then the XML processor MUST further process the normalized attribute value by discarding any leading and trailing space (#x20) characters, and by replacing sequences of space (#x20) characters by a single space (#x20) character."

"All attributes for which no declaration has been read SHOULD be treated by a non-validating processor as if declared CDATA."

The (non normative) MathML DTD does declare attributes as NMTOKENS, so they are CDATA by default.

The MathML REC says that "There should be no space between the number and the unit of a length." for attribute length. For leading/trailing whitespace, this seems less strict: "Moreover, leading and trailing whitespace in attribute values should be avoided. ".

The backward compatibility comment on mpadded attribute, seems to be there only because the authors made a mistake in the spec examples.

Comment 23

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

We can unify the code and move the parsing of nsMathMLFrame::ParseNamedSpaceValue and nsMathMLmpaddedFrame::ParseAttribute into nsMathMLElement::ParseNumericValue.

For nsMathMLFrame::ParseNamedSpaceValue, I've opened bug 673759.

I think we need to modify the aFlags parameter of nsMathMLElement::ParseNumericValue to become a reference (or add more arguments to this function that will be references). Then we can get [out] parameters about the sign and pseudo unit, that are used in mpadded. We should also add a [in] parameter ALLOW_MPADDED_SYNTAX to indicate that we can use the extended syntax instead of simply a standard CSS value.

In nsMathMLmpaddedFrame::ParseAttribute, there is a XXX syntax asking why we don't look for an attribute on an mstyle ancestor when calling nsMathMLFrame::ParseNamedSpaceValue. We don't need to do that, since the right attribute value is assumed to have been passed by the caller nsMathMLmpaddedFrame::ProcessAttributes. This XXX comment will naturally be removed when we unify the code of the three functions. Actually, it is specified in nsMathMLmpaddedFrame::ProcessAttributes that we don't access mstyle. The REC comment can be updated now we support voffset:

"MathML specifies that when the attributes height, depth or width are specified on an mstyle element, they apply only to mspace elements, and not to the corresponding attributes of mglyph, mpadded, or mtable. [...] When the lspace attribute is set with mstyle, it applies only to the mo element and not to mpadded. To be consistent, the voffset attribute of the mpadded element can not be set on mstyle."

Comment 24

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

Attachment: crashtest

Here is a crashtest.

This bug should be fixed by patch of bug 677036.

Comment 25

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

https://tbpl.mozilla.org/?tree=Try&rev=9545b7e7c463

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c243c96abdf

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9c243c96abdf