Bug 192396

Summary:

Web Inspector: Styles: toggling selected properties may cause data corruption

Product:

WebKit

Reporter:

Nikita Vasilyev <nvasilyev>

Component:

Web Inspector

Assignee:

Nikita Vasilyev <nvasilyev>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, ews-watchlist, hi, inspector-bugzilla-changes, rniwa, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	nvasilyev: review-, nvasilyev: commit-queue-
Patch	hi: review-
Archive of layout-test-results from ews106 for mac-sierra-wk2	none
Patch	nvasilyev: review-, nvasilyev: commit-queue-
WIP: single agent call	nvasilyev: review-, nvasilyev: commit-queue-
Patch	hi: review+, hi: commit-queue-
Patch	none

Nikita Vasilyev

Reported 2018-12-04 23:06:20 PST

Quickly commenting out and uncommenting selected properties may cause data corruption. 1. Open https://developer.apple.com/design/human-interface-guidelines/macos/visual-design/dark-mode/ 2. Inspect <body> 3. Open Styles panel in the right sidebar 4. Select the following 3 CSS properties: -moz-font-feature-settings: 'kern'; -webkit-font-smoothing: antialiased; -moz-osx-font-smoothing: grayscale; 5. Press Space key several 10 times as fast as possible. 6. Navigate to the source file by clicking on the CSS link. Expected: The 3 selected CSS properties should remain uncommented: -moz-font-feature-settings: 'kern'; -webkit-font-smoothing: antialiased; -moz-osx-font-smoothing: grayscale; Actual: The properties may get scrambled: -moz-font-feature-settings: 'kern';; -webki; -webkit-font-smoothing: antialiased;*/; -moz-osx-font-smoothing: grayscale;*/ale;; */bkit-font-smoothing: antialiased;moothing: grayscale; */; -moz-osx-font-smoothing: grayscale;/* -moz-osx-font-smoothing: grayscale; */ Notes: Using the style editing debug mode (Bug 192282) should help to understand the issue.

Attachments
Patch (6.49 KB, patch) 2018-12-06 16:15 PST, Nikita Vasilyev	nvasilyev: review- nvasilyev: commit-queue-	Details Formatted Diff Diff
Patch (7.66 KB, patch) 2018-12-06 23:14 PST, Nikita Vasilyev	hi: review-	Details Formatted Diff Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.24 MB, application/zip) 2018-12-07 02:28 PST, EWS Watchlist	no flags	Details
Patch (12.60 KB, patch) 2018-12-07 16:18 PST, Nikita Vasilyev	nvasilyev: review- nvasilyev: commit-queue-	Details Formatted Diff Diff
WIP: single agent call (15.42 KB, patch) 2018-12-08 15:38 PST, Nikita Vasilyev	nvasilyev: review- nvasilyev: commit-queue-	Details Formatted Diff Diff
Patch (20.62 KB, patch) 2018-12-08 16:00 PST, Nikita Vasilyev	hi: review+ hi: commit-queue-	Details Formatted Diff Diff
Patch (22.06 KB, patch) 2018-12-15 01:21 PST, Nikita Vasilyev	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2018-12-04 23:07:06 PST

<rdar://problem/46478383>

Nikita Vasilyev

Comment 2 2018-12-05 23:27:15 PST

This is the same bug as shown on https://bugs.webkit.org/show_bug.cgi?id=192282#c3.

Nikita Vasilyev

Comment 3 2018-12-06 16:15:53 PST

Created attachment 356761 [details] Patch

Nikita Vasilyev

Comment 4 2018-12-06 23:14:29 PST

Created attachment 356785 [details] Patch

EWS Watchlist

Comment 5 2018-12-07 02:28:04 PST

Comment on attachment 356785 [details] Patch Attachment 356785 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10302333 New failing tests: fast/visual-viewport/tiled-drawing/zoomed-fixed-scrolling-layers-state.html

EWS Watchlist

Comment 6 2018-12-07 02:28:06 PST

Created attachment 356790 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6

Nikita Vasilyev

Comment 7 2018-12-07 09:58:25 PST

Comment on attachment 356785 [details] Patch Unrelated test failures.

Devin Rousso

Comment 8 2018-12-07 14:15:50 PST

Comment on attachment 356785 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356785&action=review r-, due to the potential double agent call > LayoutTests/inspector/css/modify-css-property.html:131 > + name: "Comment out and uncomment a property", This is more of a "description", not a name. Our usual pattern is to use the suite name followed by a test name, all camel-cased. ModifyCSSProperty.CommentOutAndUncommentProperty > LayoutTests/inspector/css/modify-css-property.html:148 > + let styleDeclaration = getMatchedStyleDeclaration(); This should fail in the case that no style is found (would also require a `return null;` inside `getMatchedStyleDeclaration`). if (!styleDeclaration) { InspectorTest.fail("No style declaration found."); resolve(); return; } > LayoutTests/inspector/css/modify-css-property.html:154 > + getProperty("padding-right").commentOut(disabled); Ditto (>148). let property = getProperty("padding-right"); if (!property) { InspectorTest.fail("No property found."); resolve(); return; } > LayoutTests/inspector/css/modify-css-property.html:159 > + InspectorTest.expectEqual(styleDeclaration.text, ` > + /* padding-left: 2em; */ > + padding-right: 0px; > + `, `Style declaration text should update immediately with uncommented property.`); This is unfortunate, especially considering how specific it is with spacing. Is there any way we could move this outside the `InspectorText.expectEqual`, like maybe as a `const`, so it can be styled better? > LayoutTests/inspector/css/modify-css-property.html:164 > + getProperty("padding-right").commentOut(disabled); Ditto (>154). Also, is this expected to return a new property, or the same property? We should assert that the result is the same/different. > LayoutTests/inspector/css/modify-css-property.html:169 > + InspectorTest.expectEqual(styleDeclaration.text, ` > + /* padding-left: 2em; */ > + /* padding-right: 0px; */ > + `, `Style declaration text should update immediately with commented out property.`); Ditto (>159). > LayoutTests/inspector/css/modify-css-property.html:171 > + InspectorTest.expectThat(!getProperty("padding-right").enabled, `Commented out property is disabled.`); Ditto (>164). > LayoutTests/inspector/css/modify-css-property.html:212 > + /* padding-left: 2em; */ > + /* padding-right: 0px; */ We should add tests for different padding situations (e.g. if there is more than one space before/after the property). > Source/WebInspectorUI/ChangeLog:24 > + /* color: red; */ font-size: 12px I'm not sure if this is just how you formatted this, but shouldn't we be keeping the newline as well? > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:401 > + this.text = property.text.trimRight() + ";" + match[1]; This means that we'll be doing two updates of the owner style. We should try to do as few back/forth calls to the backend as possible. Also, I think this will only work if the owner style is `locked`, as otherwise the owner style text won't get updated until `CSS.setStyleText` returns. We should test this.

Nikita Vasilyev

Comment 9 2018-12-07 14:35:07 PST

(In reply to Devin Rousso from comment #8) > Comment on attachment 356785 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=356785&action=review > > r-, due to the potential double agent call Yes, two calls may happen but only when all these conditions meet: - when adding a new property, - appending it after the last property, - the last property isn't terminated with a semicolon. This is such a rare case that I prefer to keep the code simple rather than trying to avoid one backend call. >> Source/WebInspectorUI/ChangeLog:24 >> + /* color: red; */ font-size: 12px > > I'm not sure if this is just how you formatted this, but shouldn't we be keeping the newline as well? You're right, this was confusing.

Nikita Vasilyev

Comment 10 2018-12-07 16:17:24 PST

Comment on attachment 356785 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356785&action=review >> LayoutTests/inspector/css/modify-css-property.html:148 >> + let styleDeclaration = getMatchedStyleDeclaration(); > > This should fail in the case that no style is found (would also require a `return null;` inside `getMatchedStyleDeclaration`). > > if (!styleDeclaration) { > InspectorTest.fail("No style declaration found."); > resolve(); > return; > } We are in control of this page. Why would it not be found? >> LayoutTests/inspector/css/modify-css-property.html:154 >> + getProperty("padding-right").commentOut(disabled); > > Ditto (>148). > > let property = getProperty("padding-right"); > if (!property) { > InspectorTest.fail("No property found."); > resolve(); > return; > } Ditto. I'm concerned adding this after every `getProperty` call would only make tests less readable. >>> Source/WebInspectorUI/ChangeLog:24 >>> + /* color: red; */ font-size: 12px >> >> I'm not sure if this is just how you formatted this, but shouldn't we be keeping the newline as well? > > You're right, this was confusing. We used to strip whitespace, including line breaks. Now it's preserved!

Nikita Vasilyev

Comment 11 2018-12-07 16:18:06 PST

Created attachment 356850 [details] Patch I added more tests.

Nikita Vasilyev

Comment 12 2018-12-08 15:38:44 PST

Created attachment 356890 [details] WIP: single agent call I (In reply to Nikita Vasilyev from comment #9) > (In reply to Devin Rousso from comment #8) > > Comment on attachment 356785 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=356785&action=review > > > > r-, due to the potential double agent call > > Yes, two calls may happen but only when all these conditions meet: > - when adding a new property, > - appending it after the last property, > - the last property isn't terminated with a semicolon. I tried doing this in one call. The WIP is attached. I'm not going forward with this approach. The problem in the first place wasn't multiplying semicolons alone. That wouldn't cause data corruption. The data corruption was caused because the semicolons were a special case that didn't update ranges of the following properties. I don't want to make logic harder to follow. It's premature optimization. I'll add tests for inserting new properties, which were long overdue, and I'll continue with my previous approach.

Nikita Vasilyev

Comment 13 2018-12-08 16:00:26 PST

Created attachment 356891 [details] Patch

Devin Rousso

Comment 14 2018-12-12 19:15:14 PST

Comment on attachment 356891 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356891&action=review r+, but I want to see tests as described in my comments before this lands > LayoutTests/inspector/css/add-css-property.html:9 > + let suite = InspectorTest.createAsyncSuite("AddCSSProperty"); Is there a reason this needs to be an async test suite? It seems like all test code is synchronous, so we could use a sync suite. > LayoutTests/inspector/css/add-css-property.html:21 > + reject(); NIT: although this might seem counterintuitive, I'd prefer that you call `resolve()` here instead, as calling `reject()` will prevent any test cases after this one from running (it effectively kills the test chain, as none of the cases have `catch` handlers for their associated promises, so a single rejection in any test case basically aborts the entire run). If a problem is "localized" to a specific test case, we shouldn't prevent the running of subsequent tests as a result. > LayoutTests/inspector/css/add-css-property.html:25 > + styleDeclaration.locked = true; What would happen in the case that the style isn't `locked`? Is that possible? We should have some tests for this. Interestingly, both `set name` and `set rawValue` call through to `_ownerStyle.set text`, which may cause race-y things to happen. We should test what happens when `locked === false`, as in that case the cached `_ownerStyle._text` isn't immediately updated (it waits for the agent call to return with the "real" value and uses that). > LayoutTests/inspector/css/add-css-property.html:28 > + newProperty.rawValue = "green"; I noticed that there's a FIXME inside `WI.CSSProperty`: // FIXME: Remove current value getter and rename rawValue to value once the old styles sidebar is removed. Perhaps now (or a followup in the immediate future) is that time? > LayoutTests/inspector/css/add-css-property.html:37 > + name: "AddCSSProperty.InsertBeforeAndAfter", There should also be a test for "InsertBetween". > LayoutTests/inspector/css/add-css-property.html:39 > + let getMatchedStyleDeclaration = () => { Rather than repeat this helper function inside each test case, you should make one that accepts a selector as an argument (as well as the resolve/reject objects): function getMatchedStyle(selector, resolve) { for (let rule of nodeStyles.matchedRules) { if (rule.selectorText === selector) return rule.style; } InspectorTest.fail("No style found for selector " + selector); resolve(); } > LayoutTests/inspector/css/add-css-property.html:63 > + expected = `color: green;\n outline: 2px solid brown;display: block;\n`; So this patch doesn't try to add a prefix newline/spacing based on the previous line's spacing? I thought that that was part of this change? If so, that should be a followup. > LayoutTests/inspector/css/modify-css-property.html:175 > + let expectedStyleText = ` > + /* padding-left: 2em; */ > + padding-right: 0px; > + `; NIT: can you shift these to use "\n" like the other test file? > LayoutTests/inspector/css/modify-css-property.html:196 > + name: "ModifyCSSProperty.CommentOutAndUncommentProperty2", This could use a better name (e.g. "ModifyCSSProperty.CommentOutAndUncommentPropertyWithoutNewlines", and the previous test case would be "ModifyCSSProperty.CommentOutAndUncommentPropertyWithNewlines"). > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:401 > + property.text = property.text.trimRight() + ";" + match[1]; This still worries me, as we'd potentially have race-y behavior if the `_ownerStyle` isn't locked when the text is changed. I agree that it's a rare case, so I'm ok with this moving forward, but adding some tests for this situation (and others like it) would definitely help.

Nikita Vasilyev

Comment 15 2018-12-15 01:21:20 PST

Created attachment 357397 [details] Patch

Nikita Vasilyev

Comment 16 2018-12-15 01:39:56 PST

(In reply to Devin Rousso from comment #14) > > LayoutTests/inspector/css/add-css-property.html:25 > > + styleDeclaration.locked = true; > > What would happen in the case that the style isn't `locked`? Is that > possible? We should have some tests for this. > > Interestingly, both `set name` and `set rawValue` call through to > `_ownerStyle.set text`, which may cause race-y things to happen. We should > test what happens when `locked === false`, as in that case the cached > `_ownerStyle._text` isn't immediately updated (it waits for the agent call > to return with the "real" value and uses that). So far I added asserts. I don't think editing of unlocked style declarations makes any sense. I think it shouldn't be impossible: Bug 192739 - Web Inspector: Styles: prevent potential data corruption by ensuring style declarations are locked when editing

WebKit Commit Bot

Comment 17 2018-12-15 02:03:27 PST

Comment on attachment 357397 [details] Patch Clearing flags on attachment: 357397 Committed r239251: <https://trac.webkit.org/changeset/239251>

WebKit Commit Bot

Comment 18 2018-12-15 02:03:29 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.