The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla15

Status

()

Core
MathML
--
minor
RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: fredw)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla15
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed by bug 677036])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

10 years ago
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

10 years ago
Created attachment 282913 [details] [diff] [review]
fix

Take only the first of two distinct values.
Attachment #282913 - Flags: superreview?(rbs)
Attachment #282913 - Flags: review?(rbs)

Comment 2

10 years ago
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).

Updated

10 years ago
Attachment #282913 - Flags: superreview?(rbs)
Attachment #282913 - Flags: superreview-
Attachment #282913 - Flags: review?(rbs)
Attachment #282913 - Flags: review-

Comment 3

10 years ago
Created attachment 282926 [details] [diff] [review]
v2

Rejects such attributes.
Attachment #282913 - Attachment is obsolete: true
Attachment #282926 - Flags: superreview?(rbs)
Attachment #282926 - Flags: review?(rbs)

Comment 4

10 years ago
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

10 years ago
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)

Comment 6

10 years ago
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

10 years ago
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

10 years ago
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

10 years ago
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

10 years ago
Created attachment 283118 [details] [diff] [review]
Comment 7 by rbs as a patch

Attached as a patch.
QA Contact: ian → mathml
(Assignee)

Updated

7 years ago
Assignee: rbs → nobody
(Assignee)

Updated

6 years ago
Keywords: helpwanted
(Assignee)

Updated

6 years ago
Assignee: nobody → hage.jonathan
Status: NEW → ASSIGNED

Comment 11

6 years ago
The patch proposed still allows whitespaces after the sign

Comment 12

6 years ago
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...
(Assignee)

Comment 14

6 years ago
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).
(Assignee)

Comment 16

6 years ago
(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.
(Assignee)

Comment 18

6 years ago
(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...
(Assignee)

Comment 20

6 years ago
(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.
(Assignee)

Comment 22

6 years ago
(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.
(Assignee)

Comment 23

6 years ago
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."
(Assignee)

Updated

6 years ago
Depends on: 677036
(Assignee)

Updated

5 years ago
Assignee: hage.jonathan → fred.wang
(Assignee)

Comment 24

5 years ago
Created attachment 587416 [details] [diff] [review]
crashtest

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+
(Assignee)

Comment 25

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=9545b7e7c463
Keywords: helpwanted → checkin-needed
Whiteboard: [please push with the patch from bug 677036]
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c243c96abdf
Flags: in-testsuite+
Keywords: assertion, checkin-needed, testcase
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
No longer depends on: 677036
(Assignee)

Updated

5 years ago
Depends on: 677036
You need to log in before you can comment on or make changes to this bug.