Bug 192739 – Web Inspector: Styles: fix race conditions when editing

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Editing unlocked styles results in data corruption*. We should lock style declarations right before modifying them (editing name/value, toggling, or removing).

*: In most cases. Exceptions are:
   - modifying value of the last property in a style declaration.
   - removing the last property in a style declaration.
   - making a change where the new text is the same length is the old text.

[Radar WebKit Bug Importer]

<rdar://problem/46752925>

[Nikita Vasilyev]

Created attachment 358205 [details]
Patch

cq- because I need to update tests.

[Nikita Vasilyev]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Nikita Vasilyev]

Created attachment 358447 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Nikita Vasilyev]

Created attachment 358466 [details]
Patch

[Devin Rousso]

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.

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

Created attachment 358857 [details]
Patch

[Devin Rousso]

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?

[Nikita Vasilyev]

(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

[Devin Rousso]

(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.

[Nikita Vasilyev]

Created attachment 360990 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Nikita Vasilyev]

Seems like a flacky test :(

[Nikita Vasilyev]

Created attachment 361001 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Nikita Vasilyev]

Created attachment 361008 [details]
Patch

[Devin Rousso]

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.

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

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.

[Devin Rousso]

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.

[Nikita Vasilyev]

Created attachment 361104 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 361104 [details]
Patch

Clearing flags on attachment: 361104

Committed r240946: <https://trac.webkit.org/changeset/240946>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

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)

[Devin Rousso]

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).

[Nikita Vasilyev]

(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.

[Nikita Vasilyev]

(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 🤦‍♂️