192396 – Web Inspector: Styles: toggling selected properties may cause data corruption

Bug 192396 - Web Inspector: Styles: toggling selected properties may cause data corruption

Summary: Web Inspector: Styles: toggling selected properties may cause data corruption

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	Web Inspector (show other bugs)
Version:	WebKit Nightly Build
Hardware:	All All

Importance:	P2 Normal
Assignee:	Nikita Vasilyev

URL:
Keywords:	InRadar

Depends on:
Blocks:

Reported:	2018-12-04 23:06 PST by Nikita Vasilyev
Modified:	2018-12-15 02:03 PST (History)
CC List:	6 users (show)

See Also:

Attachments
Patch (6.49 KB, patch) 2018-12-06 16:15 PST, Nikita Vasilyev	nvasilyev: review- nvasilyev: commit-queue-	Details \| Formatted Diff \| Diff
Patch (7.66 KB, patch) 2018-12-06 23:14 PST, Nikita Vasilyev	hi: review-	Details \| Formatted Diff \| Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.24 MB, application/zip) 2018-12-07 02:28 PST, EWS Watchlist	no flags	Details
Patch (12.60 KB, patch) 2018-12-07 16:18 PST, Nikita Vasilyev	nvasilyev: review- nvasilyev: commit-queue-	Details \| Formatted Diff \| Diff
WIP: single agent call (15.42 KB, patch) 2018-12-08 15:38 PST, Nikita Vasilyev	nvasilyev: review- nvasilyev: commit-queue-	Details \| Formatted Diff \| Diff
Patch (20.62 KB, patch) 2018-12-08 16:00 PST, Nikita Vasilyev	hi: review+ hi: commit-queue-	Details \| Formatted Diff \| Diff
Patch (22.06 KB, patch) 2018-12-15 01:21 PST, Nikita Vasilyev	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (6) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Nikita Vasilyev 2018-12-04 23:06:20 PST

Quickly commenting out and uncommenting selected properties may cause data corruption.

1. Open https://developer.apple.com/design/human-interface-guidelines/macos/visual-design/dark-mode/
2. Inspect <body>
3. Open Styles panel in the right sidebar
4. Select the following 3 CSS properties:

    -moz-font-feature-settings: 'kern';
    -webkit-font-smoothing: antialiased;
    -moz-osx-font-smoothing: grayscale;

5. Press Space key several 10 times as fast as possible.
6. Navigate to the source file by clicking on the CSS link.


Expected:
The 3 selected CSS properties should remain uncommented:

    -moz-font-feature-settings: 'kern';
    -webkit-font-smoothing: antialiased;
    -moz-osx-font-smoothing: grayscale;


Actual:
The properties may get scrambled:

     -moz-font-feature-settings: 'kern';; -webki; -webkit-font-smoothing: antialiased;*/; -moz-osx-font-smoothing: grayscale;*/ale;; */bkit-font-smoothing: antialiased;moothing: grayscale; */; -moz-osx-font-smoothing: grayscale;/* -moz-osx-font-smoothing: grayscale; */


Notes:
Using the style editing debug mode (Bug 192282) should help to understand the issue.

Comment 1 Radar WebKit Bug Importer 2018-12-04 23:07:06 PST

<rdar://problem/46478383>

Comment 2 Nikita Vasilyev 2018-12-05 23:27:15 PST

This is the same bug as shown on https://bugs.webkit.org/show_bug.cgi?id=192282#c3.

Comment 3 Nikita Vasilyev 2018-12-06 16:15:53 PST

Created attachment 356761 [details]
Patch

Comment 4 Nikita Vasilyev 2018-12-06 23:14:29 PST

Created attachment 356785 [details]
Patch

Comment 5 EWS Watchlist 2018-12-07 02:28:04 PST

Comment on attachment 356785 [details]
Patch

Attachment 356785 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10302333

New failing tests:
fast/visual-viewport/tiled-drawing/zoomed-fixed-scrolling-layers-state.html

Comment 6 EWS Watchlist 2018-12-07 02:28:06 PST

Created attachment 356790 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

Comment 7 Nikita Vasilyev 2018-12-07 09:58:25 PST

Comment on attachment 356785 [details]
Patch

Unrelated test failures.

Comment 8 Devin Rousso 2018-12-07 14:15:50 PST

Comment on attachment 356785 [details]
Patch

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

r-, due to the potential double agent call

> LayoutTests/inspector/css/modify-css-property.html:131
> +        name: "Comment out and uncomment a property",

This is more of a "description", not a name.  Our usual pattern is to use the suite name followed by a test name, all camel-cased.

    ModifyCSSProperty.CommentOutAndUncommentProperty

> LayoutTests/inspector/css/modify-css-property.html:148
> +            let styleDeclaration = getMatchedStyleDeclaration();

This should fail in the case that no style is found (would also require a `return null;` inside `getMatchedStyleDeclaration`).

    if (!styleDeclaration) {
        InspectorTest.fail("No style declaration found.");
        resolve();
        return;
    }

> LayoutTests/inspector/css/modify-css-property.html:154
> +            getProperty("padding-right").commentOut(disabled);

Ditto (>148).

    let property = getProperty("padding-right");
    if (!property) {
        InspectorTest.fail("No property found.");
        resolve();
        return;
    }

> LayoutTests/inspector/css/modify-css-property.html:159
> +            InspectorTest.expectEqual(styleDeclaration.text, `
> +        /* padding-left: 2em; */
> +        padding-right: 0px;
> +    `, `Style declaration text should update immediately with uncommented property.`);

This is unfortunate, especially considering how specific it is with spacing.  Is there any way we could move this outside the `InspectorText.expectEqual`, like maybe as a `const`, so it can be styled better?

> LayoutTests/inspector/css/modify-css-property.html:164
> +            getProperty("padding-right").commentOut(disabled);

Ditto (>154).  Also, is this expected to return a new property, or the same property?  We should assert that the result is the same/different.

> LayoutTests/inspector/css/modify-css-property.html:169
> +            InspectorTest.expectEqual(styleDeclaration.text, `
> +        /* padding-left: 2em; */
> +        /* padding-right: 0px; */
> +    `, `Style declaration text should update immediately with commented out property.`);

Ditto (>159).

> LayoutTests/inspector/css/modify-css-property.html:171
> +            InspectorTest.expectThat(!getProperty("padding-right").enabled, `Commented out property is disabled.`);

Ditto (>164).

> LayoutTests/inspector/css/modify-css-property.html:212
> +        /* padding-left: 2em; */
> +        /* padding-right: 0px; */

We should add tests for different padding situations (e.g. if there is more than one space before/after the property).

> Source/WebInspectorUI/ChangeLog:24
> +            /* color: red; */ font-size: 12px

I'm not sure if this is just how you formatted this, but shouldn't we be keeping the newline as well?

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:401
> +                this.text = property.text.trimRight() + ";" + match[1];

This means that we'll be doing two updates of the owner style.  We should try to do as few back/forth calls to the backend as possible.

Also, I think this will only work if the owner style is `locked`, as otherwise the owner style text won't get updated until `CSS.setStyleText` returns.  We should test this.

Comment 9 Nikita Vasilyev 2018-12-07 14:35:07 PST

(In reply to Devin Rousso from comment #8)
> Comment on attachment 356785 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=356785&action=review
> 
> r-, due to the potential double agent call

Yes, two calls may happen but only when all these conditions meet:
- when adding a new property,
- appending it after the last property,
- the last property isn't terminated with a semicolon.

This is such a rare case that I prefer to keep the code simple rather than trying to avoid one backend call.

>> Source/WebInspectorUI/ChangeLog:24
>> +            /* color: red; */ font-size: 12px
> 
> I'm not sure if this is just how you formatted this, but shouldn't we be keeping the newline as well?

You're right, this was confusing.

Comment 10 Nikita Vasilyev 2018-12-07 16:17:24 PST

Comment on attachment 356785 [details]
Patch

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

>> LayoutTests/inspector/css/modify-css-property.html:148
>> +            let styleDeclaration = getMatchedStyleDeclaration();
> 
> This should fail in the case that no style is found (would also require a `return null;` inside `getMatchedStyleDeclaration`).
> 
>     if (!styleDeclaration) {
>         InspectorTest.fail("No style declaration found.");
>         resolve();
>         return;
>     }

We are in control of this page. Why would it not be found?

>> LayoutTests/inspector/css/modify-css-property.html:154
>> +            getProperty("padding-right").commentOut(disabled);
> 
> Ditto (>148).
> 
>     let property = getProperty("padding-right");
>     if (!property) {
>         InspectorTest.fail("No property found.");
>         resolve();
>         return;
>     }

Ditto. I'm concerned adding this after every `getProperty` call would only make tests less readable.

>>> Source/WebInspectorUI/ChangeLog:24
>>> +            /* color: red; */ font-size: 12px
>> 
>> I'm not sure if this is just how you formatted this, but shouldn't we be keeping the newline as well?
> 
> You're right, this was confusing.

We used to strip whitespace, including line breaks. Now it's preserved!

Comment 11 Nikita Vasilyev 2018-12-07 16:18:06 PST

Created attachment 356850 [details]
Patch

I added more tests.

Comment 12 Nikita Vasilyev 2018-12-08 15:38:44 PST

Created attachment 356890 [details]
WIP: single agent call

I (In reply to Nikita Vasilyev from comment #9)
> (In reply to Devin Rousso from comment #8)
> > Comment on attachment 356785 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=356785&action=review
> > 
> > r-, due to the potential double agent call
> 
> Yes, two calls may happen but only when all these conditions meet:
> - when adding a new property,
> - appending it after the last property,
> - the last property isn't terminated with a semicolon.

I tried doing this in one call. The WIP is attached. I'm not going forward with this approach.

The problem in the first place wasn't multiplying semicolons alone. That wouldn't cause data corruption. The data corruption was caused because the semicolons were a special case that didn't update ranges of the following properties.

I don't want to make logic harder to follow. It's premature optimization.

I'll add tests for inserting new properties, which were long overdue, and I'll continue with my previous approach.

Comment 13 Nikita Vasilyev 2018-12-08 16:00:26 PST

Created attachment 356891 [details]
Patch

Comment 14 Devin Rousso 2018-12-12 19:15:14 PST

Comment on attachment 356891 [details]
Patch

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

r+, but I want to see tests as described in my comments before this lands

> LayoutTests/inspector/css/add-css-property.html:9
> +            let suite = InspectorTest.createAsyncSuite("AddCSSProperty");

Is there a reason this needs to be an async test suite?  It seems like all test code is synchronous, so we could use a sync suite.

> LayoutTests/inspector/css/add-css-property.html:21
> +                        reject();

NIT: although this might seem counterintuitive, I'd prefer that you call `resolve()` here instead, as calling `reject()` will prevent any test cases after this one from running (it effectively kills the test chain, as none of the cases have `catch` handlers for their associated promises, so a single rejection in any test case basically aborts the entire run).  If a problem is "localized" to a specific test case, we shouldn't prevent the running of subsequent tests as a result.

> LayoutTests/inspector/css/add-css-property.html:25
> +                    styleDeclaration.locked = true;

What would happen in the case that the style isn't `locked`?  Is that possible?  We should have some tests for this.

Interestingly, both `set name` and `set rawValue` call through to `_ownerStyle.set text`, which may cause race-y things to happen.  We should test what happens when `locked === false`, as in that case the cached `_ownerStyle._text` isn't immediately updated (it waits for the agent call to return with the "real" value and uses that).

> LayoutTests/inspector/css/add-css-property.html:28
> +                    newProperty.rawValue = "green";

I noticed that there's a FIXME inside `WI.CSSProperty`:

    // FIXME: Remove current value getter and rename rawValue to value once the old styles sidebar is removed.

Perhaps now (or a followup in the immediate future) is that time?

> LayoutTests/inspector/css/add-css-property.html:37
> +                name: "AddCSSProperty.InsertBeforeAndAfter",

There should also be a test for "InsertBetween".

> LayoutTests/inspector/css/add-css-property.html:39
> +                    let getMatchedStyleDeclaration = () => {

Rather than repeat this helper function inside each test case, you should make one that accepts a selector as an argument (as well as the resolve/reject objects):

    function getMatchedStyle(selector, resolve) {
        for (let rule of nodeStyles.matchedRules) {
            if (rule.selectorText === selector)
                return rule.style;
        }

        InspectorTest.fail("No style found for selector " + selector);
        resolve();
    }

> LayoutTests/inspector/css/add-css-property.html:63
> +                    expected = `color: green;\n    outline: 2px solid brown;display: block;\n`;

So this patch doesn't try to add a prefix newline/spacing based on the previous line's spacing?  I thought that that was part of this change?  If so, that should be a followup.

> LayoutTests/inspector/css/modify-css-property.html:175
> +            let expectedStyleText = `
> +        /* padding-left: 2em; */
> +        padding-right: 0px;
> +    `;

NIT: can you shift these to use "\n" like the other test file?

> LayoutTests/inspector/css/modify-css-property.html:196
> +        name: "ModifyCSSProperty.CommentOutAndUncommentProperty2",

This could use a better name (e.g. "ModifyCSSProperty.CommentOutAndUncommentPropertyWithoutNewlines", and the previous test case would be "ModifyCSSProperty.CommentOutAndUncommentPropertyWithNewlines").

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:401
> +                property.text = property.text.trimRight() + ";" + match[1];

This still worries me, as we'd potentially have race-y behavior if the `_ownerStyle` isn't locked when the text is changed.  I agree that it's a rare case, so I'm ok with this moving forward, but adding some tests for this situation (and others like it) would definitely help.

Comment 15 Nikita Vasilyev 2018-12-15 01:21:20 PST

Created attachment 357397 [details]
Patch

Comment 16 Nikita Vasilyev 2018-12-15 01:39:56 PST

(In reply to Devin Rousso from comment #14)
> > LayoutTests/inspector/css/add-css-property.html:25
> > +                    styleDeclaration.locked = true;
> 
> What would happen in the case that the style isn't `locked`?  Is that
> possible?  We should have some tests for this.
> 
> Interestingly, both `set name` and `set rawValue` call through to
> `_ownerStyle.set text`, which may cause race-y things to happen.  We should
> test what happens when `locked === false`, as in that case the cached
> `_ownerStyle._text` isn't immediately updated (it waits for the agent call
> to return with the "real" value and uses that).

So far I added asserts. I don't think editing of unlocked style declarations makes any sense. I think it shouldn't be impossible:
Bug 192739 - Web Inspector: Styles: prevent potential data corruption by ensuring style declarations are locked when editing

Comment 17 WebKit Commit Bot 2018-12-15 02:03:27 PST

Comment on attachment 357397 [details]
Patch

Clearing flags on attachment: 357397

Committed r239251: <https://trac.webkit.org/changeset/239251>

Comment 18 WebKit Commit Bot 2018-12-15 02:03:29 PST

All reviewed patches have been landed.  Closing bug.