Closed Bug 398038 Opened 13 years ago Closed 8 years ago

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

Categories

(Core :: MathML, defect)

x86
macOS
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: jruderman, Assigned: fredw)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [fixed by bug 677036])

Attachments

(2 files, 3 obsolete files)

Attached file testcase
Loading the testcase triggers:

###!!! ASSERTION: Unexpected Pseudo Unit: '0', file /Users/jruderman/trunk/mozilla/layout/mathml/base/src/nsMathMLmpaddedFrame.cpp, line 331
Attached patch fix (obsolete) — Splinter Review
Take only the first of two distinct values.
Attachment #282913 - Flags: superreview?(rbs)
Attachment #282913 - Flags: review?(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).
Attachment #282913 - Flags: superreview?(rbs)
Attachment #282913 - Flags: superreview-
Attachment #282913 - Flags: review?(rbs)
Attachment #282913 - Flags: review-
Attached patch v2 (obsolete) — Splinter Review
Rejects such attributes.
Attachment #282913 - Attachment is obsolete: true
Attachment #282926 - Flags: superreview?(rbs)
Attachment #282926 - Flags: review?(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 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.
Attachment #282926 - Attachment is obsolete: true
Attachment #282926 - Flags: superreview?(rbs)
Attachment #282926 - Flags: review?(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"...
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;
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 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);
==================
Attached patch Comment 7 by rbs as a patch (obsolete) — Splinter Review
Attached as a patch.
QA Contact: ian → mathml
Assignee: rbs → nobody
Keywords: helpwanted
Assignee: nobody → hage.jonathan
Status: NEW → ASSIGNED
The patch proposed still allows whitespaces after the sign
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.
Unless there are very strong compat reasons here and a well-defined processing model for the spaces, I propose just not allowing the spaces...
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?
CSS does not allow spaces between the number and the unit (or between the sign and the number, for that matter).
(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?
CSS allows whitespace between any two tokens, typically, so something like:

  height: 5px ;

is perfectly fine.
(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 "?
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...
(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".
We parse those SVG attributes with the CSS parser.  So they allow leading and trailing whitespace.
(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.
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."
Depends on: 677036
Assignee: hage.jonathan → fred.wang
Attached patch crashtestSplinter Review
Here is a crashtest.

This bug should be fixed by patch of bug 677036.
Attachment #283118 - Attachment is obsolete: true
Attachment #587416 - Flags: review?(karlt)
Attachment #587416 - Flags: review?(karlt) → review+
https://tbpl.mozilla.org/?tree=Try&rev=9545b7e7c463
Whiteboard: [please push with the patch from bug 677036]
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c243c96abdf
Flags: in-testsuite+
Whiteboard: [please push with the patch from bug 677036] → [fixed by bug 677036]
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/9c243c96abdf
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
No longer depends on: 677036
Depends on: 677036
You need to log in before you can comment on or make changes to this bug.