Bug 210917 - fast/scrolling/scroll-behavior-invalidate-if-disabled.html is a flaky failure
Summary: fast/scrolling/scroll-behavior-invalidate-if-disabled.html is a flaky failure
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: cathiechen
URL:
Keywords: InRadar
Depends on: 210634
Blocks:
  Show dependency treegraph
 
Reported: 2020-04-23 09:32 PDT by cathiechen
Modified: 2020-04-26 00:38 PDT (History)
11 users (show)

See Also:


Attachments
Patch (1.66 KB, patch)
2020-04-23 09:44 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (2.83 KB, patch)
2020-04-23 20:54 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (3.02 KB, patch)
2020-04-24 01:28 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (4.51 KB, patch)
2020-04-25 09:54 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (4.48 KB, patch)
2020-04-25 20:21 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (4.52 KB, patch)
2020-04-25 23:17 PDT, cathiechen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description cathiechen 2020-04-23 09:32:50 PDT
fast/scrolling/scroll-behavior-invalidate-if-disabled.html is a flaky failure.

There are two cases try to test the access of scrollBehavior in CSSStyleDeclaration. If run scroll-behavior-validate-if-enabled.html before scroll-behavior-invalidate-if-disabled.html, the first test would restore the propertyID, then the second one would get the result which is wrong.
Comment 1 cathiechen 2020-04-23 09:44:25 PDT
Created attachment 397349 [details]
Patch
Comment 2 cathiechen 2020-04-23 10:17:43 PDT
Comment on attachment 397349 [details]
Patch

Hi,
I think the patch is ready to review, thanks:)
Comment 3 Darin Adler 2020-04-23 11:08:41 PDT
Comment on attachment 397349 [details]
Patch

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

> Source/WebCore/css/CSSStyleDeclaration.cpp:171
> +        // If the property is disabled, should return CSSPropertyInvalid from here.

This comment isn’t valuable. It just says what the code below does. If it said *why* then it might be valuable.

Given that we also do this check *before* putting a property into the cache, it does not seem good that this is needed; not great for performance, basically defeating the cache.

I think the real issue is this:

1) We try to use a single cache, but that results in us caching things that depend on settings, and we could get called later with a different settings object, or nullptr.

2) Even if it was the same settings object, if settings change, the cache needs to be cleared.

> Source/WebCore/css/CSSStyleDeclaration.cpp:172
> +        if (!isEnabledCSSProperty(propertyInfo.propertyID) && !isCSSPropertyEnabledBySettings(propertyInfo.propertyID, settingsPtr))

I would make a helper function rather than repeating this code twice. But I think the only reason we are repeating it twice is that we don’t know how to make this work with the cache.

> Source/WebCore/css/CSSStyleDeclaration.cpp:173
> +            propertyInfo = { CSSPropertyInvalid, false };

Could just return this, instead of assigning it to the local.
Comment 4 cathiechen 2020-04-23 20:47:32 PDT
Comment on attachment 397349 [details]
Patch

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

Hi Darin,

Thanks for the review:)

>> Source/WebCore/css/CSSStyleDeclaration.cpp:171
>> +        // If the property is disabled, should return CSSPropertyInvalid from here.
> 
> This comment isn’t valuable. It just says what the code below does. If it said *why* then it might be valuable.
> 
> Given that we also do this check *before* putting a property into the cache, it does not seem good that this is needed; not great for performance, basically defeating the cache.
> 
> I think the real issue is this:
> 
> 1) We try to use a single cache, but that results in us caching things that depend on settings, and we could get called later with a different settings object, or nullptr.
> 
> 2) Even if it was the same settings object, if settings change, the cache needs to be cleared.

I see... Maybe we can cache propertyInfo anyway, because the "enable checking" needs propertyID and cache can reduce the parsing work.

>> Source/WebCore/css/CSSStyleDeclaration.cpp:172
>> +        if (!isEnabledCSSProperty(propertyInfo.propertyID) && !isCSSPropertyEnabledBySettings(propertyInfo.propertyID, settingsPtr))
> 
> I would make a helper function rather than repeating this code twice. But I think the only reason we are repeating it twice is that we don’t know how to make this work with the cache.

Done. Added isCSSPropertyDisabled().

>> Source/WebCore/css/CSSStyleDeclaration.cpp:173
>> +            propertyInfo = { CSSPropertyInvalid, false };
> 
> Could just return this, instead of assigning it to the local.

Done.
Comment 5 cathiechen 2020-04-23 20:54:02 PDT
Created attachment 397420 [details]
Patch
Comment 6 cathiechen 2020-04-24 01:28:06 PDT
Created attachment 397438 [details]
Patch
Comment 7 Darin Adler 2020-04-24 10:03:53 PDT
Comment on attachment 397438 [details]
Patch

OK. I think we could refactor this a bit more dividing into one more function that does the disabled check after parsing, so the end of the function was shared between the cached and non-cashed case.
Comment 8 cathiechen 2020-04-25 09:51:42 PDT
(In reply to Darin Adler from comment #7)
> Comment on attachment 397438 [details]
> Patch
> 
> OK. I think we could refactor this a bit more dividing into one more
> function that does the disabled check after parsing, so the end of the
> function was shared between the cached and non-cashed case.

Done! Thanks:)
Comment 9 cathiechen 2020-04-25 09:54:34 PDT
Created attachment 397568 [details]
Patch
Comment 10 Darin Adler 2020-04-25 10:26:02 PDT
Comment on attachment 397568 [details]
Patch

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

> Source/WebCore/css/CSSStyleDeclaration.cpp:265
> +static CSSPropertyInfo getCSSPropertyInfoFromJavaScriptCSSPropertyName(const AtomString& propertyName, const Settings* settingsPtr)

Three more tiny things we could do after landing this (or before if not too late), none of which are critical:

- WebKit coding style says we don’t use the word "get" in the name of a function like this one. I would give it a shorter name that doesn’t repeat CSS twice.
- I’d name the settingsPtr arguments just "settings".
- I’d move the isCSSPropertyDisabled down past the larger parseJavaScriptCSSPropertyName function. Or maybe just collapse it in here again and not leave it as a separate function.

Anyway, looks good even without these changes!
Comment 11 cathiechen 2020-04-25 20:21:25 PDT
Created attachment 397607 [details]
Patch
Comment 12 cathiechen 2020-04-25 20:22:04 PDT
(In reply to Darin Adler from comment #10)
> Comment on attachment 397568 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397568&action=review
> 
> > Source/WebCore/css/CSSStyleDeclaration.cpp:265
> > +static CSSPropertyInfo getCSSPropertyInfoFromJavaScriptCSSPropertyName(const AtomString& propertyName, const Settings* settingsPtr)
> 
> Three more tiny things we could do after landing this (or before if not too
> late), none of which are critical:
> 
> - WebKit coding style says we don’t use the word "get" in the name of a
> function like this one. I would give it a shorter name that doesn’t repeat
> CSS twice.
> - I’d name the settingsPtr arguments just "settings".
> - I’d move the isCSSPropertyDisabled down past the larger
> parseJavaScriptCSSPropertyName function. Or maybe just collapse it in here
> again and not leave it as a separate function.
> 
> Anyway, looks good even without these changes!

Done! Thanks for pointing out:)
Comment 13 cathiechen 2020-04-25 23:17:20 PDT
Created attachment 397613 [details]
Patch
Comment 14 EWS 2020-04-26 00:37:29 PDT
Committed r260723: <https://trac.webkit.org/changeset/260723>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397613 [details].
Comment 15 Radar WebKit Bug Importer 2020-04-26 00:38:13 PDT
<rdar://problem/62383245>