Bug 192739 - Web Inspector: Styles: fix race conditions when editing
Summary: Web Inspector: Styles: fix race conditions when editing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-12-15 01:34 PST by Nikita Vasilyev
Modified: 2019-02-05 00:26 PST (History)
7 users (show)

See Also:


Attachments
Patch (7.66 KB, patch)
2019-01-02 14:18 PST, Nikita Vasilyev
nvasilyev: review-
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
[Video] Changing style attribute (6.82 MB, video/quicktime)
2019-01-02 15:11 PST, Nikita Vasilyev
no flags Details
Archive of layout-test-results from ews102 for mac-sierra (2.42 MB, application/zip)
2019-01-02 15:21 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.11 MB, application/zip)
2019-01-02 15:34 PST, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-sierra (1.91 MB, application/zip)
2019-01-02 16:08 PST, Build Bot
no flags Details
Patch (15.04 KB, patch)
2019-01-05 17:33 PST, Nikita Vasilyev
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.44 MB, application/zip)
2019-01-05 18:35 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.25 MB, application/zip)
2019-01-05 18:46 PST, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-sierra (1.97 MB, application/zip)
2019-01-05 19:26 PST, Build Bot
no flags Details
Patch (15.90 KB, patch)
2019-01-06 12:34 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch (16.23 KB, patch)
2019-01-10 18:02 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch (21.32 KB, patch)
2019-02-02 18:47 PST, Nikita Vasilyev
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-highsierra (2.03 MB, application/zip)
2019-02-02 20:29 PST, Build Bot
no flags Details
Patch (21.50 KB, patch)
2019-02-03 01:55 PST, Nikita Vasilyev
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-highsierra (2.04 MB, application/zip)
2019-02-03 03:38 PST, Build Bot
no flags Details
Patch (20.94 KB, patch)
2019-02-03 04:01 PST, Nikita Vasilyev
drousso: review+
drousso: commit-queue-
Details | Formatted Diff | Diff
Patch (21.89 KB, patch)
2019-02-04 14:59 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2018-12-15 01:34:45 PST
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.
Comment 1 Radar WebKit Bug Importer 2018-12-15 01:35:22 PST
<rdar://problem/46752925>
Comment 2 Nikita Vasilyev 2019-01-02 14:18:03 PST
Created attachment 358205 [details]
Patch

cq- because I need to update tests.
Comment 3 Nikita Vasilyev 2019-01-02 15:11:29 PST
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 4 Build Bot 2019-01-02 15:20:58 PST
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
Comment 5 Build Bot 2019-01-02 15:21:00 PST
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 6 Build Bot 2019-01-02 15:34:29 PST
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
Comment 7 Build Bot 2019-01-02 15:34:31 PST
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 8 Build Bot 2019-01-02 16:08:53 PST
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
Comment 9 Build Bot 2019-01-02 16:08:54 PST
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
Comment 10 Nikita Vasilyev 2019-01-05 17:33:16 PST
Created attachment 358447 [details]
Patch
Comment 11 Build Bot 2019-01-05 18:35:10 PST
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
Comment 12 Build Bot 2019-01-05 18:35:12 PST
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 13 Build Bot 2019-01-05 18:46:45 PST
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
Comment 14 Build Bot 2019-01-05 18:46:47 PST
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 15 Build Bot 2019-01-05 19:26:29 PST
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
Comment 16 Build Bot 2019-01-05 19:26:31 PST
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
Comment 17 Nikita Vasilyev 2019-01-06 12:34:49 PST
Created attachment 358466 [details]
Patch
Comment 18 Devin Rousso 2019-01-08 19:52:30 PST
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 19 Nikita Vasilyev 2019-01-10 17:50:59 PST
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.
Comment 20 Nikita Vasilyev 2019-01-10 18:02:23 PST
Created attachment 358857 [details]
Patch
Comment 21 Devin Rousso 2019-01-10 18:53:47 PST
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?
Comment 22 Nikita Vasilyev 2019-01-14 11:55:29 PST
(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
Comment 23 Devin Rousso 2019-01-14 12:11:17 PST
(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.
Comment 24 Nikita Vasilyev 2019-02-02 18:47:26 PST Comment hidden (obsolete)
Comment 25 Build Bot 2019-02-02 20:29:34 PST Comment hidden (obsolete)
Comment 26 Build Bot 2019-02-02 20:29:36 PST Comment hidden (obsolete)
Comment 27 Nikita Vasilyev 2019-02-03 00:51:46 PST
Seems like a flacky test :(
Comment 28 Nikita Vasilyev 2019-02-03 01:55:13 PST
Created attachment 361001 [details]
Patch
Comment 29 Build Bot 2019-02-03 03:38:21 PST
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
Comment 30 Build Bot 2019-02-03 03:38:23 PST Comment hidden (obsolete)
Comment 31 Nikita Vasilyev 2019-02-03 04:01:56 PST
Created attachment 361008 [details]
Patch
Comment 32 Devin Rousso 2019-02-04 08:46:24 PST
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 33 Nikita Vasilyev 2019-02-04 14:33:12 PST
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 34 Nikita Vasilyev 2019-02-04 14:39:22 PST
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 35 Devin Rousso 2019-02-04 14:41:08 PST
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.
Comment 36 Nikita Vasilyev 2019-02-04 14:59:29 PST
Created attachment 361104 [details]
Patch
Comment 37 WebKit Commit Bot 2019-02-04 15:30:55 PST
Comment on attachment 361104 [details]
Patch

Clearing flags on attachment: 361104

Committed r240946: <https://trac.webkit.org/changeset/240946>
Comment 38 WebKit Commit Bot 2019-02-04 15:30:57 PST
All reviewed patches have been landed.  Closing bug.
Comment 39 Simon Fraser (smfr) 2019-02-04 20:40:17 PST
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 40 Devin Rousso 2019-02-04 20:44:34 PST
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).
Comment 41 Nikita Vasilyev 2019-02-05 00:03:37 PST
(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.
Comment 42 Nikita Vasilyev 2019-02-05 00:26:59 PST
(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 🤦‍♂️