Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2103043004:
Fix EditingStyle::mergeStyle()'s handling of custom properties (Closed)
|
Created:
4 years, 5 months ago by Timothy Loh Modified:
4 years, 4 months ago CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-style_chromium.org, chromium-reviews, dglazkov+blink, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix 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
Committed: https://crrev.com/aadb63893e4c1358d1e5139aa29552eb190682c8
Cr-Commit-Position: refs/heads/master@{#410614}
Patch Set 1 #
Total comments: 6
Patch Set 2 : test tweak #
Total comments: 8
Patch Set 3 : test tweak #
Total comments: 3
Patch Set 4 : gtest instead of layout test #
Messages
Total messages: 31 (11 generated)
Description was changed from ========== ---------------------This line is 72 characters long-------------------- bla BUG=622420 ========== to ========== ---------------------This line is 72 characters long-------------------- 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 ==========
timloh@chromium.org changed reviewers: + alancutter@chromium.org
Message was sent while issue was closed.
Description was changed from ========== ---------------------This line is 72 characters long-------------------- 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 ========== to ========== 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 ==========
timloh@chromium.org changed reviewers: + yosin@chromium.org
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.
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/LayoutTe... File third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html (right): https://codereview.chromium.org/2103043004/diff/1/third_party/WebKit/LayoutTe... 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/LayoutTe... 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/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html (right): https://codereview.chromium.org/2103043004/diff/1/third_party/WebKit/LayoutTe... 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/LayoutTe... 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.
lgtm with test nit. https://codereview.chromium.org/2103043004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html (right): https://codereview.chromium.org/2103043004/diff/1/third_party/WebKit/LayoutTe... 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/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html (right): https://codereview.chromium.org/2103043004/diff/20001/third_party/WebKit/Layo... 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/Layo... 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/Layo... 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/Layo... 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/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html (right): https://codereview.chromium.org/2103043004/diff/1/third_party/WebKit/LayoutTe... 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/Layo... File third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html (right): https://codereview.chromium.org/2103043004/diff/20001/third_party/WebKit/Layo... 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/Layo... 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/Layo... 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/Layo... 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'|. removed
https://codereview.chromium.org/2103043004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html (right): https://codereview.chromium.org/2103043004/diff/40001/third_party/WebKit/Layo... 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/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html (right): https://codereview.chromium.org/2103043004/diff/40001/third_party/WebKit/Layo... 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/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html (right): https://codereview.chromium.org/2103043004/diff/40001/third_party/WebKit/Layo... 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.
On 2016/07/07 at 05:47:22, yosin wrote: > https://codereview.chromium.org/2103043004/diff/40001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html (right): > > https://codereview.chromium.org/2103043004/diff/40001/third_party/WebKit/Layo... > 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. 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.
yosin@chromium.org changed reviewers: + tkent@chromium.org
+tkent@, WDYT?
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?
On 2016/07/07 05:47:22, Yosi_UTC9 wrote: > 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 removed the LayoutTest and added the gTest you suggested without modifications. On 2016/07/14 08:14:13, tkent wrote: > 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? In this case the test was by an *external* fuzzer and not ClusterFuzz. The crash doesn't repro when I change "keygen" to a different element (I tried div, span, button, select, and a few more) and I don't know about the editing codepaths enough to understand why.
lgtm since it has a test. On 2016/08/09 at 03:27:13, timloh wrote: > On 2016/07/07 05:47:22, Yosi_UTC9 wrote: > > 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 removed the LayoutTest and added the gTest you suggested without modifications. > > On 2016/07/14 08:14:13, tkent wrote: > > 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? > > In this case the test was by an *external* fuzzer and not ClusterFuzz. I don't understand your point of External or ClusterFuzz. Any fuzzers generates cryptic script not friendly for human. > The crash doesn't repro when I change "keygen" to a different element (I tried div, span, button, select, and a few more) and I don't know about the editing codepaths enough to understand why. I wrote gTest from your changes to infer how to hit the case without deep understanding of code path. In my experience, minimizing Fuzzer's generated script is difficult and identify code path. Using gTest allows us to directly call a function causing an issue with parameters to reproduce. It is an edge case and subject for unit test.
The CQ bit was checked by timloh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alancutter@chromium.org Link to the patchset: https://codereview.chromium.org/2103043004/#ps60001 (title: "gtest instead of layout test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/09 05:15:59, Yosi_UTC9 wrote: > On 2016/08/09 at 03:27:13, timloh wrote: > > In this case the test was by an *external* fuzzer and not ClusterFuzz. > > I don't understand your point of External or ClusterFuzz. Any fuzzers generates > cryptic script not friendly for human. The crash has been around for a while but was never identified by CF, so we likely don't have coverage for this intersection of features in CF. If I understand correctly, CF has fuzzers which take existing layout tests and mutate them, so I wanted to have the test to help it.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by timloh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://crrev.com/aadb63893e4c1358d1e5139aa29552eb190682c8 Cr-Commit-Position: refs/heads/master@{#410614} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/aadb63893e4c1358d1e5139aa29552eb190682c8 Cr-Commit-Position: refs/heads/master@{#410614} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
