Bug 42894 - RenderLayer crashes on page with MathML
Summary: RenderLayer crashes on page with MathML
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.5
: P4 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-23 06:46 PDT by Alex Milowski
Modified: 2010-08-26 16:59 PDT (History)
9 users (show)

See Also:


Attachments
Patch to fix crash (2.85 KB, patch)
2010-07-23 06:46 PDT, Alex Milowski
mitz: review-
Details | Formatted Diff | Diff
Revised patch (23.31 KB, patch)
2010-08-03 04:17 PDT, François Sausset
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Milowski 2010-07-23 06:46:29 PDT
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.
Comment 1 Kenneth Rohde Christiansen 2010-07-23 07:59:17 PDT
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.
Comment 2 Kenneth Rohde Christiansen 2010-07-23 07:59:48 PDT
Hyatt, can you have a look? Is the actualRenderer != 0 assumption wrong?
Comment 3 mitz 2010-07-23 08:01:11 PDT
Why isn't there a regression test?
Comment 4 Alex Milowski 2010-07-23 08:22:37 PDT
(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;
}
Comment 5 Alex Milowski 2010-07-23 08:24:04 PDT
(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.
Comment 6 Alex Milowski 2010-07-23 15:17:15 PDT
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.
Comment 7 Alex Milowski 2010-07-23 15:27:16 PDT
Removing the use of setStyle() in createRenderer() also fixes the crash.
Comment 8 François Sausset 2010-08-03 04:17:16 PDT
Created attachment 63320 [details]
Revised patch

New patch removing unnecessary calls to setStyle() plus a regression test (from Alex).
Comment 9 WebKit Commit Bot 2010-08-04 12:57:09 PDT
Comment on attachment 63320 [details]
Revised patch

Clearing flags on attachment: 63320

Committed r64668: <http://trac.webkit.org/changeset/64668>
Comment 10 WebKit Commit Bot 2010-08-04 12:57:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Adam Roben (:aroben) 2010-08-04 13:01:42 PDT
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.
Comment 12 WebKit Review Bot 2010-08-04 13:24:16 PDT
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
Comment 13 Alex Milowski 2010-08-05 07:00:57 PDT
(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.
Comment 14 Eric Seidel (no email) 2010-08-05 09:16:25 PDT
(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. :)