REGRESSION(r183770): Crash inside WebEditorClient::shouldApplyStyle when applying...
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 13 May 2015 21:58:41 +0000 (21:58 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 13 May 2015 21:58:41 +0000 (21:58 +0000)
https://bugs.webkit.org/show_bug.cgi?id=144949
Source/WebCore:

<rdar://problem/20895753>

Reviewed by Darin Adler.

The crash was caused by the variant of applyStyleToSelection that takes EditingStyle passing
a null pointer to shouldApplyStyle when we're only applying text decoration changes so that
m_mutableStyle in the editing style is null. This didn't reproduce in execCommand since we
wouldn't call shouldApplyStyle in that case. It didn't reproduce in my manual testing because
font panel also sets text shadow, which ends up filling up m_mutableStyle.

Fixed the bug by creating a mutable style properties when one is not provided by EditingStyle.
Also fixed the "FIXME" in the function by converting text decoration changes to a corresponding
text decoration value. The values passed to shouldApplyStyle now matches the old behavior prior
to r183770.

Test: editing/style/underline-by-user.html

* editing/EditingStyle.cpp:
(WebCore::EditingStyle::styleWithResolvedTextDecorations): Added.
* editing/EditingStyle.h:
* editing/Editor.cpp:
(WebCore::Editor::applyStyleToSelection): Use styleWithResolvedTextDecorations to avoid the crash.

LayoutTests:

Reviewed by Darin Adler.

Added a test that emulates underlining of text by the user. Unlike document.execCommand,
testRunner.execCommand simulates a user initiated editing command and therefore invokes
shouldApplyStyle.

* editing/style/underline-by-user-expected.txt: Added.
* editing/style/underline-by-user.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/editing/style/underline-by-user-expected.txt [new file with mode: 0644]
LayoutTests/editing/style/underline-by-user.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/editing/EditingStyle.cpp
Source/WebCore/editing/EditingStyle.h
Source/WebCore/editing/Editor.cpp

index 7bbb80eaa5b19c40695c8f779103e899033505ba..6b4d8522c95a5a193236b449e4d492bebbb5a2ae 100644 (file)
@@ -1,3 +1,17 @@
+2015-05-13  Ryosuke Niwa  <rniwa@webkit.org>
+
+        REGRESSION(r183770): Crash inside WebEditorClient::shouldApplyStyle when applying underline
+        https://bugs.webkit.org/show_bug.cgi?id=144949
+
+        Reviewed by Darin Adler.
+
+        Added a test that emulates underlining of text by the user. Unlike document.execCommand,
+        testRunner.execCommand simulates a user initiated editing command and therefore invokes
+        shouldApplyStyle.
+
+        * editing/style/underline-by-user-expected.txt: Added.
+        * editing/style/underline-by-user.html: Added.
+
 2015-05-13  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [ES6] Implement String.raw
diff --git a/LayoutTests/editing/style/underline-by-user-expected.txt b/LayoutTests/editing/style/underline-by-user-expected.txt
new file mode 100644 (file)
index 0000000..0d02775
--- /dev/null
@@ -0,0 +1,3 @@
+This tests user initialized underlining of text. To manually test, underline "hello" below. WebKit should not crash.
+
+hello
diff --git a/LayoutTests/editing/style/underline-by-user.html b/LayoutTests/editing/style/underline-by-user.html
new file mode 100644 (file)
index 0000000..bd134c3
--- /dev/null
@@ -0,0 +1,15 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>This tests user initialized underlining of text. To manually test, underline "hello" below. WebKit should not crash.</p>
+<div id="editor" contenteditable>hello</div>
+<script>
+document.getElementById('editor').focus();
+document.execCommand('selectAll', false, null);
+if (window.testRunner) {
+    testRunner.dumpAsText();
+    testRunner.execCommand('underline', false, null);
+}
+</script>
+</body>
+</html>
index 094a015f28a5021eb53b03648815534d61c51da9..3c07f00dfa6a77aa71170f21ae9895cda8fd0c41 100644 (file)
@@ -1,3 +1,30 @@
+2015-05-13  Ryosuke Niwa  <rniwa@webkit.org>
+
+        REGRESSION(r183770): Crash inside WebEditorClient::shouldApplyStyle when applying underline
+        https://bugs.webkit.org/show_bug.cgi?id=144949
+        <rdar://problem/20895753>
+
+        Reviewed by Darin Adler.
+
+        The crash was caused by the variant of applyStyleToSelection that takes EditingStyle passing
+        a null pointer to shouldApplyStyle when we're only applying text decoration changes so that
+        m_mutableStyle in the editing style is null. This didn't reproduce in execCommand since we
+        wouldn't call shouldApplyStyle in that case. It didn't reproduce in my manual testing because
+        font panel also sets text shadow, which ends up filling up m_mutableStyle.
+
+        Fixed the bug by creating a mutable style properties when one is not provided by EditingStyle.
+        Also fixed the "FIXME" in the function by converting text decoration changes to a corresponding
+        text decoration value. The values passed to shouldApplyStyle now matches the old behavior prior
+        to r183770.
+
+        Test: editing/style/underline-by-user.html
+
+        * editing/EditingStyle.cpp:
+        (WebCore::EditingStyle::styleWithResolvedTextDecorations): Added.
+        * editing/EditingStyle.h:
+        * editing/Editor.cpp:
+        (WebCore::Editor::applyStyleToSelection): Use styleWithResolvedTextDecorations to avoid the crash.
+
 2015-05-13  Eric Carlson  <eric.carlson@apple.com>
 
         Work around HTMLMediaElement::documentDidResumeFromPageCache being called twice
index f2b0104019d2ddf93ea2773721c000f8cd320f3a..dcf4fabcb8f7341b0d8f5f99abce3460ec4f8437 100644 (file)
@@ -525,6 +525,28 @@ bool EditingStyle::isEmpty() const
         && underlineChange() == TextDecorationChange::None && strikeThroughChange() == TextDecorationChange::None;
 }
 
+Ref<MutableStyleProperties> EditingStyle::styleWithResolvedTextDecorations() const
+{
+    bool hasTextDecorationChanges = underlineChange() != TextDecorationChange::None || strikeThroughChange() != TextDecorationChange::None;
+    if (m_mutableStyle && !hasTextDecorationChanges)
+        return *m_mutableStyle;
+
+    Ref<MutableStyleProperties> style = m_mutableStyle ? m_mutableStyle->mutableCopy() : MutableStyleProperties::create();
+
+    Ref<CSSValueList> valueList = CSSValueList::createSpaceSeparated();
+    if (underlineChange() == TextDecorationChange::Add)
+        valueList->append(cssValuePool().createIdentifierValue(CSSValueUnderline));
+    if (strikeThroughChange() == TextDecorationChange::Add)
+        valueList->append(cssValuePool().createIdentifierValue(CSSValueLineThrough));
+
+    if (valueList->length())
+        style->setProperty(CSSPropertyTextDecoration, valueList.ptr());
+    else
+        style->setProperty(CSSPropertyTextDecoration, cssValuePool().createIdentifierValue(CSSValueNone));
+
+    return style;
+}
+
 bool EditingStyle::textDirection(WritingDirection& writingDirection) const
 {
     if (!m_mutableStyle)
index 4ea980a94084d9367dca44b7e5700f5341258b3c..1279ce9a11c5cde8ce207761b5a70566f546bb08 100644 (file)
@@ -112,6 +112,7 @@ public:
     WEBCORE_EXPORT ~EditingStyle();
 
     MutableStyleProperties* style() { return m_mutableStyle.get(); }
+    Ref<MutableStyleProperties> styleWithResolvedTextDecorations() const;
     bool textDirection(WritingDirection&) const;
     bool isEmpty() const;
     void setStyle(PassRefPtr<MutableStyleProperties>);
index 1ac7b87f550a80d910e077063a82f049383f607b..9c49bebdd83ee4a8666d3f3334b8c2e9783aa41e 100644 (file)
@@ -947,7 +947,7 @@ void Editor::applyStyleToSelection(Ref<EditingStyle>&& style, EditAction editing
         return;
 
     // FIXME: This is wrong for text decorations since m_mutableStyle is empty.
-    if (!client() || !client()->shouldApplyStyle(style->style(), m_frame.selection().toNormalizedRange().get()))
+    if (!client() || !client()->shouldApplyStyle(style->styleWithResolvedTextDecorations().ptr(), m_frame.selection().toNormalizedRange().get()))
         return;
 
     applyStyle(WTF::move(style), editingAction);