WebCore: Bug 31574 - Crashing bug when removing <ruby> element
authorrolandsteiner@chromium.org <rolandsteiner@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 19 Nov 2009 03:15:49 +0000 (03:15 +0000)
committerrolandsteiner@chromium.org <rolandsteiner@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 19 Nov 2009 03:15:49 +0000 (03:15 +0000)
(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.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@51169 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/ruby/ruby-remove-expected.txt [new file with mode: 0644]
LayoutTests/fast/ruby/ruby-remove.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/rendering/RenderRubyRun.cpp
WebCore/rendering/RenderRubyRun.h

index 9543ac5..11d170c 100644 (file)
@@ -1,3 +1,16 @@
+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>
 
         Reviewed by Darin Adler.
diff --git a/LayoutTests/fast/ruby/ruby-remove-expected.txt b/LayoutTests/fast/ruby/ruby-remove-expected.txt
new file mode 100644 (file)
index 0000000..ab69dcf
--- /dev/null
@@ -0,0 +1 @@
+SUCCESS!
diff --git a/LayoutTests/fast/ruby/ruby-remove.html b/LayoutTests/fast/ruby/ruby-remove.html
new file mode 100644 (file)
index 0000000..b834583
--- /dev/null
@@ -0,0 +1,18 @@
+<html>
+<head>
+<script>
+function test()
+{
+    if (window.layoutTestController)
+        layoutTestController.dumpAsText();
+    var ruby = document.getElementById("t");
+    ruby.innerHTML = ''; // This line mustn't crash 
+    document.getElementById("result").firstChild.data = 'SUCCESS!';
+}
+</script>
+</head>
+<body onload="test()">
+<div id="t"><ruby>f</div>
+<div id="result">FAILED!</div>
+</body>
+</html>
index c8a8576..0c3a147 100644 (file)
@@ -1,3 +1,32 @@
+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>
 
         Reviewed by Kenneth Rohde Christiansen.
index 561178b..8578c55 100644 (file)
@@ -41,6 +41,7 @@ namespace WebCore {
 
 RenderRubyRun::RenderRubyRun(Node* node)
     : RenderBlock(node)
+    , m_beingDestroyed(false)
 {
     setReplaced(true);
     setInline(true);
@@ -50,6 +51,13 @@ RenderRubyRun::~RenderRubyRun()
 {
 }
 
+void RenderRubyRun::destroy()
+{
+    // Mark if the run is being destroyed to avoid trouble in removeChild().
+    m_beingDestroyed = true;
+    RenderBlock::destroy();
+}
+
 bool RenderRubyRun::hasRubyText() const
 {
     // The only place where a ruby text can be is in the first position
@@ -155,7 +163,7 @@ void RenderRubyRun::removeChild(RenderObject* child)
 {
     // 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();
         if (base && rightNeighbour && rightNeighbour->isRubyRun()) {
@@ -169,7 +177,7 @@ void RenderRubyRun::removeChild(RenderObject* child)
 
     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();
         if (base && !base->firstChild()) {
@@ -178,7 +186,7 @@ void RenderRubyRun::removeChild(RenderObject* child)
             base->destroy();
         }
 
-        // 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);
             deleteLineBoxTree();
index 52ee72c..361dfe5 100644 (file)
@@ -46,6 +46,8 @@ public:
     RenderRubyRun(Node*);
     virtual ~RenderRubyRun();
 
+    virtual void destroy();
+
     virtual const char* renderName() const { return "RenderRubyRun (anonymous)"; }
 
     virtual bool isRubyRun() const { return true; }
@@ -68,6 +70,9 @@ public:
 
 protected:
     RenderRubyBase* createRubyBase() const;
+    
+private:
+    bool m_beingDestroyed;
 };
 
 } // namespace WebCore