Source/WebCore: Hold refptr to identified previous sibling within findPlaceForCounter.
authorcdn@chromium.org <cdn@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 4 Oct 2011 19:47:32 +0000 (19:47 +0000)
committercdn@chromium.org <cdn@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 4 Oct 2011 19:47:32 +0000 (19:47 +0000)
https://bugs.webkit.org/show_bug.cgi?id=68563

Reviewed by Adam Barth.

Test: fast/css/counters/counter-after-style-crash.html

* rendering/RenderCounter.cpp:
(WebCore::findPlaceForCounter):

LayoutTests: Add test for crash when performing rich text mutations with counter nodes.
https://bugs.webkit.org/show_bug.cgi?id=68563

Reviewed by Adam Barth.

* fast/css/counters/counter-after-style-crash-expected.txt: Added.
* fast/css/counters/counter-after-style-crash.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/css/counters/counter-after-style-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/css/counters/counter-after-style-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderCounter.cpp

index aae98b0..e030a3d 100644 (file)
@@ -1,3 +1,13 @@
+2011-09-21  Cris Neckar  <cdn@chromium.org>
+
+        Add test for crash when performing rich text mutations with counter nodes.
+        https://bugs.webkit.org/show_bug.cgi?id=68563
+
+        Reviewed by Adam Barth.
+
+        * fast/css/counters/counter-after-style-crash-expected.txt: Added.
+        * fast/css/counters/counter-after-style-crash.html: Added.
+
 2011-10-04  Joshua Bell  <jsbell@chromium.org>
 
         IndexedDB add() should fail if key is NaN
diff --git a/LayoutTests/fast/css/counters/counter-after-style-crash-expected.txt b/LayoutTests/fast/css/counters/counter-after-style-crash-expected.txt
new file mode 100644 (file)
index 0000000..5b6af73
--- /dev/null
@@ -0,0 +1 @@
+PASS: Counters updated successfully without crashing
diff --git a/LayoutTests/fast/css/counters/counter-after-style-crash.html b/LayoutTests/fast/css/counters/counter-after-style-crash.html
new file mode 100644 (file)
index 0000000..8a0af67
--- /dev/null
@@ -0,0 +1,32 @@
+<html>
+  <script>
+  if (window.layoutTestController) {
+    layoutTestController.waitUntilDone();
+    layoutTestController.dumpAsText();
+  }
+  </script>
+  <style>
+    div {
+      counter-reset:ctr
+    }
+
+    :after {
+      content:counter(x);
+      counter-increment:ctr;
+    }
+  </style>                          
+  <junk>TESTING..</div><div><div></div>
+  <span></span>
+  <table>
+    </script>
+    <script>
+      document.designMode='on';
+      document.execCommand('selectall');
+      document.execCommand('italic');
+      document.execCommand('removeformat');
+
+      document.body.innerHTML = "PASS: Counters updated successfully without crashing";
+      layoutTestController.notifyDone();
+    </script>
+  </table>
+</html>
\ No newline at end of file
index faedcd9..7e60c56 100644 (file)
@@ -1,3 +1,15 @@
+2011-09-21  Cris Neckar  <cdn@chromium.org>
+
+        Hold refptr to identified previous sibling within findPlaceForCounter.
+        https://bugs.webkit.org/show_bug.cgi?id=68563
+
+        Reviewed by Adam Barth.
+
+        Test: fast/css/counters/counter-after-style-crash.html
+
+        * rendering/RenderCounter.cpp:
+        (WebCore::findPlaceForCounter):
+
 2011-10-04  Joshua Bell  <jsbell@chromium.org>
 
         IndexedDB add() should fail if key is NaN
index 6217380..c30daf3 100644 (file)
@@ -303,13 +303,15 @@ static bool findPlaceForCounter(RenderObject* counterOwner, const AtomicString&
     // we are trying to find a place for. This is the next renderer to be checked.
     RenderObject* currentRenderer = previousInPreOrder(counterOwner);
     previousSibling = 0;
+    RefPtr<CounterNode> previousSiblingProtector = 0;
+
     while (currentRenderer) {
         CounterNode* currentCounter = makeCounterNode(currentRenderer, identifier, false);
         if (searchEndRenderer == currentRenderer) {
             // We may be at the end of our search.
             if (currentCounter) {
                 // We have a suitable counter on the EndSearchRenderer.
-                if (previousSibling) { // But we already found another counter that we come after.
+                if (previousSiblingProtector) { // But we already found another counter that we come after.
                     if (currentCounter->actsAsReset()) {
                         // We found a reset counter that is on a renderer that is a sibling of ours or a parent.
                         if (isReset && areRenderersElementsSiblings(currentRenderer, counterOwner)) {
@@ -326,16 +328,19 @@ static bool findPlaceForCounter(RenderObject* counterOwner, const AtomicString&
                         // In some cases renders can be reparented (ex. nodes inside a table but not in a column or row).
                         // In these cases the identified previousSibling will be invalid as its parent is different from
                         // our identified parent.
-                        if (previousSibling->parent() != currentCounter)
-                            previousSibling = 0;
+                        if (previousSiblingProtector->parent() != currentCounter)
+                            previousSiblingProtector = 0;
+
+                        previousSibling = previousSiblingProtector.get();
                         return true;
                     }
                     // CurrentCounter, the counter at the EndSearchRenderer, is not reset.
                     if (!isReset || !areRenderersElementsSiblings(currentRenderer, counterOwner)) {
                         // If the node we are placing is not reset or we have found a counter that is attached
                         // to an ancestor of the placed counter's owner renderer we know we are a sibling of that node.
-                        ASSERT(currentCounter->parent() == previousSibling->parent());
+                        ASSERT(currentCounter->parent() == previousSiblingProtector->parent());
                         parent = currentCounter->parent();
+                        previousSibling = previousSiblingProtector.get();
                         return true;
                     }
                 } else { 
@@ -350,6 +355,7 @@ static bool findPlaceForCounter(RenderObject* counterOwner, const AtomicString&
                             return parent;
                         }
                         parent = currentCounter;
+                        previousSibling = previousSiblingProtector.get();
                         return true;
                     }
                     if (!isReset || !areRenderersElementsSiblings(currentRenderer, counterOwner)) {
@@ -357,7 +363,7 @@ static bool findPlaceForCounter(RenderObject* counterOwner, const AtomicString&
                         previousSibling = currentCounter;
                         return true;
                     }
-                    previousSibling = currentCounter;
+                    previousSiblingProtector = currentCounter;
                 }
             }
             // We come here if the previous sibling or parent of our owner renderer had no
@@ -370,18 +376,18 @@ static bool findPlaceForCounter(RenderObject* counterOwner, const AtomicString&
             // counter being placed is attached to.
             if (currentCounter) {
                 // We found a suitable counter.
-                if (previousSibling) {
+                if (previousSiblingProtector) {
                     // Since we had a suitable previous counter before, we should only consider this one as our 
                     // previousSibling if it is a reset counter and hence the current previousSibling is its child.
                     if (currentCounter->actsAsReset()) {
-                        previousSibling = currentCounter;
+                        previousSiblingProtector = currentCounter;
                         // We are no longer interested in previous siblings of the currentRenderer or their children
                         // as counters they may have attached cannot be the previous sibling of the counter we are placing.
                         currentRenderer = parentElement(currentRenderer)->renderer();
                         continue;
                     }
                 } else
-                    previousSibling = currentCounter;
+                    previousSiblingProtector = currentCounter;
                 currentRenderer = previousSiblingOrParent(currentRenderer);
                 continue;
             }
@@ -390,7 +396,7 @@ static bool findPlaceForCounter(RenderObject* counterOwner, const AtomicString&
         // which may be done twice in some cases. Rearranging the decision points though, to accommodate this 
         // performance improvement would create more code duplication than is worthwhile in my oppinion and may further
         // impede the readability of this already complex algorithm.
-        if (previousSibling)
+        if (previousSiblingProtector)
             currentRenderer = previousSiblingOrParent(currentRenderer);
         else
             currentRenderer = previousInPreOrder(currentRenderer);