Reviewers: alancutter, Yosi_UTC9
CL:
https://codereview.chromium.org/2103043004/Message:
I don't really know the surrounding context to this code, so the test is just
copied from clusterfuzz as I couldn't work out how to simplify it. I couldn't
get a test to crash a local build, but the test does crash my dev and stable
chrome builds.
Description:
Fix EditingStyle::mergeStyle()'s handling of custom properties
This patch fixes the logic of EditingStyle::mergeStyle() to correctly
handle custom properties. Currently it serializes the CSSValue and then
reparses it, which, aside from being inefficient, doesn't work for
custom properties as the custom property name is lost (since we only
have the enum value CSSPropertyVariable).
BUG=622420
Base URL:
https://chromium.googlesource.com/chromium/src.git@masterAffected files (+25, -1 lines):
A third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html
A third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash-expected.txt
M third_party/WebKit/Source/core/css/StylePropertySet.cpp
M third_party/WebKit/Source/core/editing/EditingStyle.cpp
Index: third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash-expected.txt
diff --git a/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash-expected.txt b/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash-expected.txt
new file mode 100644
index 0000000000000000000000000000000000000000..b964f496bc201639ca4bc47563d767bbb222271a
--- /dev/null
+++ b/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash-expected.txt
@@ -0,0 +1,3 @@
+CONSOLE WARNING: line 6: The <keygen> element is deprecated and will be removed in M54, around October 2016. See
https://www.chromestatus.com/features/5716060992962560 for more details.
+
+This test passes if it doesn't crash.
Index: third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html
diff --git a/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html b/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html
new file mode 100644
index 0000000000000000000000000000000000000000..800a236d2361d804e83ee0e95a481d39cd7c95f0
--- /dev/null
+++ b/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html
@@ -0,0 +1,19 @@
+<script>
+document.documentElement.contentEditable="true"
+document.documentElement.appendChild(document.createElement('table'))
+eAcronym = document.createElement('acronym')
+document.documentElement.appendChild(eAcronym)
+document.documentElement.appendChild(document.createElement('keygen'))
+newElem = document.createElement('figure')
+newElem.style.cssText = '--AAAA: var(--BBBB)'
+document.documentElement.appendChild(newElem)
+eCite = document.createElement('cite')
+eCite.style.cssText = 'float: var(--CCCC)'
+eAcronym.appendChild(eCite)
+eCite.appendChild(document.createElement('marquee'));
+document.execCommand('SelectAll', 'false', 'true')
+document.execCommand('RemoveFormat', 'true', 'true')
+window.testRunner && testRunner.dumpAsText();
+</script>
+
+This test passes if it doesn't crash.
Index: third_party/WebKit/Source/core/css/StylePropertySet.cpp
diff --git a/third_party/WebKit/Source/core/css/StylePropertySet.cpp b/third_party/WebKit/Source/core/css/StylePropertySet.cpp
index da2011104f05b9186b55b600b5a4bcb4d182da7b..680039f26f1dd69cdc70ab9d14ac52b5cfbf858d 100644
--- a/third_party/WebKit/Source/core/css/StylePropertySet.cpp
+++ b/third_party/WebKit/Source/core/css/StylePropertySet.cpp
@@ -290,6 +290,8 @@ bool StylePropertySet::isPropertyImplicit(CSSPropertyID propertyID) const
bool MutableStylePropertySet::setProperty(CSSPropertyID unresolvedProperty, const String& value, bool important, StyleSheetContents* contextStyleSheet)
{
+ DCHECK_GE(unresolvedProperty, firstCSSProperty);
+
// Setting the value to an empty string just removes the property in both IE and Gecko.
// Setting it to null seems to produce less consistent results, but we treat it just the same.
if (value.isEmpty())
Index: third_party/WebKit/Source/core/editing/EditingStyle.cpp
diff --git a/third_party/WebKit/Source/core/editing/EditingStyle.cpp b/third_party/WebKit/Source/core/editing/EditingStyle.cpp
index 503a775758c2357efb9c415d85aca523cd5a61bf..bb85cabc102024b01ed59a11553d09c936f79c67 100644
--- a/third_party/WebKit/Source/core/editing/EditingStyle.cpp
+++ b/third_party/WebKit/Source/core/editing/EditingStyle.cpp
@@ -1180,7 +1180,7 @@ void EditingStyle::mergeStyle(const StylePropertySet* style, CSSPropertyOverride
}
if (mode == OverrideValues || (mode == DoNotOverrideValues && !value))
- m_mutableStyle->setProperty(
property.id(), property.value()->cssText(), property.isImportant());
+ m_mutableStyle->setProperty(property.toCSSProperty());
}
}