Crash in EditingStyle::mergeStyle

https://bugs.webkit.org/show_bug.cgi?id=94740 Patch by Sukolsak Sakshuwong <sukolsak@gmail.com> on 2012-08-28 Reviewed by Ryosuke Niwa. Source/WebCore: This bug happened when we selected "1<progress><a style>2</a></progress>" and executed a create link command because 1. The selection ended at <progress>, not the text node inside it, because <progress> is an atomic node. 2. We called removeInlineStyle() to remove conflicting styles. Since the selection started at the text node "1" and ended at <progress>, we did not get to remove <a>. 3. We called fixRangeAndApplyInlineStyle(), which in turn called applyInlineStyleToNodeRange(). This method split the node range into smaller runs. In this case, the run was the whole "1<progress><a style>2</a></progress>". 4. We called removeStyleFromRunBeforeApplyingStyle(). This method tried to remove <a> by calling removeInlineStyleFromElement() on <a> with extractedStyle = 0. But the method expected that extractedStyle was not null. So, it crashed. This bug doesn't happen with non-atomic nodes because if <a> is inside a non-atomic node, <a> will be covered by the selection. Therefore, it will be removed in step #2 and we will never call removeInlineStyleFromElement() on <a> again. Thus, the assertion that extractedStyle is not null is reasonable. Hence, this patch fixes this bug by skipping over atomic nodes when we apply style. Test: editing/style/apply-style-atomic.html * editing/ApplyStyleCommand.cpp: (WebCore::ApplyStyleCommand::removeStyleFromRunBeforeApplyingStyle): (WebCore::ApplyStyleCommand::removeInlineStyle): LayoutTests: * editing/style/apply-style-atomic-expected.txt: Added. * editing/style/apply-style-atomic.html: Added. git-svn-id: http://svn.webkit.org/repository/webkit/trunk@126865 268f45cc-cd09-0410-ab3c-d52691b4dbfc
eseidel · Aug 28, 2012 · d5720d8 · d5720d8
1 parent c09e79f
commit d5720d8
Show file tree

Hide file tree

Showing 5 changed files with 98 additions and 2 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,13 @@
+2012-08-28  Sukolsak Sakshuwong  <sukolsak@gmail.com>
+
+        Crash in EditingStyle::mergeStyle
+        https://bugs.webkit.org/show_bug.cgi?id=94740
+
+        Reviewed by Ryosuke Niwa.
+
+        * editing/style/apply-style-atomic-expected.txt: Added.
+        * editing/style/apply-style-atomic.html: Added.
+
 2012-08-28  Thiago Marcos P. Santos  <thiago.santos@intel.com>
 
         [EFL] Range input ignores padding

diff --git a/LayoutTests/editing/style/apply-style-atomic-expected.txt b/LayoutTests/editing/style/apply-style-atomic-expected.txt
@@ -0,0 +1,17 @@
+Test that WebKit does not crash when we apply style to atomic elements and that the style is not applied inside atomic elements.
+| <a>
+|   href="a"
+|   "<#selection-anchor>1"
+|   <progress>
+|     <a>
+|       style=""
+|       "2"
+|     <shadow:root>
+|       <div>
+|         shadow:pseudoId="-webkit-progress-inner-element"
+|         <div>
+|           shadow:pseudoId="-webkit-progress-bar"
+|           <div>
+|             style="width: -100%;"
+|             shadow:pseudoId="-webkit-progress-value"
+|   <#selection-focus>
diff --git a/LayoutTests/editing/style/apply-style-atomic.html b/LayoutTests/editing/style/apply-style-atomic.html
@@ -0,0 +1,24 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../../resources/dump-as-markup.js"></script>
+</head>
+<body>
+<div id="edit" contentEditable="true">1<progress><a style>2</a></progress></div>
+<script>
+Markup.description('Test that WebKit does not crash when we apply style to atomic elements ' +
+'and that the style is not applied inside atomic elements.')
+
+function select(node) {
+    var range = document.createRange();
+    range.selectNodeContents(node);
+    window.getSelection().addRange(range);
+}
+
+var edit = document.getElementById("edit");
+select(edit);
+document.execCommand("createlink", false, "a");
+Markup.dump(edit);
+</script>
+</body>
+</html>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,39 @@
+2012-08-28  Sukolsak Sakshuwong  <sukolsak@gmail.com>
+
+        Crash in EditingStyle::mergeStyle
+        https://bugs.webkit.org/show_bug.cgi?id=94740
+
+        Reviewed by Ryosuke Niwa.
+
+        This bug happened when we selected "1<progress><a style>2</a></progress>"
+        and executed a create link command because
+
+        1. The selection ended at <progress>, not the text node inside it, because
+           <progress> is an atomic node.
+        2. We called removeInlineStyle() to remove conflicting styles.
+           Since the selection started at the text node "1" and ended at <progress>,
+           we did not get to remove <a>.
+        3. We called fixRangeAndApplyInlineStyle(), which in turn called
+           applyInlineStyleToNodeRange(). This method split the node range
+           into smaller runs. In this case, the run was the whole
+           "1<progress><a style>2</a></progress>".
+        4. We called removeStyleFromRunBeforeApplyingStyle(). This method tried
+           to remove <a> by calling removeInlineStyleFromElement() on <a> with
+           extractedStyle = 0. But the method expected that extractedStyle was not null.
+           So, it crashed.
+
+        This bug doesn't happen with non-atomic nodes because if <a> is inside a non-atomic
+        node, <a> will be covered by the selection. Therefore, it will be removed in
+        step #2 and we will never call removeInlineStyleFromElement() on <a>
+        again. Thus, the assertion that extractedStyle is not null is reasonable.
+        Hence, this patch fixes this bug by skipping over atomic nodes when we apply style.
+
+        Test: editing/style/apply-style-atomic.html
+
+        * editing/ApplyStyleCommand.cpp:
+        (WebCore::ApplyStyleCommand::removeStyleFromRunBeforeApplyingStyle):
+        (WebCore::ApplyStyleCommand::removeInlineStyle):
+
 2012-08-28  Thiago Marcos P. Santos  <thiago.santos@intel.com>
 
         [EFL] Range input ignores padding

diff --git a/Source/WebCore/editing/ApplyStyleCommand.cpp b/Source/WebCore/editing/ApplyStyleCommand.cpp
@@ -798,7 +798,11 @@ bool ApplyStyleCommand::removeStyleFromRunBeforeApplyingStyle(EditingStyle* styl
 
     RefPtr<Node> next = runStart;
     for (RefPtr<Node> node = next; node && node->inDocument() && node != pastEndNode; node = next) {
-        next = node->traverseNextNode();
+        if (editingIgnoresContent(node.get())) {
+            ASSERT(!node->contains(pastEndNode.get()));
+            next = node->traverseNextSibling();
+        } else
+            next = node->traverseNextNode();
         if (!node->isHTMLElement())
             continue;
 
@@ -1048,7 +1052,12 @@ void ApplyStyleCommand::removeInlineStyle(EditingStyle* style, const Position &s
 
     Node* node = start.deprecatedNode();
     while (node) {
-        RefPtr<Node> next = node->traverseNextNode();
+        RefPtr<Node> next;
+        if (editingIgnoresContent(node)) {
+            ASSERT(node == end.deprecatedNode() || !node->contains(end.deprecatedNode()));
+            next = node->traverseNextSibling();
+        } else
+            next = node->traverseNextNode();
         if (node->isHTMLElement() && nodeFullySelected(node, start, end)) {
             RefPtr<HTMLElement> elem = toHTMLElement(node);
             RefPtr<Node> prev = elem->traversePreviousNodePostOrder();