2011-06-30 Abhishek Arya <inferno@chromium.org>
authorinferno@chromium.org <inferno@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 30 Jun 2011 17:00:48 +0000 (17:00 +0000)
committerinferno@chromium.org <inferno@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 30 Jun 2011 17:00:48 +0000 (17:00 +0000)
        Reviewed by Ryosuke Niwa.

        Crash when calling DOMSubtreeModified event when extracting range
        contents.
        https://bugs.webkit.org/show_bug.cgi?id=63650

        Convert a few nodes to RefPtrs and add commonRoot verification checks
        for Range::processContents.

        Tests: fast/dom/Range/range-extract-contents-event-fire-crash.html
               fast/dom/Range/range-extract-contents-event-fire-crash2.html

        * dom/Range.cpp:
        (WebCore::childOfCommonRootBeforeOffset):
        (WebCore::Range::processContents):
        (WebCore::Range::processContentsBetweenOffsets):
        (WebCore::Range::processAncestorsAndTheirSiblings):
2011-06-29  Abhishek Arya  <inferno@chromium.org>

        Reviewed by Ryosuke Niwa.

        Crash when calling DOMSubtreeModified event when extracting range
        contents.
        https://bugs.webkit.org/show_bug.cgi?id=63650

        * fast/dom/Range/range-extract-contents-event-fire-crash-expected.txt: Added.
        * fast/dom/Range/range-extract-contents-event-fire-crash.html: Added.
        * fast/dom/Range/range-extract-contents-event-fire-crash2-expected.txt: Added.
        * fast/dom/Range/range-extract-contents-event-fire-crash2.html: Added.
        * fast/dom/Range/range-extractContents.html: remove the appending of fragment
        in this crasher test since we now refptr the nodes and leftContents will be visible.
        This crasher test does not need to show the extractContents fragment.

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

LayoutTests/ChangeLog
LayoutTests/fast/dom/Range/range-extract-contents-event-fire-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/Range/range-extract-contents-event-fire-crash.html [new file with mode: 0644]
LayoutTests/fast/dom/Range/range-extract-contents-event-fire-crash2-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/Range/range-extract-contents-event-fire-crash2.html [new file with mode: 0644]
LayoutTests/fast/dom/Range/range-extractContents.html
Source/WebCore/ChangeLog
Source/WebCore/dom/Range.cpp

index 63e4471..3503f0c 100644 (file)
@@ -1,3 +1,19 @@
+2011-06-29  Abhishek Arya  <inferno@chromium.org>
+
+        Reviewed by Ryosuke Niwa.
+
+        Crash when calling DOMSubtreeModified event when extracting range
+        contents.
+        https://bugs.webkit.org/show_bug.cgi?id=63650
+
+        * fast/dom/Range/range-extract-contents-event-fire-crash-expected.txt: Added.
+        * fast/dom/Range/range-extract-contents-event-fire-crash.html: Added.
+        * fast/dom/Range/range-extract-contents-event-fire-crash2-expected.txt: Added.
+        * fast/dom/Range/range-extract-contents-event-fire-crash2.html: Added.
+        * fast/dom/Range/range-extractContents.html: remove the appending of fragment
+        in this crasher test since we now refptr the nodes and leftContents will be visible.
+        This crasher test does not need to show the extractContents fragment.
+
 2011-06-30  Nate Chapin  <japhet@chromium.org>
 
         Unreviewed, chromium test expectations update after r90101.
diff --git a/LayoutTests/fast/dom/Range/range-extract-contents-event-fire-crash-expected.txt b/LayoutTests/fast/dom/Range/range-extract-contents-event-fire-crash-expected.txt
new file mode 100644 (file)
index 0000000..f528f9c
--- /dev/null
@@ -0,0 +1 @@
+PASS: does not crash
diff --git a/LayoutTests/fast/dom/Range/range-extract-contents-event-fire-crash.html b/LayoutTests/fast/dom/Range/range-extract-contents-event-fire-crash.html
new file mode 100644 (file)
index 0000000..3ffb575
--- /dev/null
@@ -0,0 +1,32 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>
+    <span>
+        <span id="start"></span>
+    </span>
+</p>
+<p>
+    <span>
+        <span id="end"></span>
+    </span>
+</p>
+<script>
+if (window.layoutTestController)
+    layoutTestController.dumpAsText();
+
+function runTest()
+{
+    document.removeEventListener("DOMSubtreeModified", runTest, false);
+    document.body.innerHTML = 'PASS: does not crash';
+}
+
+document.addEventListener("DOMSubtreeModified", runTest, false);
+
+var r = document.createRange();
+r.setStartBefore(document.getElementById('start'));
+r.setEndAfter(document.getElementById('end'));
+r.extractContents();
+</script>
+</body>
+</html>
diff --git a/LayoutTests/fast/dom/Range/range-extract-contents-event-fire-crash2-expected.txt b/LayoutTests/fast/dom/Range/range-extract-contents-event-fire-crash2-expected.txt
new file mode 100644 (file)
index 0000000..f528f9c
--- /dev/null
@@ -0,0 +1 @@
+PASS: does not crash
diff --git a/LayoutTests/fast/dom/Range/range-extract-contents-event-fire-crash2.html b/LayoutTests/fast/dom/Range/range-extract-contents-event-fire-crash2.html
new file mode 100644 (file)
index 0000000..018a1d7
--- /dev/null
@@ -0,0 +1,30 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>
+    <span>
+        <span id="start"></span>
+    </span>
+    <span>
+        <span id="end"></span>
+    </span>
+</p>
+<script>
+if (window.layoutTestController)
+    layoutTestController.dumpAsText();
+
+function runTest()
+{
+    document.removeEventListener("DOMSubtreeModified", runTest, false);
+    document.body.innerHTML = 'PASS: does not crash';
+}
+
+document.addEventListener("DOMSubtreeModified", runTest, false);
+
+var r = document.createRange();
+r.setStartBefore(document.getElementById('start'));
+r.setEndAfter(document.getElementById('end'));
+r.extractContents();
+</script>
+</body>
+</html>
index f10ae50..e896cc8 100644 (file)
@@ -21,7 +21,6 @@
                 r.setStartBefore(document.getElementById('start'));
                 r.setEndAfter(document.getElementById('end'));
                 var fragment = r.extractContents();
-                document.body.appendChild(fragment);
             } catch(e) {
             }
             log('PASS: No crash.');
index ffc7297..6b010c6 100644 (file)
@@ -1,3 +1,23 @@
+2011-06-30  Abhishek Arya  <inferno@chromium.org>
+
+        Reviewed by Ryosuke Niwa.
+
+        Crash when calling DOMSubtreeModified event when extracting range
+        contents.
+        https://bugs.webkit.org/show_bug.cgi?id=63650
+
+        Convert a few nodes to RefPtrs and add commonRoot verification checks
+        for Range::processContents.
+
+        Tests: fast/dom/Range/range-extract-contents-event-fire-crash.html
+               fast/dom/Range/range-extract-contents-event-fire-crash2.html
+
+        * dom/Range.cpp:
+        (WebCore::childOfCommonRootBeforeOffset):
+        (WebCore::Range::processContents):
+        (WebCore::Range::processContentsBetweenOffsets):
+        (WebCore::Range::processAncestorsAndTheirSiblings):
+
 2011-06-30  Dan Bernstein  <mitz@apple.com>
 
         Reviewed by Adele Peterson.
index f7f0c7a..b2574a0 100644 (file)
@@ -628,7 +628,9 @@ static inline Node* childOfCommonRootBeforeOffset(Node* container, unsigned offs
 {
     ASSERT(container);
     ASSERT(commonRoot);
-    ASSERT(commonRoot->contains(container));
+    
+    if (!commonRoot->contains(container))
+        return 0;
 
     if (container == commonRoot) {
         container = container->firstChild();
@@ -682,7 +684,7 @@ PassRefPtr<DocumentFragment> Range::processContents(ActionType action, Exception
     if (ec)
         return 0;
 
-    Node* commonRoot = commonAncestorContainer(ec);
+    RefPtr<Node> commonRoot = commonAncestorContainer(ec);
     if (ec)
         return 0;
     ASSERT(commonRoot);
@@ -693,8 +695,8 @@ PassRefPtr<DocumentFragment> Range::processContents(ActionType action, Exception
     }
 
     // what is the highest node that partially selects the start / end of the range?
-    Node* partialStart = highestAncestorUnderCommonRoot(m_start.container(), commonRoot);
-    Node* partialEnd = highestAncestorUnderCommonRoot(m_end.container(), commonRoot);
+    RefPtr<Node> partialStart = highestAncestorUnderCommonRoot(m_start.container(), commonRoot.get());
+    RefPtr<Node> partialEnd = highestAncestorUnderCommonRoot(m_end.container(), commonRoot.get());
 
     // Start and end containers are different.
     // There are three possibilities here:
@@ -713,29 +715,32 @@ PassRefPtr<DocumentFragment> Range::processContents(ActionType action, Exception
     //
     // These are deleted, cloned, or extracted (i.e. both) depending on action.
 
+    // Note that we are verifying that our common root hierarchy is still intact
+    // after any DOM mutation event, at various stages below. See webkit bug 60350.
+
     RefPtr<Node> leftContents;
-    if (m_start.container() != commonRoot) {
+    if (m_start.container() != commonRoot && commonRoot->contains(m_start.container())) {
         leftContents = processContentsBetweenOffsets(action, 0, m_start.container(), m_start.offset(), lengthOfContentsInNode(m_start.container()), ec);
-        leftContents = processAncestorsAndTheirSiblings(action, m_start.container(), ProcessContentsForward, leftContents, commonRoot, ec);
+        leftContents = processAncestorsAndTheirSiblings(action, m_start.container(), ProcessContentsForward, leftContents, commonRoot.get(), ec);
     }
 
     RefPtr<Node> rightContents;
-    if (m_end.container() != commonRoot) {
+    if (m_end.container() != commonRoot && commonRoot->contains(m_end.container())) {
         rightContents = processContentsBetweenOffsets(action, 0, m_end.container(), 0, m_end.offset(), ec);
-        rightContents = processAncestorsAndTheirSiblings(action, m_end.container(), ProcessContentsBackward, rightContents, commonRoot, ec);
+        rightContents = processAncestorsAndTheirSiblings(action, m_end.container(), ProcessContentsBackward, rightContents, commonRoot.get(), ec);
     }
 
     // delete all children of commonRoot between the start and end container
-    Node* processStart = childOfCommonRootBeforeOffset(m_start.container(), m_start.offset(), commonRoot);
-    if (m_start.container() != commonRoot) // processStart contains nodes before m_start.
+    RefPtr<Node> processStart = childOfCommonRootBeforeOffset(m_start.container(), m_start.offset(), commonRoot.get());
+    if (processStart && m_start.container() != commonRoot) // processStart contains nodes before m_start.
         processStart = processStart->nextSibling();
-    Node* processEnd = childOfCommonRootBeforeOffset(m_end.container(), m_end.offset(), commonRoot);
+    RefPtr<Node> processEnd = childOfCommonRootBeforeOffset(m_end.container(), m_end.offset(), commonRoot.get());
 
     // Collapse the range, making sure that the result is not within a node that was partially selected.
     if (action == EXTRACT_CONTENTS || action == DELETE_CONTENTS) {
-        if (partialStart)
+        if (partialStart && commonRoot->contains(partialStart.get()))
             setStart(partialStart->parentNode(), partialStart->nodeIndex() + 1, ec);
-        else if (partialEnd)
+        else if (partialEnd && commonRoot->contains(partialEnd.get()))
             setStart(partialEnd->parentNode(), partialEnd->nodeIndex(), ec);
         if (ec)
             return 0;
@@ -750,7 +755,7 @@ PassRefPtr<DocumentFragment> Range::processContents(ActionType action, Exception
 
     if (processStart) {
         NodeVector nodes;
-        for (Node* n = processStart; n && n != processEnd; n = n->nextSibling())
+        for (Node* n = processStart.get(); n && n != processEnd; n = n->nextSibling())
             nodes.append(n);
         processNodes(action, nodes, commonRoot, fragment, ec);
     }
@@ -841,7 +846,7 @@ PassRefPtr<Node> Range::processContentsBetweenOffsets(ActionType action, PassRef
         break;
     }
 
-    return result;
+    return result.release();
 }
 
 void Range::processNodes(ActionType action, Vector<RefPtr<Node> >& nodes, PassRefPtr<Node> oldContainer, PassRefPtr<Node> newContainer, ExceptionCode& ec)
@@ -906,7 +911,7 @@ PassRefPtr<Node> Range::processAncestorsAndTheirSiblings(ActionType action, Node
         firstChildInAncestorToProcess = direction == ProcessContentsForward ? ancestor->nextSibling() : ancestor->previousSibling();
     }
 
-    return clonedContainer;
+    return clonedContainer.release();
 }
 
 PassRefPtr<DocumentFragment> Range::extractContents(ExceptionCode& ec)