Bug 179461 – Web Inspector: Styles Redesign: data corruption when updating values quickly

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

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.

[Radar WebKit Bug Importer]

<rdar://problem/35431882>

[Nikita Vasilyev]

This wasn't a problem in the old styles sidebar because it treated text in the view (CodeMirror instance) as the source of truth.

[Nikita Vasilyev]

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)

[Nikita Vasilyev]

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

[Nikita Vasilyev]

Created attachment 328921 [details]
Patch

[Nikita Vasilyev]

Created attachment 328922 [details]
Patch (debug)

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

Created attachment 328926 [details]
Patch for review

[Nikita Vasilyev]

Created attachment 329870 [details]
Patch for review

Rebaselined.

[Devin Rousso]

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.

[Joseph Pecoraro]

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.

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

Created attachment 331260 [details]
Patch for review

[Nikita Vasilyev]

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

[Joseph Pecoraro]

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)

[Nikita Vasilyev]

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

[Joseph Pecoraro]

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.

[Nikita Vasilyev]

Created attachment 331971 [details]
Patch for landing

[WebKit Commit Bot]

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

[Nikita Vasilyev]

Created attachment 331972 [details]
Patch for landing

[Nikita Vasilyev]

Comment on attachment 331972 [details]
Patch for landing

Forgot to fix the assert.

[Nikita Vasilyev]

Created attachment 331974 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 331974 [details]
Patch for landing

Clearing flags on attachment: 331974

Committed r227370: <https://trac.webkit.org/changeset/227370>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.