Bug 31574 – Crashing bug when removing <ruby> element

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Roland Steiner]

Found by Shinichiro Hamaji.

The renderer crashes if a <ruby> tag is being deleted via JavaScript. The example given was:

<div id="t"><ruby>f</div>
<script>t.innerHTML = '';</script>

[Roland Steiner]

Created attachment 43341 [details]
patch - fix crash

Cause of the bug:

  1.) RenderBlock::destroy() of the RenderRubyRun called destroyLeftoverChildren()
  2.) that called destroy() of the RenderRubyBase(), which in RenderObject::destroy() calls remove()
  3.) remove() is being redirected as parent()->removeChild() in RenderObject.h
  4.) this triggers the special handling of child removal in RenderRubyRun that causes it to destroy itself
  5.) On returning from all this the renderer crashes when accessing a member
      or virtual function on this now illegal object.

I therefore added a flag that tracks if the ruby run is being destroyed. If so, avoid doing the special handling in removeChild that caused this. It's not the most elegant solution, but the easiest to implement without touching unrelated code. Also, it's self-documenting.

[Roland Steiner]

The bug may also be triggered by the code that handles invalid HTML markup rather than just JavaScript.

Updated to P1 as it's a crashing bug.

[Eric Seidel (no email)]

Comment on attachment 43341 [details]
patch - fix crash

I'm not well enough versed with the ruby code to understand why the ruby classes need this special memory management.  How does RenderRubyRun relate to RenderInline for instance?  Does RenderInline use a similar m_beingDestroyed method?

I'm missing enough context to understand why this fix is the right fix.

[Darin Adler]

Comment on attachment 43341 [details]
patch - fix crash

I agree that there are probably more elegant fixes. But this should do, at least for now.

r=me

[Roland Steiner]

committed in r51169

[Eric Seidel (no email)]

There are several tools which can auto-close the bugs for you.

bugzilla-tool land-diff is one of them. :)
mark-bug-fixed is another.

[Roland Steiner]

(In reply to comment #6)

Thanks, good to know! I assumed that bugs needed verification that they are indeed fixed.