Changeset 51169 in webkit

Timestamp:

Nov 18, 2009 7:15:49 PM (14 years ago)

Author:

rolandsteiner@chromium.org

Message:

WebCore: Bug 31574 - Crashing bug when removing <ruby> element
(​https://bugs.webkit.org/show_bug.cgi?id=31574)

Reviewed by Darin Adler.

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.

Test: fast/ruby/ruby-remove.html

• rendering/RenderRubyRun.cpp:

(WebCore::RenderRubyRun::RenderRubyRun):
(WebCore::RenderRubyRun::destroy):
(WebCore::RenderRubyRun::removeChild):

• rendering/RenderRubyRun.h:

LayoutTests: Bug 31574 - Crashing bug when removing <ruby> element
(​https://bugs.webkit.org/show_bug.cgi?id=31574)

Reviewed by Darin Adler.

Layout test to verify it no longer crashes when the <ruby> element
is being removed.

• fast/ruby/ruby-remove-expected.txt: Added.
• fast/ruby/ruby-remove.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/ruby/ruby-remove-expected.txt (added)
• LayoutTests/fast/ruby/ruby-remove.html (added)
• WebCore/ChangeLog (modified) (1 diff)
• WebCore/rendering/RenderRubyRun.cpp (modified) (5 diffs)
• WebCore/rendering/RenderRubyRun.h (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2009-11-19  Roland Steiner  <rolandsteiner@chromium.org>
+
+        Reviewed by Darin Adler.
+
+        Bug 31574 -  Crashing bug when removing <ruby> element
+        (https://bugs.webkit.org/show_bug.cgi?id=31574)
+        
+        Layout test to verify it no longer crashes when the <ruby> element
+        is being removed.
+
+        * fast/ruby/ruby-remove-expected.txt: Added.
+        * fast/ruby/ruby-remove.html: Added.
+
 2009-11-18  Kent Tamura  <tkent@chromium.org>
 

trunk/WebCore/ChangeLog

@@ +1 @@
+2009-11-19  Roland Steiner  <rolandsteiner@chromium.org>
+
+        Reviewed by Darin Adler.
+
+        Bug 31574 -  Crashing bug when removing <ruby> element
+        (https://bugs.webkit.org/show_bug.cgi?id=31574)
+
+        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.
+
+        Test: fast/ruby/ruby-remove.html
+
+        * rendering/RenderRubyRun.cpp:
+        (WebCore::RenderRubyRun::RenderRubyRun):
+        (WebCore::RenderRubyRun::destroy):
+        (WebCore::RenderRubyRun::removeChild):
+        * rendering/RenderRubyRun.h:
+
 2009-11-18  Laszlo Gombos  <laszlo.1.gombos@nokia.com>
 

trunk/WebCore/rendering/RenderRubyRun.cpp

@@ -42 +42 @@
 RenderRubyRun::RenderRubyRun(Node* node)
     : RenderBlock(node)
+    , m_beingDestroyed(false)
 {
     setReplaced(true);
@@ -49 +50 @@
 RenderRubyRun::~RenderRubyRun()
 {
+}
+
+void RenderRubyRun::destroy()
+{
+    // Mark if the run is being destroyed to avoid trouble in removeChild().
+    m_beingDestroyed = true;
+    RenderBlock::destroy();
 }
 
@@ -156 +164 @@
     // If the child is a ruby text, then merge the ruby base with the base of
     // the right sibling run, if possible.
-    if (!documentBeingDestroyed() && child->isRubyText()) {
+    if (!m_beingDestroyed && !documentBeingDestroyed() && child->isRubyText()) {
         RenderRubyBase* base = rubyBase();
         RenderObject* rightNeighbour = nextSibling();
@@ -170 +178 @@
     RenderBlock::removeChild(child);
 
-    if (!documentBeingDestroyed()) {
+    if (!m_beingDestroyed && !documentBeingDestroyed()) {
         // Check if our base (if any) is now empty. If so, destroy it.
         RenderBlock* base = rubyBase();
@@ -179 +187 @@
         }
 
-        // If any of the above leaves the run empty, destroy it as well
+        // If any of the above leaves the run empty, destroy it as well.
         if (isEmpty()) {
             parent()->removeChild(this);

trunk/WebCore/rendering/RenderRubyRun.h

@@ -47 +47 @@
     virtual ~RenderRubyRun();
 
+    virtual void destroy();
+
     virtual const char* renderName() const { return "RenderRubyRun (anonymous)"; }
 
@@ -69 +71 @@
 protected:
     RenderRubyBase* createRubyBase() const;
+    
+private:
+    bool m_beingDestroyed;
 };