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: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Attachments: |
|
Description
Nikita Vasilyev
2018-12-04 23:06:20 PST
This is the same bug as shown on https://bugs.webkit.org/show_bug.cgi?id=192282#c3. Created attachment 356761 [details]
Patch
Created attachment 356785 [details]
Patch
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 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
Comment on attachment 356785 [details]
Patch
Unrelated test failures.
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. (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. 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! Created attachment 356850 [details]
Patch
I added more tests.
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. Created attachment 356891 [details]
Patch
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. Created attachment 357397 [details]
Patch
(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 Comment on attachment 357397 [details] Patch Clearing flags on attachment: 357397 Committed r239251: <https://trac.webkit.org/changeset/239251> All reviewed patches have been landed. Closing bug. |