1190646 - "ASSERTION: Stretching should be in the vertical direction"

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Stretching should be in the vertical direction: 'isVertical', file layout/mathml/nsMathMLChar.cpp, line 1131

The assertion is part of code added in https://hg.mozilla.org/mozilla-central/rev/7d09c6ea9ae5 (patch for bug 407059).

Stir DOM found this by modifying https://dxr.mozilla.org/mozilla-central/source/layout/reftests/mathml/stretchy-largeop-1.html (test for bug 1092053).

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[James Kitchener (:jkitch)]

The code later checks if largeops have reached a sufficient size by checking the vertical direction only
https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/layout/mathml/nsMathMLChar.cpp#l1240

so at first glance simply forcing isVertical to true for largeops is enough.

I'll have a more detailed look to confirm the correctness of that thought.

Comment 3

[James Kitchener (:jkitch)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf38f8aaf238

Comment 4

[James Kitchener (:jkitch)]

Attachment: patch

The rest of the function defines the minimum size of all largeops as a height (not a problem as horizontally stretching laregops do not exist), so the result of this failed assertion is that largeops are not stretched to the appropriate size.

There are two options.  
1.  Change the callers so largeops are detected and are always passed vertical directions for stretching.
2.  Ignore the direction passed to the function and always treat largeops as vertical.

As there are no horizontally stretching largeops, option 2 seems the simpler choice.  The former is complicated by multiple code paths (with/without inferred mrows) and nesting.

Comment 5

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

I think the point of the assertion is really to verify that largeops are stretched in vertical direction, so it would be better to ensure the stretching is called with the right direction.

So I propose to check that all largeop operators in layout/mathml/mathfont.properties also have direction:vertical. I think it is enough to fix the bug?

Also, can you please open a bug for the XXX comment in

http://hg.mozilla.org/mozilla-central/file/3e51753a099f/layout/mathml/nsMathMLmoFrame.cpp#l455

and reference it in the source code ; with a warning about this stretch direction requirement?

Comment 6

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

@Jesse: do you use these tests in your fuzzing tools:

http://hg.mozilla.org/mozilla-central/file/3e51753a099f/layout/mathml/tests/stretchy-and-large-operators.html
https://github.com/fred-wang/MathFonts#testcase

Comment 7

[Jesse Ruderman]

No, I only use reftests/crashtests as starting points for DOMFuzz. I suppose I could add other static-ish test suites, or even try my luck with the DOM at the end of each mochitest.

Comment 8

[James Kitchener (:jkitch)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3eb073773698

Comment 9

[James Kitchener (:jkitch)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c85352423ea8

Comment 10

[James Kitchener (:jkitch)]

Attachment: patch

All largeops are considered vertical in the operator dictionary.

A testcase covering the affected operators has been added to detect the assertion and ensure that it doesn't cause unintended stretching.

Comment 11

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

Comment on attachment 8651766 [details] [diff] [review]
patch

Review of attachment 8651766 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 12

[James Kitchener (:jkitch)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=29082f3a5e47

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/64f4fe2fb89b

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/64f4fe2fb89b