Summary: | Web Inspector: Styles: fix race conditions when editing | ||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
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, simon.fraser, webkit-bug-importer | ||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Nikita Vasilyev
2018-12-15 01:34:45 PST
Created attachment 358205 [details]
Patch
cq- because I need to update tests.
Created attachment 358212 [details]
[Video] Changing style attribute
Note that this patch doesn't make the existing locking obsolete! Locking still makes sense when the edited CSS is being modified by web page's JS.
Take a look at the video. Notice how the styles update in the sidebar. When the properties are focused in the sidebar, any external updates are ignored and you could still edit.
Comment on attachment 358205 [details] Patch Attachment 358205 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10608821 New failing tests: inspector/css/modify-css-property.html Created attachment 358216 [details]
Archive of layout-test-results from ews102 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 358205 [details] Patch Attachment 358205 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10608868 New failing tests: inspector/css/modify-css-property.html Created attachment 358217 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 358205 [details] Patch Attachment 358205 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/10608913 New failing tests: inspector/css/modify-css-property.html Created attachment 358219 [details]
Archive of layout-test-results from ews117 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 358447 [details]
Patch
Comment on attachment 358447 [details] Patch Attachment 358447 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10644638 New failing tests: inspector/css/add-css-property.html Created attachment 358450 [details]
Archive of layout-test-results from ews101 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 358447 [details] Patch Attachment 358447 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10644646 New failing tests: inspector/css/add-css-property.html Created attachment 358451 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 358447 [details] Patch Attachment 358447 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/10644686 New failing tests: inspector/css/add-css-property.html Created attachment 358453 [details]
Archive of layout-test-results from ews113 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 358466 [details]
Patch
Comment on attachment 358466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358466&action=review I gotta say, attachment 358212 [details] looks magical 😍 It might be worth mentioning in comment/ChangeLog that this change is purely in the frontend, meaning that the backend can update as much as it wants, and the count wouldn't ever update until the frontend does something. I spent a little while reading through the code trying to figure out where `_updatesInProgressCount` gets updated by a change coming from the backend before I realized that. > LayoutTests/inspector/css/modify-css-property.html:121 > + .then((event) => { Style: we don't usually indent `.then()`. > LayoutTests/inspector/css/modify-css-property.html:161 > .then((event) => { Ditto (>121). > Source/WebInspectorUI/ChangeLog:20 > + Unsure there's no race conditions by introducing `_updatesInProgressCount`: Is there any way to "make sure" of this? Perhaps some tests that do a `requestAnimationFrame` loop with you changing values in the frontend while that happens? > Source/WebInspectorUI/ChangeLog:33 > + Change the setter to a method since it has side effects including an asynchronous backend call. This isn't part of our style guide AFAIK. We have getters/setters with side effects in other places (e.g. `WI.CSSRule.prototype.set selectorText`). > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:390 > + // This causes a backend call which is asynchronous yet changes in the model on the front-end are synchronous. I'm not sure what "purpose" this comment serves. Is it trying to explain something? Should readers of `WI.CSSProperty` know about the implementation details of `WI.CSSStyleDeclaration`? > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:106 > + let forceUpdate = options.forceUpdate || false; This extra check is unnecessary. If `forceUpdate` wasn't supplied, it would already be falsy. You can just use `options.forceUpdate` everywhere instead. > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:108 > + // When two consequent setText calls happen (A and B), only update when the last call (B) is finished. I don't think this needs to be both in the ChangeLog and the code. I'd put it in one or the other. > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:232 > + this._updatesInProgressCount++; Style: `++this.__updatesInProgressCount;` > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:240 > + this._updatesInProgressCount--; Style: `--this.__updatesInProgressCount;` > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:241 > + console.assert(this._updatesInProgressCount >= 0, "_updatesInProgressCount cannot be a negative number."); Should we actually constrain `_updatesInProgressCount` in the case that this somehow get's wrong? > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:244 > + let timeoutId = setTimeout(() => { NIT: this should be defined above `styleTextDidChange` for visibility. > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:247 > + }, 1000); NIT: is this "too long" to wait? I know this is only for error-handling, but I'm curious as to the actual "time" it would take to trigger this? > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:467 > + this.refresh().finally(callback); NIT: you could just have a `.then()`, since there's no way for it to reject. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:-79 > - get nameTextField() { return this._nameTextField; } > - get valueTextField() { return this._valueTextField; } These should either be undone or added to the ChangeLog. Comment on attachment 358466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358466&action=review >> Source/WebInspectorUI/ChangeLog:20 >> + Unsure there's no race conditions by introducing `_updatesInProgressCount`: > > Is there any way to "make sure" of this? Perhaps some tests that do a `requestAnimationFrame` loop with you changing values in the frontend while that happens? I think this test would likely be flaky but I'll add a test with changing values in a loop. >> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:232 >> + this._updatesInProgressCount++; > > Style: `++this.__updatesInProgressCount;` Why do you want to use two underscores? >> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:247 >> + }, 1000); > > NIT: is this "too long" to wait? I know this is only for error-handling, but I'm curious as to the actual "time" it would take to trigger this? Using MacBook Pro 2017: On https://www.apple.com, most updates take 90-120ms. On https://stackoverflow.com, most updates take 1500-3000ms and Web Inspector is barely usable 😱 I think I'll change the timer to be 2 seconds. Created attachment 358857 [details]
Patch
Comment on attachment 358466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358466&action=review >>> Source/WebInspectorUI/ChangeLog:20 >>> + Unsure there's no race conditions by introducing `_updatesInProgressCount`: >> >> Is there any way to "make sure" of this? Perhaps some tests that do a `requestAnimationFrame` loop with you changing values in the frontend while that happens? > > I think this test would likely be flaky but I'll add a test with changing values in a loop. You can take a look at the "inspector/canvas/recording-*" tests to see how you may want to synchronize various `requestAnimationFrame` loops. It could probably be something like: 1. make a change to the style 2. start a `requestAnimationFrame` loop (maybe increment some number?) - every time the `requestAnimationFrame` finishes, fire an event to the frontend, maybe with the current value as `data` 3. after the next `requestAnimationFrame` event, try to change the style in the frontend 4. wait till the next `requestAnimationFrame` and check that the number incremented more than what you set it to be 5. stop the `requestAnimationFrame` loop >>> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:232 >>> + this._updatesInProgressCount++; >> >> Style: `++this.__updatesInProgressCount;` > > Why do you want to use two underscores? Typo :P >>> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:247 >>> + }, 1000); >> >> NIT: is this "too long" to wait? I know this is only for error-handling, but I'm curious as to the actual "time" it would take to trigger this? > > Using MacBook Pro 2017: > On https://www.apple.com, most updates take 90-120ms. > On https://stackoverflow.com, most updates take 1500-3000ms and Web Inspector is barely usable 😱 > > I think I'll change the timer to be 2 seconds. Woah, any idea why <https://stackoverflow.com> takes so long? Can you file a new bug about that? (In reply to Devin Rousso from comment #21) > Comment on attachment 358466 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=358466&action=review > > >>> Source/WebInspectorUI/ChangeLog:20 > >>> + Unsure there's no race conditions by introducing `_updatesInProgressCount`: > >> > >> Is there any way to "make sure" of this? Perhaps some tests that do a `requestAnimationFrame` loop with you changing values in the frontend while that happens? > > > > I think this test would likely be flaky but I'll add a test with changing values in a loop. > > You can take a look at the "inspector/canvas/recording-*" tests to see how > you may want to synchronize various `requestAnimationFrame` loops. Where should I look, exactly? I don't see any requestAnimationFrame calls in LayoutTests/inspector/canvas/ at all. > > Using MacBook Pro 2017: > > On https://www.apple.com, most updates take 90-120ms. > > On https://stackoverflow.com, most updates take 1500-3000ms and Web Inspector is barely usable 😱 > > > > I think I'll change the timer to be 2 seconds. > > Woah, any idea why <https://stackoverflow.com> takes so long? Can you file > a new bug about that? Bug 193367: Web Inspector: Elements tab is slow on stackoverflow.com (In reply to Nikita Vasilyev from comment #22) > Where should I look, exactly? I don't see any requestAnimationFrame calls in LayoutTests/inspector/canvas/ at all. Ah, whoops. Those tests use a `setTimeout(..., 0)` loop. inspector/canvas/recording-2d.html and inspector/canvas/resources/recording-utilites.js should have what you're looking for. I outlined an idea of a test that might work in comment #21. You could do something "fake" like increment the element's `z-index` based on the previous `z-index` value, so that when the frontend changes the value (it could set it to -100 or something) you could check that the value has changed significantly from what it was before (by comparing the values sent to the frontend by the `TestPage.dispatchEventToFrontend` calls. Created attachment 360990 [details]
Patch
Comment on attachment 360990 [details] Patch Attachment 360990 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11011368 New failing tests: inspector/css/modify-css-property-race.html Created attachment 360994 [details]
Archive of layout-test-results from ews113 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-highsierra Platform: Mac OS X 10.13.6
Seems like a flacky test :( Created attachment 361001 [details]
Patch
Comment on attachment 361001 [details] Patch Attachment 361001 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11014138 New failing tests: inspector/css/modify-css-property-race.html Created attachment 361007 [details]
Archive of layout-test-results from ews113 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 361008 [details]
Patch
Comment on attachment 361008 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361008&action=review r=me, thanks for adding the extra tests. I think there's a few tweaks to be made, but its basically there 😁 Please address them before landing. Awesome work! > LayoutTests/inspector/css/modify-css-property-race.html:8 > +function expand(iterationCount) { Is there a reason to add this maximum count? I feel like on slower debug builds, this may cause problems. I think this logic would still work even if you let it continue rAF-looping infinitely. > LayoutTests/inspector/css/modify-css-property-race.html:30 > + TestPage.dispatchEventToFrontend("TestPage-didStopExpanding"); This isn't used anywhere. Given my comments below, I think we can remove it. > LayoutTests/inspector/css/modify-css-property-race.html:71 > + if (valueModifiedByWebInspector) { We should wait for one more non-inspector-modification update to make sure that we haven't "ruined" the iteration cycle. Rather than keeping a boolean `valueModifiedByWebInspector`, we could keep a `updatedCount` and do different things depending on the value (e.g. `0` is the current `else`, `1` is the current `if`, and `2` would do the same as the `if` but instead check that the value is > 10 and would then end the test). > LayoutTests/inspector/css/modify-css-property-race.html:79 > + InspectorTest.log("First style change."); We should assert (or even `expectGreaterThan`) that the current value is > 42, just to make sure that the `expand` is working as expected. > LayoutTests/inspector/css/modify-css-property.html:127 > + // WI.CSSStyleDeclaration.Event.PropertiesChanged event should not fire when the style declaration is locked. Along these lines, we should `assert` in the event listener that `!styleDeclaration.locked` just to make sure we aren't firing at the wrong time. > LayoutTests/inspector/css/modify-css-property.html:159 > + styleDeclaration.awaitEvent(WI.CSSStyleDeclaration.Event.PropertiesChanged).then((event) => { One issue with using `awaitEvent` is that there isn't a good way to remove the added event listener, and we will want to remove this event listener before the next test so that each case is "independent". I think this would be better off using `singleFireEventListener` (as well as `removeEventListener). > LayoutTests/inspector/css/modify-css-property.html:176 > + resolve(); Putting `resolve` here won't give the event listener above a chance to fire. I think we need to wait for the `PropertiesChanged` event to actually fire (expected) before finishing the test. > LayoutTests/inspector/css/modify-css-property.html:203 > + styleDeclaration.locked = false; Would there be any different of a result if `locked` was set to `true`? I'd imagine that this wouldn't be different, as `locked` should only affect backend updates to the frontend. Comment on attachment 361008 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361008&action=review >> LayoutTests/inspector/css/modify-css-property.html:159 >> + styleDeclaration.awaitEvent(WI.CSSStyleDeclaration.Event.PropertiesChanged).then((event) => { > > One issue with using `awaitEvent` is that there isn't a good way to remove the added event listener, and we will want to remove this event listener before the next test so that each case is "independent". I think this would be better off using `singleFireEventListener` (as well as `removeEventListener). Seems like you stepped on the same rake as I did! `awaitEvent` is the same as `singleFireEventListener`: static awaitEvent(eventType) { let wrapper = new WI.WrappedPromise; this.singleFireEventListener(eventType, (event) => wrapper.resolve(event)); return wrapper.promise; } I think we should rename `awaitEvent`. >> LayoutTests/inspector/css/modify-css-property.html:176 >> + resolve(); > > Putting `resolve` here won't give the event listener above a chance to fire. I think we need to wait for the `PropertiesChanged` event to actually fire (expected) before finishing the test. Good point, this test is faulty. >> LayoutTests/inspector/css/modify-css-property.html:203 >> + styleDeclaration.locked = false; > > Would there be any different of a result if `locked` was set to `true`? I'd imagine that this wouldn't be different, as `locked` should only affect backend updates to the frontend. It wouldn't with this patch, but it would before. Comment on attachment 361008 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361008&action=review >>> LayoutTests/inspector/css/modify-css-property.html:159 >>> + styleDeclaration.awaitEvent(WI.CSSStyleDeclaration.Event.PropertiesChanged).then((event) => { >> >> One issue with using `awaitEvent` is that there isn't a good way to remove the added event listener, and we will want to remove this event listener before the next test so that each case is "independent". I think this would be better off using `singleFireEventListener` (as well as `removeEventListener). > > Seems like you stepped on the same rake as I did! `awaitEvent` is the same as `singleFireEventListener`: > > static awaitEvent(eventType) > { > let wrapper = new WI.WrappedPromise; > this.singleFireEventListener(eventType, (event) => wrapper.resolve(event)); > return wrapper.promise; > } > > I think we should rename `awaitEvent`. Re-reading your comment, it looks like you knew that `awaitEvent` listens for an event only once. Anyway, I wonder how it didn't affect other tests. Comment on attachment 361008 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361008&action=review >>>> LayoutTests/inspector/css/modify-css-property.html:159 >>>> + styleDeclaration.awaitEvent(WI.CSSStyleDeclaration.Event.PropertiesChanged).then((event) => { >>> >>> One issue with using `awaitEvent` is that there isn't a good way to remove the added event listener, and we will want to remove this event listener before the next test so that each case is "independent". I think this would be better off using `singleFireEventListener` (as well as `removeEventListener). >> >> Seems like you stepped on the same rake as I did! `awaitEvent` is the same as `singleFireEventListener`: >> >> static awaitEvent(eventType) >> { >> let wrapper = new WI.WrappedPromise; >> this.singleFireEventListener(eventType, (event) => wrapper.resolve(event)); >> return wrapper.promise; >> } >> >> I think we should rename `awaitEvent`. > > Re-reading your comment, it looks like you knew that `awaitEvent` listens for an event only once. > > Anyway, I wonder how it didn't affect other tests. Yes, `awaitEvent` is effectively a "promisified" version of `singleFireEventListener`. When I write tests, I typically try to remove any event listeners I've added in that test, so if I added an event listener that I was expecting _not_ to be fired, I would use `singleFireEventListener` instead. Created attachment 361104 [details]
Patch
Comment on attachment 361104 [details] Patch Clearing flags on attachment: 361104 Committed r240946: <https://trac.webkit.org/changeset/240946> All reviewed patches have been landed. Closing bug. The test is failing on EWS: https://webkit-queues.webkit.org/results/11035028 [1/1] inspector/css/modify-css-property-race.html failed unexpectedly (text diff) Comment on attachment 361104 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361104&action=review > LayoutTests/inspector/css/modify-css-property-race.html:69 > + InspectorTest.expectGreaterThan(valueNumber, 42); It's generally a very bad idea to log exact values like this, because it's always possible that Web Inspector is slow to open/dispatch and may miss a step (or two). Adding a message (3rd argument) will ensure that the value never gets logged, and provide some much needed context in the results file. > LayoutTests/inspector/css/modify-css-property-race.html:72 > + InspectorTest.expectGreaterThan(valueNumber, 42); Ditto (69). > LayoutTests/inspector/css/modify-css-property-race.html:79 > + InspectorTest.expectGreaterThanOrEqual(valueNumber, 10); Ditto (69). (In reply to Simon Fraser (smfr) from comment #39) > The test is failing on EWS: > https://webkit-queues.webkit.org/results/11035028 > [1/1] inspector/css/modify-css-property-race.html failed unexpectedly (text > diff) Where do I see layout test results? E.g. the diff of expected/actual. It's been a while. (In reply to Devin Rousso from comment #40) > Comment on attachment 361104 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=361104&action=review > > > LayoutTests/inspector/css/modify-css-property-race.html:69 > > + InspectorTest.expectGreaterThan(valueNumber, 42); > > It's generally a very bad idea to log exact values like this, because it's > always possible that Web Inspector is slow to open/dispatch and may miss a > step (or two). Adding a message (3rd argument) will ensure that the value > never gets logged, and provide some much needed context in the results file. > > > LayoutTests/inspector/css/modify-css-property-race.html:72 > > + InspectorTest.expectGreaterThan(valueNumber, 42); > > Ditto (69). > > > LayoutTests/inspector/css/modify-css-property-race.html:79 > > + InspectorTest.expectGreaterThanOrEqual(valueNumber, 10); > > Ditto (69). I see what I did 🤦♂️ |