Title: [108009] trunk
Revision
108009
Author
rn...@webkit.org
Date
2012-02-16 17:43:50 -0800 (Thu, 16 Feb 2012)

Log Message

Crash in visiblePositionForIndex
https://bugs.webkit.org/show_bug.cgi?id=77683

Reviewed by Eric Seidel.

Source/WebCore: 

Fixed the crash.

Test: editing/execCommand/applyblockelement-visiblepositionforindex-crash.html

* editing/ApplyBlockElementCommand.cpp:
(WebCore::ApplyBlockElementCommand::doApply):
* editing/InsertListCommand.cpp:
(WebCore::InsertListCommand::doApply):
* editing/htmlediting.cpp:
(WebCore::indexForVisiblePosition):
* editing/htmlediting.h:
(WebCore):

LayoutTests: 

Add a regression test. It crashes Webkit with very high frequency.

* editing/execCommand/applyblockelement-visiblepositionforindex-crash-expected.txt: Added.
* editing/execCommand/applyblockelement-visiblepositionforindex-crash.html: Added.

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (108008 => 108009)


--- trunk/LayoutTests/ChangeLog	2012-02-17 01:40:33 UTC (rev 108008)
+++ trunk/LayoutTests/ChangeLog	2012-02-17 01:43:50 UTC (rev 108009)
@@ -1,3 +1,15 @@
+2012-02-16  Ryosuke Niwa  <rn...@webkit.org>
+
+        Crash in visiblePositionForIndex
+        https://bugs.webkit.org/show_bug.cgi?id=77683
+
+        Reviewed by Eric Seidel.
+
+        Add a regression test. It crashes Webkit with very high frequency.
+
+        * editing/execCommand/applyblockelement-visiblepositionforindex-crash-expected.txt: Added.
+        * editing/execCommand/applyblockelement-visiblepositionforindex-crash.html: Added.
+
 2012-02-16  Dana Jansens  <dan...@chromium.org>
 
         [chromium] Empty divs not transforming overflow correctly

Added: trunk/LayoutTests/editing/execCommand/applyblockelement-visiblepositionforindex-crash-expected.txt (0 => 108009)


--- trunk/LayoutTests/editing/execCommand/applyblockelement-visiblepositionforindex-crash-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/editing/execCommand/applyblockelement-visiblepositionforindex-crash-expected.txt	2012-02-17 01:43:50 UTC (rev 108009)
@@ -0,0 +1 @@
+PASS. WebKit didn't crash.

Added: trunk/LayoutTests/editing/execCommand/applyblockelement-visiblepositionforindex-crash.html (0 => 108009)


--- trunk/LayoutTests/editing/execCommand/applyblockelement-visiblepositionforindex-crash.html	                        (rev 0)
+++ trunk/LayoutTests/editing/execCommand/applyblockelement-visiblepositionforindex-crash.html	2012-02-17 01:43:50 UTC (rev 108009)
@@ -0,0 +1,24 @@
+<script>
+
+if (window.layoutTestController)
+    layoutTestController.dumpAsText();
+
+function runTest() {
+    window.getSelection().setBaseAndExtent(start, 0, null, 0);
+    document.execCommand("Indent");
+    document.body.innerHTML = "PASS. WebKit didn't crash.";
+}
+</script>
+<body _onload_="runTest();">
+  <defs contenteditable="true" id="start">
+  <rt id="rt">A
+
+<script>
+document.write("text");
+try {
+    elem = document.getElementById("rt");
+    var new_elem = document.createElement("ruby");
+    new_elem.innerHTML = elem.innerHTML;
+    elem.parentNode.insertBefore(new_elem, elem);
+} catch (e) {}
+</script>

Modified: trunk/Source/WebCore/ChangeLog (108008 => 108009)


--- trunk/Source/WebCore/ChangeLog	2012-02-17 01:40:33 UTC (rev 108008)
+++ trunk/Source/WebCore/ChangeLog	2012-02-17 01:43:50 UTC (rev 108009)
@@ -1,3 +1,23 @@
+2012-02-16  Ryosuke Niwa  <rn...@webkit.org>
+
+        Crash in visiblePositionForIndex
+        https://bugs.webkit.org/show_bug.cgi?id=77683
+
+        Reviewed by Eric Seidel.
+
+        Fixed the crash.
+
+        Test: editing/execCommand/applyblockelement-visiblepositionforindex-crash.html
+
+        * editing/ApplyBlockElementCommand.cpp:
+        (WebCore::ApplyBlockElementCommand::doApply):
+        * editing/InsertListCommand.cpp:
+        (WebCore::InsertListCommand::doApply):
+        * editing/htmlediting.cpp:
+        (WebCore::indexForVisiblePosition):
+        * editing/htmlediting.h:
+        (WebCore):
+
 2012-02-16  Matthew Delaney  <mdela...@apple.com>
 
         ShadowBlur.cpp's cached content matching needs to consider m_layerSize changes

Modified: trunk/Source/WebCore/editing/ApplyBlockElementCommand.cpp (108008 => 108009)


--- trunk/Source/WebCore/editing/ApplyBlockElementCommand.cpp	2012-02-17 01:40:33 UTC (rev 108008)
+++ trunk/Source/WebCore/editing/ApplyBlockElementCommand.cpp	2012-02-17 01:43:50 UTC (rev 108009)
@@ -80,10 +80,10 @@
     VisiblePosition endOfSelection = selection.visibleEnd();
     ASSERT(!startOfSelection.isNull());
     ASSERT(!endOfSelection.isNull());
-    Element* startScope = 0;
-    int startIndex = indexForVisiblePosition(startOfSelection, &startScope);
-    Element* endScope = 0;
-    int endIndex = indexForVisiblePosition(endOfSelection, &endScope);
+    RefPtr<Element> startScope;
+    int startIndex = indexForVisiblePosition(startOfSelection, startScope);
+    RefPtr<Element> endScope;
+    int endIndex = indexForVisiblePosition(endOfSelection, endScope);
 
     formatSelection(startOfSelection, endOfSelection);
 
@@ -93,8 +93,8 @@
     ASSERT(startIndex >= 0);
     ASSERT(startIndex <= endIndex);
     if (startScope == endScope && startIndex >= 0 && startIndex <= endIndex) {
-        VisiblePosition start(visiblePositionForIndex(startIndex, startScope));
-        VisiblePosition end(visiblePositionForIndex(endIndex, endScope));
+        VisiblePosition start(visiblePositionForIndex(startIndex, startScope.get()));
+        VisiblePosition end(visiblePositionForIndex(endIndex, endScope.get()));
         if (start.isNotNull() && end.isNotNull())
             setEndingSelection(VisibleSelection(start, end, endingSelection().isDirectional()));
     }

Modified: trunk/Source/WebCore/editing/InsertListCommand.cpp (108008 => 108009)


--- trunk/Source/WebCore/editing/InsertListCommand.cpp	2012-02-17 01:40:33 UTC (rev 108008)
+++ trunk/Source/WebCore/editing/InsertListCommand.cpp	2012-02-17 01:43:50 UTC (rev 108009)
@@ -152,11 +152,11 @@
                 // FIXME: This is an inefficient way to keep selection alive because indexForVisiblePosition walks from
                 // the beginning of the document to the endOfSelection everytime this code is executed.
                 // But not using index is hard because there are so many ways we can lose selection inside doApplyForSingleParagraph.
-                Element* scope = 0;
-                int indexForEndOfSelection = indexForVisiblePosition(endOfSelection, &scope);
+                RefPtr<Element> scope;
+                int indexForEndOfSelection = indexForVisiblePosition(endOfSelection, scope);
                 doApplyForSingleParagraph(forceCreateList, listTag, currentSelection.get());
                 if (endOfSelection.isNull() || endOfSelection.isOrphan() || startOfLastParagraph.isNull() || startOfLastParagraph.isOrphan()) {
-                    endOfSelection = visiblePositionForIndex(indexForEndOfSelection, scope);
+                    endOfSelection = visiblePositionForIndex(indexForEndOfSelection, scope.get());
                     // If endOfSelection is null, then some contents have been deleted from the document.
                     // This should never happen and if it did, exit early immediately because we've lost the loop invariant.
                     ASSERT(endOfSelection.isNotNull());

Modified: trunk/Source/WebCore/editing/htmlediting.cpp (108008 => 108009)


--- trunk/Source/WebCore/editing/htmlediting.cpp	2012-02-17 01:40:33 UTC (rev 108008)
+++ trunk/Source/WebCore/editing/htmlediting.cpp	2012-02-17 01:43:50 UTC (rev 108009)
@@ -1072,31 +1072,24 @@
 // opertion is unreliable. TextIterator's TextIteratorEmitsCharactersBetweenAllVisiblePositions mode needs to be fixed, 
 // or these functions need to be changed to iterate using actual VisiblePositions.
 // FIXME: Deploy these functions everywhere that TextIterators are used to convert between VisiblePositions and indices.
-int indexForVisiblePosition(const VisiblePosition& visiblePosition, Element **scope)
+int indexForVisiblePosition(const VisiblePosition& visiblePosition, RefPtr<Element>& scope)
 {
     if (visiblePosition.isNull())
         return 0;
-        
+
     Position p(visiblePosition.deepEquivalent());
     Document* document = p.anchorNode()->document();
-    
-    Element* root;
     Node* shadowRoot = p.anchorNode()->shadowTreeRootNode();
-    
+
     if (shadowRoot) {
         // Use the shadow root for form elements, since TextIterators will not enter shadow content.
         ASSERT(shadowRoot->isElementNode());
-        root = static_cast<Element*>(shadowRoot);
+        scope = static_cast<Element*>(shadowRoot);
     } else
-        root = document->documentElement();
-    
-    if (scope) {
-        ASSERT(!*scope);
-        *scope = root;
-    }
-    
-    RefPtr<Range> range = Range::create(document, firstPositionInNode(root), p.parentAnchoredEquivalent());
-    
+        scope = document->documentElement();
+
+    RefPtr<Range> range = Range::create(document, firstPositionInNode(scope.get()), p.parentAnchoredEquivalent());
+
     return TextIterator::rangeLength(range.get(), true);
 }
 

Modified: trunk/Source/WebCore/editing/htmlediting.h (108008 => 108009)


--- trunk/Source/WebCore/editing/htmlediting.h	2012-02-17 01:40:33 UTC (rev 108008)
+++ trunk/Source/WebCore/editing/htmlediting.h	2012-02-17 01:43:50 UTC (rev 108009)
@@ -180,7 +180,7 @@
     
 int comparePositions(const VisiblePosition&, const VisiblePosition&);
 
-int indexForVisiblePosition(const VisiblePosition&, Element **scope);
+int indexForVisiblePosition(const VisiblePosition&, RefPtr<Element>& scope);
 VisiblePosition visiblePositionForIndex(int index, Element *scope);
 
 // -------------------------------------------------------------------------
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-changes

Reply via email to