Crash in visiblePositionForIndex
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 17 Feb 2012 01:43:50 +0000 (01:43 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 17 Feb 2012 01:43:50 +0000 (01:43 +0000)
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.

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

LayoutTests/ChangeLog
LayoutTests/editing/execCommand/applyblockelement-visiblepositionforindex-crash-expected.txt [new file with mode: 0644]
LayoutTests/editing/execCommand/applyblockelement-visiblepositionforindex-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/editing/ApplyBlockElementCommand.cpp
Source/WebCore/editing/InsertListCommand.cpp
Source/WebCore/editing/htmlediting.cpp
Source/WebCore/editing/htmlediting.h

index 05a4a9b..e31ecbd 100644 (file)
@@ -1,3 +1,15 @@
+2012-02-16  Ryosuke Niwa  <rniwa@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  <danakj@chromium.org>
 
         [chromium] Empty divs not transforming overflow correctly
diff --git a/LayoutTests/editing/execCommand/applyblockelement-visiblepositionforindex-crash-expected.txt b/LayoutTests/editing/execCommand/applyblockelement-visiblepositionforindex-crash-expected.txt
new file mode 100644 (file)
index 0000000..2afa0bf
--- /dev/null
@@ -0,0 +1 @@
+PASS. WebKit didn't crash.
diff --git a/LayoutTests/editing/execCommand/applyblockelement-visiblepositionforindex-crash.html b/LayoutTests/editing/execCommand/applyblockelement-visiblepositionforindex-crash.html
new file mode 100644 (file)
index 0000000..bbc0e77
--- /dev/null
@@ -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>
index ab61080..e604513 100644 (file)
@@ -1,3 +1,23 @@
+2012-02-16  Ryosuke Niwa  <rniwa@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  <mdelaney@apple.com>
 
         ShadowBlur.cpp's cached content matching needs to consider m_layerSize changes
index a7c0a78..b32641b 100644 (file)
@@ -80,10 +80,10 @@ void ApplyBlockElementCommand::doApply()
     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 @@ void ApplyBlockElementCommand::doApply()
     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()));
     }
index dbc22ba..e4e3578 100644 (file)
@@ -152,11 +152,11 @@ void InsertListCommand::doApply()
                 // 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());
index b0432cf..0704610 100644 (file)
@@ -1072,31 +1072,24 @@ VisibleSelection selectionForParagraphIteration(const VisibleSelection& original
 // 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);
 }
 
index 0f2d1bd..abb49e3 100644 (file)
@@ -180,7 +180,7 @@ bool lineBreakExistsAtVisiblePosition(const VisiblePosition&);
     
 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);
 
 // -------------------------------------------------------------------------