Bug 179461 - Web Inspector: Styles Redesign: data corruption when updating values quickly
Summary: Web Inspector: Styles Redesign: data corruption when updating values quickly
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: 2017-11-08 18:14 PST by Nikita Vasilyev
Modified: 2018-01-22 16:52 PST (History)
5 users (show)

See Also:


Attachments
Patch (11.95 KB, patch)
2017-12-09 16:23 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch (debug) (13.15 KB, patch)
2017-12-09 16:53 PST, Nikita Vasilyev
nvasilyev: review-
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
[Animated GIF] With debug patch applied (12.39 KB, text/plain)
2017-12-09 16:56 PST, Nikita Vasilyev
no flags Details
[Animated GIF] With debug patch applied (281.70 KB, image/gif)
2017-12-09 16:58 PST, Nikita Vasilyev
no flags Details
Patch for review (12.39 KB, patch)
2017-12-09 16:59 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch for review (12.42 KB, patch)
2017-12-19 18:42 PST, Nikita Vasilyev
drousso: review-
Details | Formatted Diff | Diff
Patch for review (21.51 KB, patch)
2018-01-12 17:25 PST, Nikita Vasilyev
joepeck: review+
Details | Formatted Diff | Diff
Patch for landing (21.63 KB, patch)
2018-01-22 15:56 PST, Nikita Vasilyev
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (21.63 KB, patch)
2018-01-22 16:00 PST, Nikita Vasilyev
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (22.35 KB, patch)
2018-01-22 16:29 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 2017-11-08 18:14:22 PST
When updating values quickly, sometimes data gets corrupted because of a race condition.

Steps:
1. Open http://nv.github.io/webkit-inspector-bugs/styles-redesign/tests/color.html
2. Inspect "item #1"
3. Click on the inline swatch before "whitesmoke" to display color picker.
4. Click to pick a new color and drag a cursor to cause a burst of updates.

Expected:
Colors update ASAP and no exceptions are thrown.

Actual:
[Error] Assertion Failed: _styleSheetTextRange data is invalid.
	_updateOwnerStyleText (CSSProperty.js:362)
	_updateStyleText (CSSProperty.js:339)
	rawValue (CSSProperty.js:212)
	_handleValueChange (SpreadsheetStyleProperty.js:462)
	(anonymous function) (SpreadsheetStyleProperty.js:321)
	dispatch (Object.js:170)
	dispatchEventToListeners (Object.js:177)
	_updateSwatch (InlineSwatch.js:141)
	_valueEditorValueDidChange (InlineSwatch.js:262)
	dispatch (Object.js:170)
	dispatchEventToListeners (Object.js:177)
	_updateColor (ColorPicker.js:202)
	colorWheelColorDidChange (ColorPicker.js:163)
	_updateColorForMouseEvent (ColorWheel.js:180)
	_handleMousemove (ColorWheel.js:137)
	handleEvent (ColorWheel.js:117)

NOTES:

WI.CSSStyleDeclaration.prototype.text setter is asynchronous.

    cssStyleDeclaration.text //"color: red;"
    cssStyleDeclaration.text = "color: green;"
    cssStyleDeclaration.text //"color: red;"

WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object
{
    ...
    set text(text)
    {
        ...
        this._nodeStyles.changeStyleText(this, text);
    }

this._nodeStyles.changeStyleText sends the text to the backend. cssStyleDeclaration.text gets updated only after getting a response back from the backend.
Comment 1 Radar WebKit Bug Importer 2017-11-08 18:14:36 PST
<rdar://problem/35431882>
Comment 2 Nikita Vasilyev 2017-11-08 18:21:19 PST
This wasn't a problem in the old styles sidebar because it treated text in the view (CodeMirror instance) as the source of truth.
Comment 3 Nikita Vasilyev 2017-11-21 17:29:38 PST
One way of fixing this would be to ignore WI.CSSStyleDeclaration updates from the backend when its corresponding view is being edited (e.g. one of CSS properties are in focus).

WI.CSSStyleDeclaration gets updates in the following scenarios:
1. CSS edited in Styles panel.
2. CSS edited in Resources tab.
3. CSS modified with JS.

This strategy should work well with (1) and (2), but not with (3). Since we currently don't receive updates from the backend when CSSOM changes are made anyway, I think it's an okay strategy.

Bug 92644 - Web Inspector: CSS rule changes made via CSSStyleSheet DOM API are not reflected in the Styles pane
Bug 141450 - Web Inspector: Better support for CSSOM StyleSheet mutations (insertRule/deleteRule)
Comment 4 Nikita Vasilyev 2017-12-01 19:11:32 PST
Locking (ignoring WI.CSSStyleDeclaration updates from the backend while it's being edited) has issues: the front-end doesn't know if a newly added CSS property is valid or overridden.

Back to the drawing board :(
Comment 5 Nikita Vasilyev 2017-12-09 16:23:45 PST
Created attachment 328921 [details]
Patch
Comment 6 Nikita Vasilyev 2017-12-09 16:53:47 PST
Created attachment 328922 [details]
Patch (debug)
Comment 7 Nikita Vasilyev 2017-12-09 16:56:38 PST
Created attachment 328923 [details]
[Animated GIF] With debug patch applied

The debug patch highlights SpreadsheetCSSStyleDeclarationEditor with pink background when CSSStyleDeclaration is locked.

Otherwise, it's exactly like the patch for review.
Comment 8 Nikita Vasilyev 2017-12-09 16:58:10 PST
Created attachment 328924 [details]
[Animated GIF] With debug patch applied

The debug patch highlights SpreadsheetCSSStyleDeclarationEditor with pink background when CSSStyleDeclaration is locked.

Otherwise, it's exactly like the patch for review.
Comment 9 Nikita Vasilyev 2017-12-09 16:59:54 PST
Created attachment 328926 [details]
Patch for review
Comment 10 Nikita Vasilyev 2017-12-19 18:42:21 PST
Created attachment 329870 [details]
Patch for review

Rebaselined.
Comment 11 Devin Rousso 2017-12-19 19:41:55 PST
Comment on attachment 329870 [details]
Patch for review

View in context: https://bugs.webkit.org/attachment.cgi?id=329870&action=review

r-, only because this definitely needs some tests.  The logic otherwise sounds fine, but having tests (specifically on CSSStyleDeclaration) would really help clarify what will happen in the various cases:

 - not locked, style changes via page's JS
 - locked, style changes via page's JS
 - locked, style changes via WebInspector
 - locked, style changes via WebInspector and page's JS (both orderings)
 - etc.

It should be pretty simple to verify just by checking the style's text.

> Source/WebInspectorUI/ChangeLog:20
> +        To correctly display invalid and overridden properties, the backend allowed to update

I think you're missing a word in this sentence, like "is".

> Source/WebInspectorUI/ChangeLog:50
> +        When selector is focused, clicking on the white-space should not add a new blank property.

I'd move this comment after the final item for this file, so it's a bit easier to read.

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:105
> +    update(text, properties, styleSheetTextRange, dontFireEvents, suppressLock)

It'd be nice of we could move some of these "optional" properties to an object parameter,

    update(text, properties, styleSheetTextRange, options = {})

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:247
> +    // spreadsheetStyleProperty delegate

NIT: SpreadsheetStyleProperty

Also, these should probably go underneath the "Public" section.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:354
> +        if (this._delegate && typeof this._delegate.stylePropertyInlineSwatchActivated === "function") {

I don't think we ever have a situation where `this._delegate` is invalid.  I think you can remove this.
Comment 12 Joseph Pecoraro 2017-12-19 20:24:07 PST
Comment on attachment 329870 [details]
Patch for review

View in context: https://bugs.webkit.org/attachment.cgi?id=329870&action=review

>> Source/WebInspectorUI/ChangeLog:20
>> +        To correctly display invalid and overridden properties, the backend allowed to update
> 
> I think you're missing a word in this sentence, like "is".

Grammar: "the backend allowed to" => "the backend is allowed to"

> Source/WebInspectorUI/ChangeLog:23
> +        front-end changes.

I think you also said this update when text is the same is also useful to update other states like validity of the property. Might be worth including something along that line.

---

In principle this approach sounds fine.

If you have a situation like:

        Front-end:  (1)-(2)---(3)-(blur)
        Back-end:          (1)-------------(2)-(3)

We would end up applying update (2) and (3) right? Hopefully without any visual flicker.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:74
> +        if (this._ownerStyle && this._ownerStyle.locked && text !== this._text)
> +            return;

A comment here, like in the ChangeLog, would make sense about why we update even though we are locked. It is non-obvious.
Comment 13 Nikita Vasilyev 2018-01-10 16:44:53 PST
Comment on attachment 329870 [details]
Patch for review

View in context: https://bugs.webkit.org/attachment.cgi?id=329870&action=review

>> Source/WebInspectorUI/ChangeLog:23
>> +        front-end changes.
> 
> I think you also said this update when text is the same is also useful to update other states like validity of the property. Might be worth including something along that line.
> 
> ---
> 
> In principle this approach sounds fine.
> 
> If you have a situation like:
> 
>         Front-end:  (1)-(2)---(3)-(blur)
>         Back-end:          (1)-------------(2)-(3)
> 
> We would end up applying update (2) and (3) right? Hopefully without any visual flicker.

Good catch!

Yes, it would apply (2) and (3) when changing a value and clicking outside of the styles sidebar. Ideally, it should be just (3). On my machine, most updates from the backend happen <25ms after the change on the front-end, so it's hard to be change the value and click outside of the styles sidebar that quickly. So far I haven't done any performance testing on extremely large documents and slow hardware, but it may worth doing in the future.

When changing a value and tabbing to the next property name, a blur event happens first, shortly followed by a focus event. The delay between the two is 0-1ms on my machine. I don't know how blur and focus events are implemented in WebKit. It may be possibly to receive a backend payload after the blur event but before the focus event, but I haven't been able to reproduce it so far.
Comment 14 Nikita Vasilyev 2018-01-12 17:25:26 PST
Created attachment 331260 [details]
Patch for review
Comment 15 Nikita Vasilyev 2018-01-12 17:32:40 PST
(In reply to Devin Rousso from comment #11)
> Comment on attachment 329870 [details]
> Patch for review
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329870&action=review
> 
> r-, only because this definitely needs some tests.  The logic otherwise
> sounds fine, but having tests (specifically on CSSStyleDeclaration) would
> really help clarify what will happen in the various cases:
> 
>  - not locked, style changes via page's JS
>  - locked, style changes via page's JS
>  - locked, style changes via WebInspector
>  - locked, style changes via WebInspector and page's JS (both orderings)
>  - etc.

Some of these are tricky to test. For example: "locked, style changes via page's JS".

Style change happens asynchronously and it doesn't trigger any related events (such as  WI.CSSStyleDeclaration.Event.PropertiesChanged). I could have listened that WI.CSSStyleDeclaration.Event.PropertiesChanged does NOT get called in the next 500ms or so but that could be flaky and/or slow, and I decided not to do it.
Comment 16 Joseph Pecoraro 2018-01-19 17:19:21 PST
Using this patch I still saw occasional assertions. Harmless I assume since everything seemed to behave as expected:

    [Error] Assertion Failed
        update (CSSStyleDeclaration.js:175)
        _parseStyleDeclarationPayload (DOMNodeStyles.js:676)
        _parseRulePayload (DOMNodeStyles.js:758)
        parseRuleMatchArrayPayload (DOMNodeStyles.js:103)
        fetchedMatchedStyles (DOMNodeStyles.js:126)
        (anonymous function) (DOMNodeStyles.js:88)
        _dispatchResponseToCallback (Connection.js:146)
        _dispatchResponse (Connection.js:116)
        dispatch (Connection.js:69)
        dispatch (InspectorBackend.js:152)
        dispatchNextQueuedMessageFromBackend (MessageDispatcher.js:42)
Comment 17 Nikita Vasilyev 2018-01-19 17:42:42 PST
(In reply to Joseph Pecoraro from comment #16)
> Using this patch I still saw occasional assertions. Harmless I assume since
> everything seemed to behave as expected:
> 
>     [Error] Assertion Failed
>         update (CSSStyleDeclaration.js:175)
>         _parseStyleDeclarationPayload (DOMNodeStyles.js:676)
>         _parseRulePayload (DOMNodeStyles.js:758)
>         parseRuleMatchArrayPayload (DOMNodeStyles.js:103)
>         fetchedMatchedStyles (DOMNodeStyles.js:126)
>         (anonymous function) (DOMNodeStyles.js:88)
>         _dispatchResponseToCallback (Connection.js:146)
>         _dispatchResponse (Connection.js:116)
>         dispatch (Connection.js:69)
>         dispatch (InspectorBackend.js:152)
>         dispatchNextQueuedMessageFromBackend (MessageDispatcher.js:42)

This is an old assertion:

        // Don't fire the event if there is text and it hasn't changed.
        if (oldText && this._text && oldText === this._text) {
            // We shouldn't have any added or removed properties in this case.
            console.assert(!addedProperties.length && !removedProperties.length);

Not sure why it's triggering, investigating.
Comment 18 Joseph Pecoraro 2018-01-19 19:08:10 PST
Comment on attachment 331260 [details]
Patch for review

View in context: https://bugs.webkit.org/attachment.cgi?id=331260&action=review

This seems reasonable, lets live on it.

r=me

> LayoutTests/inspector/css/modify-css-property.html:19
> +        name: "Update value when CSSStyleDeclaration is unlocked",

I'd suggest "not-locked". "unlocked" sounds like the verb, so it comes across as "when it is unlocked", which is not actually what is happening here.

> LayoutTests/inspector/css/modify-css-property.html:126
> +            InspectorTest.evaluateInPage("makeWide()");

Style: We've been using template strings for code, especially in cases like this: evaluateInPage(`code()`)

> LayoutTests/inspector/css/modify-css-property.html:128
> +            resolve();

This test can resolve in multiple ways. Here (immediately) or above with awaitEvent. I think you should keep only the awaitEvent version, since that happens after this one and includes expectations.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:52
> +        this.element.addEventListener("focus", () => this.focused = true, true);

Style: We put braces around arrow function bodies if the return value is not significant:

    () => { this.focused = true; }

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:53
> +        this.element.addEventListener("blur", (event) => {

I worry that somehow this might miss something, but I didn't have any issues in practice.
Comment 19 Nikita Vasilyev 2018-01-22 15:56:05 PST
Created attachment 331971 [details]
Patch for landing
Comment 20 WebKit Commit Bot 2018-01-22 15:57:31 PST
Comment on attachment 331971 [details]
Patch for landing

Rejecting attachment 331971 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 331971, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/6175003
Comment 21 Nikita Vasilyev 2018-01-22 16:00:18 PST
Created attachment 331972 [details]
Patch for landing
Comment 22 Nikita Vasilyev 2018-01-22 16:17:49 PST
Comment on attachment 331972 [details]
Patch for landing

Forgot to fix the assert.
Comment 23 Nikita Vasilyev 2018-01-22 16:29:09 PST
Created attachment 331974 [details]
Patch for landing
Comment 24 WebKit Commit Bot 2018-01-22 16:52:37 PST
Comment on attachment 331974 [details]
Patch for landing

Clearing flags on attachment: 331974

Committed r227370: <https://trac.webkit.org/changeset/227370>
Comment 25 WebKit Commit Bot 2018-01-22 16:52:39 PST
All reviewed patches have been landed.  Closing bug.