Last Comment Bug 398038 - <mpadded height="20 10"> triggers "###!!! ASSERTION: Unexpected Pseudo Unit"
: <mpadded height="20 10"> triggers "###!!! ASSERTION: Unexpected Pseudo Unit"
Status: RESOLVED FIXED
[fixed by bug 677036]
:
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: x86 Mac OS X
: -- minor (vote)
: mozilla15
Assigned To: Frédéric Wang (:fredw)
:
: Anthony Jones (:kentuckyfriedtakahe, :k17e)
Mentors:
Depends on: 677036
Blocks: 344905 347580
  Show dependency treegraph
 
Reported: 2007-09-29 18:59 PDT by Jesse Ruderman
Modified: 2012-05-19 09:06 PDT (History)
10 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (146 bytes, application/xhtml+xml)
2007-09-29 18:59 PDT, Jesse Ruderman
no flags Details
fix (1.77 KB, patch)
2007-09-30 14:05 PDT, Vlad Sukhoy
rbs: review-
rbs: superreview-
Details | Diff | Splinter Review
v2 (1.78 KB, patch)
2007-09-30 15:56 PDT, Vlad Sukhoy
no flags Details | Diff | Splinter Review
Comment 7 by rbs as a patch (1.28 KB, patch)
2007-10-01 20:04 PDT, Vlad Sukhoy
no flags Details | Diff | Splinter Review
crashtest (980 bytes, patch)
2012-01-10 11:51 PST, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2007-09-29 18:59:12 PDT
Created attachment 282864 [details]
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 2007-09-30 14:05:27 PDT
Created attachment 282913 [details] [diff] [review]
fix

Take only the first of two distinct values.
Comment 2 rbs 2007-09-30 14:40:35 PDT
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 2007-09-30 15:56:01 PDT
Created attachment 282926 [details] [diff] [review]
v2

Rejects such attributes.
Comment 4 rbs 2007-09-30 17:02:13 PDT
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 2007-09-30 17:54:25 PDT
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 2007-09-30 18:29:02 PDT
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 2007-10-01 05:49:04 PDT
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 2007-10-01 16:46:12 PDT
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 2007-10-01 17:20:00 PDT
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 2007-10-01 20:04:30 PDT
Created attachment 283118 [details] [diff] [review]
Comment 7 by rbs as a patch

Attached as a patch.
Comment 11 Jonathan Hage 2011-07-22 01:16:47 PDT
The patch proposed still allows whitespaces after the sign
Comment 12 Jonathan Hage 2011-07-22 06:51:22 PDT
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 [:bz] (still a bit busy) 2011-07-22 07:10:10 PDT
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) 2011-07-22 07:21:53 PDT
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 [:bz] (still a bit busy) 2011-07-22 07:29:22 PDT
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) 2011-07-22 07:35:37 PDT
(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 [:bz] (still a bit busy) 2011-07-22 07:40:26 PDT
CSS allows whitespace between any two tokens, typically, so something like:

  height: 5px ;

is perfectly fine.
Comment 18 Frédéric Wang (:fredw) 2011-07-22 07:45:39 PDT
(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 [:bz] (still a bit busy) 2011-07-22 08:04:16 PDT
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) 2011-07-22 08:13:16 PDT
(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 [:bz] (still a bit busy) 2011-07-22 08:15:46 PDT
We parse those SVG attributes with the CSS parser.  So they allow leading and trailing whitespace.
Comment 22 Frédéric Wang (:fredw) 2011-07-22 08:59:18 PDT
(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) 2011-07-24 02:14:49 PDT
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) 2012-01-10 11:51:48 PST
Created attachment 587416 [details] [diff] [review]
crashtest

Here is a crashtest.

This bug should be fixed by patch of bug 677036.
Comment 25 Frédéric Wang (:fredw) 2012-05-15 07:57:55 PDT
https://tbpl.mozilla.org/?tree=Try&rev=9545b7e7c463
Comment 26 Ryan VanderMeulen [:RyanVM] 2012-05-15 15:34:25 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c243c96abdf
Comment 27 Ed Morley [:emorley] 2012-05-16 03:33:09 PDT
https://hg.mozilla.org/mozilla-central/rev/9c243c96abdf

Note You need to log in before you can comment on or make changes to this bug.