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 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.
Hyatt, can you have a look? Is the actualRenderer != 0 assumption wrong?
Why isn't there a regression test?
(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; }
(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.
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.
Removing the use of setStyle() in createRenderer() also fixes the crash.
Created attachment 63320 [details] Revised patch New patch removing unnecessary calls to setStyle() plus a regression test (from Alex).
Comment on attachment 63320 [details] Revised patch Clearing flags on attachment: 63320 Committed r64668: <http://trac.webkit.org/changeset/64668>
All reviewed patches have been landed. Closing bug.
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.
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
(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.
(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. :)