RenderLayer crashes on page with MathML

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alex Milowski]

Created attachment 62423 [details]
Patch to fix crash

The code in RenderLayer makes some assumptions about the rendering tree that turn out to not be true.  As such, it does get a render object back in certain methods and causes an exception and crash due to a zero pointer.

An example page that has this issue is:

   http://golem.ph.utexas.edu/wiki/instiki/show/Sandbox

The attached patch fixes the crash.

[Kenneth Rohde Christiansen]

Comment on attachment 62423 [details]
Patch to fix crash

The patch seems OK, but why wasn't this needed before? I mean, maybe the code is correct assuming that we will have an actualRenderer and the bug is elsewhere. Would be nice if Hyatt could have a look.

[Kenneth Rohde Christiansen]

Hyatt, can you have a look? Is the actualRenderer != 0 assumption wrong?

Why isn't there a regression test?

[Alex Milowski]

(In reply to comment #1)
> (From update of attachment 62423 [details])
> The patch seems OK, but why wasn't this needed before? I mean, maybe the code is correct assuming that we will have an actualRenderer and the bug is elsewhere. Would be nice if Hyatt could have a look.

It is unclear why this is happening except that the referenced page is much more complex than our tests.  This must be coming from the combination of the styles used on the page in conjunction with the MathML rendering.  

The backtrace has this happening at the time that setStyle() is called the newly created RenderMathMLMath instance in the createRenderer() call on the MathMLMathElement.   When the setStyle() is called, it causes a style difference to be calculated and that is when things go wrong.

That code looks like:

RenderObject* MathMLMathElement::createRenderer(RenderArena* arena, RenderStyle* style)
{
    RenderMathMLMath* renderer = new (arena) RenderMathMLMath(this);
    renderer->setStyle(style);
    return renderer;
}

[Alex Milowski]

(In reply to comment #3)
> Why isn't there a regression test?

Good point but very hard to narrow down from that page.  I'll see what I can do to create a test case.

[Alex Milowski]

The layer is only created when someone has a CSS rule like:

math {
    overflow: auto;
}

We don't have that in any of our tests but that website does.  That's why we
haven't seen this before.

Oddly, the MathML code is the only code that calls setStyle() in createRenderer() and that is why this problem is localized to the MathML implementation.

[Alex Milowski]

Removing the use of setStyle() in createRenderer() also fixes the crash.

[François Sausset]

Created attachment 63320 [details]
Revised patch

New patch removing unnecessary calls to setStyle() plus a regression test (from Alex).

[WebKit Commit Bot]

Comment on attachment 63320 [details]
Revised patch

Clearing flags on attachment: 63320

Committed r64668: <http://trac.webkit.org/changeset/64668>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Roben (:aroben)]

Seems like the regression test could have used dumpAsText in order to have cross-platform results. There's no need for a whole render tree dump when you just want to find out if you crashed or not.

[WebKit Review Bot]

http://trac.webkit.org/changeset/64668 might have broken GTK Linux 64-bit Debug
The following changes are on the blame list:
http://trac.webkit.org/changeset/64667
http://trac.webkit.org/changeset/64668
http://trac.webkit.org/changeset/64669

[Alex Milowski]

(In reply to comment #11)
> Seems like the regression test could have used dumpAsText in order to have cross-platform results. There's no need for a whole render tree dump when you just want to find out if you crashed or not.

I expect to expand the use of user styles directly on MathML 'math' element as this test case demonstrates.  As such, having a pixel dump is a good thing.

Otherwise, yes, I agree.

[Eric Seidel (no email)]

(In reply to comment #13)
> (In reply to comment #11)
> > Seems like the regression test could have used dumpAsText in order to have cross-platform results. There's no need for a whole render tree dump when you just want to find out if you crashed or not.
> 
> I expect to expand the use of user styles directly on MathML 'math' element as this test case demonstrates.  As such, having a pixel dump is a good thing.
> 
> Otherwise, yes, I agree.

Generally each test has its own purpose.  Are you suggesting you're going to modify this test later on and thus need pixel test results?

I can't think of any reason for a crashing test to ever need pixel results, unless its crashing painting code. :)