Fix EditingStyle::mergeStyle()'s handling of custom properties (issue 2103043004 by timloh@chromium.org)

0 views
Skip to first unread message

tim...@chromium.org

unread,
Jun 29, 2016, 12:30:06 AM6/29/16
to alanc...@chromium.org, yo...@chromium.org, chromium...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, alexis...@intel.com, blink-...@chromium.org, rob....@samsung.com
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@master

Affected 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());
}
}



yosin@chromium.org via codereview.chromium.org

unread,
Jun 29, 2016, 1:11:42 AM6/29/16
to tim...@chromium.org, alanc...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, rob....@samsung.com
I can't access crbug.com/622420. Could you add me CC in that bug?
Thanks in advance.


https://codereview.chromium.org/2103043004/diff/1/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html
File
third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html
(right):

https://codereview.chromium.org/2103043004/diff/1/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html#newcode15
third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html:15:

document.execCommand('RemoveFormat', 'true', 'true')
Could you put write HTML in the file rather than constructing by script?

https://codereview.chromium.org/2103043004/diff/1/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html#newcode16
third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html:16:
window.testRunner && testRunner.dumpAsText();
Could you use test w3c harness? Using |assert_selection()| is preferred
in this case.
Sample: crrev.com/396768

https://codereview.chromium.org/2103043004/

tim...@chromium.org

unread,
Jun 29, 2016, 1:22:05 AM6/29/16
to yo...@chromium.org, alanc...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, rob....@samsung.com

https://codereview.chromium.org/2103043004/diff/1/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html
File
third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html
(right):

https://codereview.chromium.org/2103043004/diff/1/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html#newcode15
third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html:15:
document.execCommand('RemoveFormat', 'true', 'true')
On 2016/06/29 05:11:41, Yosi_UTC9 wrote:
> Could you put write HTML in the file rather than constructing by
script?

I tried doing this sort of thing but it no longer reproduced the issue,
and I don't really know what's going on in the editing code to work out
why. So I ended up just leaving the code pretty much verbatim from the
clusterfuzz case.


https://codereview.chromium.org/2103043004/diff/1/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html#newcode16
third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html:16:
window.testRunner && testRunner.dumpAsText();
On 2016/06/29 05:11:42, Yosi_UTC9 wrote:
> Could you use test w3c harness? Using |assert_selection()| is
preferred in this
> case.
> Sample: crrev.com/396768

I'll try, but I might not be able to get it to repro with it.

https://codereview.chromium.org/2103043004/

alanc...@chromium.org

unread,
Jun 29, 2016, 3:26:11 AM6/29/16
to tim...@chromium.org, yo...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, rob....@samsung.com
lgtm with test nit.



https://codereview.chromium.org/2103043004/diff/1/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html
File
third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html
(right):

https://codereview.chromium.org/2103043004/diff/1/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html#newcode16
third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html:16:
window.testRunner && testRunner.dumpAsText();
On 2016/06/29 at 05:22:04, Timothy Loh wrote:
> On 2016/06/29 05:11:42, Yosi_UTC9 wrote:
> > Could you use test w3c harness? Using |assert_selection()| is
preferred in this
> > case.
> > Sample: crrev.com/396768
>
> I'll try, but I might not be able to get it to repro with it.

testharness.js is desirable. You may need to wait a frame with
async_test() if an empty test(() => {}) doesn't work.

https://codereview.chromium.org/2103043004/

yosin@chromium.org via codereview.chromium.org

unread,
Jun 30, 2016, 12:07:38 AM6/30/16
to tim...@chromium.org, alanc...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, rob....@samsung.com

https://codereview.chromium.org/2103043004/diff/20001/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html
File
third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html
(right):

https://codereview.chromium.org/2103043004/diff/20001/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html#newcode11
third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html:11:
newElem.style.cssText = '--AAAA: var(--BBBB)';
I think this line should not affect test since it is meaningless style.

https://codereview.chromium.org/2103043004/diff/20001/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html#newcode14
third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html:14:
eCite.style.cssText = 'float: var(--CCCC)';
I think this line should not affect test since it is meaningless style.

https://codereview.chromium.org/2103043004/diff/20001/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html#newcode17
third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html:17:
document.execCommand('SelectAll', 'false', 'true');
nit: you don't need to have |, 'false', 'true'|.

https://codereview.chromium.org/2103043004/diff/20001/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html#newcode18
third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html:18:
document.execCommand('RemoveFormat', 'true', 'true');
nit: you don't need to have |, 'true', 'true'|.

https://codereview.chromium.org/2103043004/

tim...@chromium.org

unread,
Jun 30, 2016, 12:16:42 AM6/30/16
to yo...@chromium.org, alanc...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, rob....@samsung.com

https://codereview.chromium.org/2103043004/diff/1/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html
File
third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html
(right):

https://codereview.chromium.org/2103043004/diff/1/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html#newcode16
third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html:16:
window.testRunner && testRunner.dumpAsText();
On 2016/06/29 07:26:10, alancutter wrote:
> On 2016/06/29 at 05:22:04, Timothy Loh wrote:
> > On 2016/06/29 05:11:42, Yosi_UTC9 wrote:
> > > Could you use test w3c harness? Using |assert_selection()| is
preferred in
> this
> > > case.
> > > Sample: crrev.com/396768
> >
> > I'll try, but I might not be able to get it to repro with it.
>
> testharness.js is desirable. You may need to wait a frame with
async_test() if
> an empty test(() => {}) doesn't work.

I updated the test with a test(() => {}) call, seems to still work. I'm
not really sure how I should be using assert_selection() since I'm just
checking whether the page crashes.


https://codereview.chromium.org/2103043004/diff/20001/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html
File
third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html
(right):

https://codereview.chromium.org/2103043004/diff/20001/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html#newcode11
third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html:11:
newElem.style.cssText = '--AAAA: var(--BBBB)';
On 2016/06/30 04:07:38, Yosi_UTC9 wrote:
> I think this line should not affect test since it is meaningless
style.

This is necessary for the crash, since this results in the declaration
which mergeStyle doesn't correctly handle.


https://codereview.chromium.org/2103043004/diff/20001/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html#newcode14
third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html:14:
eCite.style.cssText = 'float: var(--CCCC)';
On 2016/06/30 04:07:38, Yosi_UTC9 wrote:
> I think this line should not affect test since it is meaningless
style.

Seems to be required to repro the crash.


https://codereview.chromium.org/2103043004/diff/20001/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html#newcode17
third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html:17:
document.execCommand('SelectAll', 'false', 'true');
On 2016/06/30 04:07:38, Yosi_UTC9 wrote:
> nit: you don't need to have |, 'false', 'true'|.

removed


https://codereview.chromium.org/2103043004/diff/20001/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html#newcode18
third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html:18:
document.execCommand('RemoveFormat', 'true', 'true');
On 2016/06/30 04:07:38, Yosi_UTC9 wrote:
> nit: you don't need to have |, 'true', 'true'|.

yosin@chromium.org via codereview.chromium.org

unread,
Jul 1, 2016, 5:28:50 AM7/1/16
to tim...@chromium.org, alanc...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, rob....@samsung.com

https://codereview.chromium.org/2103043004/diff/40001/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html
File
third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html
(right):

https://codereview.chromium.org/2103043004/diff/40001/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html#newcode1
third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html:1:
<!doctype html>
How about writing gTest for this case, e.g. EditingStyleTest?
This layout test script is rather cryptic and hard to understand "flat:
var(--CCC)" related to |mergeStyle()|.

https://codereview.chromium.org/2103043004/

tim...@chromium.org

unread,
Jul 6, 2016, 10:54:33 PM7/6/16
to yo...@chromium.org, alanc...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, rob....@samsung.com

https://codereview.chromium.org/2103043004/diff/40001/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html
File
third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html
(right):

https://codereview.chromium.org/2103043004/diff/40001/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html#newcode1
third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html:1:
<!doctype html>
On 2016/07/01 09:28:49, Yosi_UTC9 wrote:
> How about writing gTest for this case, e.g. EditingStyleTest?
> This layout test script is rather cryptic and hard to understand
"flat:
> var(--CCC)" related to |mergeStyle()|.

The test right now is just a copy of the clusterfuzz test with the
changes that reviewers have suggested. I'm not familiar with editing
code to write gTest test for it. I'm also not sure if using gTest is a
good idea for tests where we just want to verify that something doesn't
crash.

https://codereview.chromium.org/2103043004/

yosin@chromium.org via codereview.chromium.org

unread,
Jul 7, 2016, 1:47:23 AM7/7/16
to tim...@chromium.org, alanc...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, rob....@samsung.com

https://codereview.chromium.org/2103043004/diff/40001/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html
File
third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html
(right):

https://codereview.chromium.org/2103043004/diff/40001/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html#newcode1
third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html:1:
<!doctype html>
On 2016/07/07 at 02:54:32, Timothy Loh wrote:
> On 2016/07/01 09:28:49, Yosi_UTC9 wrote:
> > How about writing gTest for this case, e.g. EditingStyleTest?
> > This layout test script is rather cryptic and hard to understand
"flat:
> > var(--CCC)" related to |mergeStyle()|.
>
> The test right now is just a copy of the clusterfuzz test with the
changes that reviewers have suggested. I'm not familiar with editing
code to write gTest test for it. I'm also not sure if using gTest is a
good idea for tests where we just want to verify that something doesn't
crash.

We would like to check result of the regressed function to avoid future
regression in addition to not crashing.
I write an example of gTest for test: http://crrev.com/2126143002
I think gTest is simpler and more specific in this case.

I hope you agree to using gTest rather than layout test.

https://codereview.chromium.org/2103043004/

alanc...@chromium.org

unread,
Jul 14, 2016, 3:23:18 AM7/14/16
to tim...@chromium.org, yo...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, rob....@samsung.com
We should have a layout test to ensure the crash is no longer triggered just by
using the web platform API, gTests don't provide the same verification.
Stylistic decisions about whether the test should be cleaned up and whether a
gTest should be made should not block landing this crash fix, please see the
associated bug.

https://codereview.chromium.org/2103043004/

yosin@chromium.org via codereview.chromium.org

unread,
Jul 14, 2016, 3:49:56 AM7/14/16
to tim...@chromium.org, alanc...@chromium.org, tk...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, rob....@samsung.com

tk...@chromium.org

unread,
Jul 14, 2016, 4:14:13 AM7/14/16
to tim...@chromium.org, yo...@chromium.org, alanc...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, rob....@samsung.com
On 2016/07/14 at 07:49:55, yosin wrote:
> +tkent@, WDYT?

IMO, we should make C++ unit tests for crash fixes. Crash bugs strongly depend
on internal code structure, and layout tests for crash bugs easily become
meaningless by internal code changes. For example, Oilpan must make many tests
meaningless, but we have no good ways to know what tests we can remove.

Even if we write a layout test for a crash, we should minimize the test case
produced by ClusterFuzz in order to make the test robust. We'll remove "keygen"
element soon. Will the test make sense after that?


https://codereview.chromium.org/2103043004/
Reply all
Reply to author
Forward
0 new messages