Bug 162933 – REGRESSION(r203289): Assertion in MathOperator::stretchTo() on Wikipedia Page

• Component: MathML
• Status: RESOLVED FIXED

[Brent Fulgham]

Visit the Wikipedia page for Levenshtein Distance in a debug build of WebKit:

    https://en.wikipedia.org/wiki/Levenshtein_distance

This triggers the following assertion:

    void MathOperator::stretchTo(const RenderStyle& style, LayoutUnit targetSize)
    {
        ASSERT(m_operatorType == Type::VerticalOperator || m_operatorType == Type::HorizontalOperator);
        calculateStretchyData(style, false, targetSize);
        if (m_stretchType == StretchType::GlyphAssembly) {

The assertion fires because we are attempting to apply the 'stretchTo' operation on a "NormalOperator" type.

[Brent Fulgham]

<rdar://problem/28570590>

[Frédéric Wang Nélar]

Are you able to isolate the MathML fragment that triggers the assertion?

As I see, RenderMathMLOperator::updateMathOperator() sets the type to NormalOperator if !shouldAllowStretching() i.e. we don't have !(textContent() && (hasOperatorFlag(MathMLOperatorDictionary::Stretchy) || isLargeOperatorInDisplayStyle()))

On the other hand, MathOperator::stretchTo are called from the RenderMathMLOperator::stretchTo functions that explicitly checks hasOperatorFlag(MathMLOperatorDictionary::Stretchy). Is is possible that textContent() is empty? Or that RenderMathMLOperator::updateMathOperator() has not been called in time?

[Brent Fulgham]

(In reply to comment #2)
> Are you able to isolate the MathML fragment that triggers the assertion?

I don't have a reduced test case.

> ... Is is possible that
> textContent() is empty? Or that RenderMathMLOperator::updateMathOperator()
> has not been called in time?

'textContent' is not empty. The 'operatorChar' member is set to "{".

[Brent Fulgham]

(In reply to comment #3)

Wait! I take it all back. I was not hitting the crash when I inspected those values.

It appears that the RenderMathMLOperator object has an operatorChar of "", so yes -- it does seem to be empty.

[Frédéric Wang Nélar]

(In reply to comment #4)
> (In reply to comment #3)
> 
> Wait! I take it all back. I was not hitting the crash when I inspected those
> values.
> 
> It appears that the RenderMathMLOperator object has an operatorChar of "",
> so yes -- it does seem to be empty.

OK, in that case we only need to avoid the (two IIRC?) calls to RenderMathMLOperator::stretchTo when the textContent == 0 (and maybe add assert in RenderMathMLOperator::stretchTo too).

[Brent Fulgham]

Created attachment 293495 [details]
Test Case showing the null image

Reduced test case.

[Brent Fulgham]

The assertion gets hit with the attached test case, specifically with this line:

     <mo fence="true" stretchy="true"></mo>

Looks like we need to account for the possibility that content may be generated with an empty element of this kind.

[Frédéric Wang Nélar]

(In reply to comment #7)
> The assertion gets hit with the attached test case, specifically with this
> line:
> 
>      <mo fence="true" stretchy="true"></mo>
> 
> Looks like we need to account for the possibility that content may be
> generated with an empty element of this kind.

Yes, I think it will be considered vertical + stretchy. fence="true" should not be needed to trigger the assert.

[Frédéric Wang Nélar]

Created attachment 293537 [details]
Patch

@Brent: This is a quick untested patch. Maybe a stronger solution is just to introduce

bool RenderMathMLOperator::isStretchy()
{
  return textContent() && element().hasProperty(MathMLOperatorDictionary::Stretchy);
}

and then replace each call to RenderMathMLOperator::hasProperty(MathMLOperatorDictionary::Stretchy) in the RenderMathML* classes with a call to RenderMathMLOperator::isStretchy().

[Brent Fulgham]

(In reply to comment #9)
> Created attachment 293537 [details]
> Patch
> 
> @Brent: This is a quick untested patch. Maybe a stronger solution is just to
> introduce
> 
> bool RenderMathMLOperator::isStretchy()
> {
>   return textContent() &&
> element().hasProperty(MathMLOperatorDictionary::Stretchy);
> }
> 
> and then replace each call to
> RenderMathMLOperator::hasProperty(MathMLOperatorDictionary::Stretchy) in the
> RenderMathML* classes with a call to RenderMathMLOperator::isStretchy().

Could we just use "shouldAllowStretching()"? It is very similar:

bool RenderMathMLOperator::shouldAllowStretching() const
{
    return textContent() && (hasOperatorFlag(MathMLOperatorDictionary::Stretchy) || isLargeOperatorInDisplayStyle());
}

[Brent Fulgham]

I confirmed that Frédéric's patch works. We can generate a final patch once he confirms whether the existing shouldAllowStretching is suitable, or if we must use the new 'isStretchy()' method.

[Frédéric Wang Nélar]

(In reply to comment #10)
> Could we just use "shouldAllowStretching()"? It is very similar:
> 
> bool RenderMathMLOperator::shouldAllowStretching() const
> {
>     return textContent() &&
> (hasOperatorFlag(MathMLOperatorDictionary::Stretchy) ||
> isLargeOperatorInDisplayStyle());
> }

I don't think so, stretchTo must only be called with stretchy operators. If we include large operators in displaystyle (e.g. <math display="block"><mo>&Sum</mo></math>) then I suspect you can also hit an ASSERT in RenderMathMLOperator because hasOperatorFlag(MathMLOperatorDictionary::Stretchy) is not satisfied.

[Frédéric Wang Nélar]

(In reply to comment #10)
> Could we just use "shouldAllowStretching()"?

Incidentally, I suspect this method is just a legacy of old code that was kept after the refactoring. I believe that now we can just expand it inside RenderMathMLOperator::updateMathOperator() by doing something like:

if (isStretchy())
    type = isVertical() ? MathOperator::Type::VerticalOperator : MathOperator::Type::HorizontalOperator;
else if (textContent() && isLargeOperatorInDisplayStyle())
    type = MathOperator::Type::DisplayOperator;
else  
    type = MathOperator::Type::NormalOperator;

and then the condition in RenderMathMLOperator::useMathOperator() will just become

m_mathOperator.type() != MathOperator::Type::NormalOperator || textContent() == minusSign

where we add the accessor MathOperator::type() { return m_operatorType; }

We can even include the textContent() check inside isLargeOperatorInDisplayStyle and discard the case <math display="block"><mo largeop="true"></mo></math>.

[Brent Fulgham]

Created attachment 293573 [details]
Patch

[Brent Fulgham]

(In reply to comment #13)
> (In reply to comment #10)
> > Could we just use "shouldAllowStretching()"?
> 
> Incidentally, I suspect this method is just a legacy of old code that was
> kept after the refactoring. I believe that now we can just expand it inside
> RenderMathMLOperator::updateMathOperator() by doing something like:
> 
> if (isStretchy())
>     type = isVertical() ? MathOperator::Type::VerticalOperator :
> MathOperator::Type::HorizontalOperator;
> else if (textContent() && isLargeOperatorInDisplayStyle())
>     type = MathOperator::Type::DisplayOperator;
> else  
>     type = MathOperator::Type::NormalOperator;
> 
> and then the condition in RenderMathMLOperator::useMathOperator() will just
> become
> 
> m_mathOperator.type() != MathOperator::Type::NormalOperator || textContent()
> == minusSign
> 
> where we add the accessor MathOperator::type() { return m_operatorType; }
> 
> We can even include the textContent() check inside
> isLargeOperatorInDisplayStyle and discard the case <math display="block"><mo
> largeop="true"></mo></math>.

Seems like a nice idea for a future refactoring! :-)

[Frédéric Wang Nélar]

Comment on attachment 293573 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=293573&action=review

lgtm, but I'm not a reviewer.

> Source/WebCore/ChangeLog:11
> +        We shouldn't be trying to apply stretch operations on an empty MathML element. Create a

I think it's actually any operator that is not made of a single character. For example the assert probably happens with <mo stretchy="true">++<mo> too.

> LayoutTests/mathml/empty-mo.html:8
> +    <math xmlns="http://www.w3.org/1998/Math/MathML">

Maybe you can reduce this test case to <math><mo stretchy="true"></mo></math> ?

[Frédéric Wang Nélar]

(In reply to comment #15)
> Seems like a nice idea for a future refactoring! :-)

I opened bug 164313

[Alejandro G. Castro]

(In reply to comment #16)
> Comment on attachment 293573 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=293573&action=review
> 
> lgtm, but I'm not a reviewer.
>

lgtm too.

> > Source/WebCore/ChangeLog:11
> > +        We shouldn't be trying to apply stretch operations on an empty MathML element. Create a
> 
> I think it's actually any operator that is not made of a single character.
> For example the assert probably happens with <mo stretchy="true">++<mo> too.
> 

Can we add also this case (++) to the test, and check if the textContent() part of the condition is doing what we want for that case.

[Brent Fulgham]

Comment on attachment 293573 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=293573&action=review

>>> Source/WebCore/ChangeLog:11
>>> +        We shouldn't be trying to apply stretch operations on an empty MathML element. Create a
>> 
>> I think it's actually any operator that is not made of a single character. For example the assert probably happens with <mo stretchy="true">++<mo> too.
> 
> Can we add also this case (++) to the test, and check if the textContent() part of the condition is doing what we want for that case.

Done.

[Brent Fulgham]

Created attachment 293684 [details]
Patch

[Brent Fulgham]

(In reply to comment #16)
> Comment on attachment 293573 [details]

> Maybe you can reduce this test case to <math><mo
> stretchy="true"></mo></math> ?

Yes, this did work.

I've revised the test cases as you suggested.

[Brent Fulgham]

Committed r208296: <http://trac.webkit.org/changeset/208296>