Bug 229218 – Web Inspector: Style rules declared after a rule whose selector has over 8192 components are not shown correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Patrick Angle]

Created attachment 435732 [details]
Screenshot of Issue

<rdar://81578033>

[Patrick Angle]

Created attachment 435733 [details]
Reduced Test Case

[Patrick Angle]

Created attachment 435740 [details]
Patch v1.0

[Patrick Angle]

Created attachment 435741 [details]
Patch v1.1

[Patrick Angle]

Created attachment 435742 [details]
Screenshot of Patch v1.x

[Patrick Angle]

Comment on attachment 435741 [details]
Patch v1.1

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

> Source/WebCore/css/StyleRule.h:126
> +    bool m_isSplitFromRule { false };
> +    bool m_isLastRuleSplitFromRule { false };

Actually, I could express the info needed for this fix as a single member (something like `m_isUnsplitRuleOrLastRuleSplitFromRule` - open to suggestions on naming). Default to true, and handle setting it to false for all but the last created rule when splitting.

[Patrick Angle]

Created attachment 435803 [details]
Patch v1.2 - Resolve failing test in DEBUG, make sure all selectors make it to the frontend

[Devin Rousso]

Comment on attachment 435803 [details]
Patch v1.2 - Resolve failing test in DEBUG, make sure all selectors make it to the frontend

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

> Source/WebCore/css/StyleRule.h:100
> +    bool isSplitFromRule() const { return m_isSplitFromRule; }

It's a bit odd that this is set by the constructor while the other is set by a setter.  Can we have this be a setter too?  Is the codepath where it's `true` hit that often?

Also, I see that these are only ever set to `true`.  Perhaps we could have these be one-way setters like `void markAsSplitFromRule() { m_isSplitFromRule = true; }` so that there's no concern over it being used to "undo" a previous marking.

> Source/WebCore/inspector/InspectorStyleSheet.cpp:1096
> +Vector<RefPtr<CSSStyleRule>> InspectorStyleSheet::cssStyleRulesSplitFromSameRule(RefPtr<CSSStyleRule> rule)

Is `nullptr` ever expected/allowed inside this `Vector`?  If not, can we use `Vector<Ref<CSSStyleRule>>` instead?

> Source/WebCore/inspector/InspectorStyleSheet.cpp:1098
> +    if (!rule->styleRule().isSplitFromRule())

Since you're assuming that the `RefPtr<CSSStyleRule>` is valid, can we pass a `CSSStyleRule&` here instead?

> Source/WebCore/inspector/InspectorStyleSheet.cpp:1102
> +    if (!m_flatRules.contains(rule))

NIT: Instead of doing a `contains` and then a `find`, could you do the `find` first and then just `firstIndexOfSplitRule != notFound` (which is what `contains` does anyways)?

> Source/WebCore/inspector/InspectorStyleSheet.cpp:1114
> +    for (auto index = firstIndexOfSplitRule; index < m_flatRules.size(); ++index) {

NIT: `auto i = ...`

> Source/WebCore/inspector/InspectorStyleSheet.cpp:1115
> +        auto ruleAtIndex = m_flatRules.at(index);

NIT: `auto rule = ...`

> Source/WebCore/inspector/InspectorStyleSheet.cpp:1121
> +        if (ruleAtIndex->styleRule().isLastRuleSplitFromRule())

So is `isLastRuleSplitFromRule` only really needed to know when to stop iterating?  Wouldn't `isSplitFromRule` already take care of that (i.e. the item after the last `isSplitFromRule`)?

> Source/WebCore/inspector/InspectorStyleSheet.cpp:1155
> +    auto webCoreSelectors = selectorsForCSSStyleRule(rule);

NIT: I'd rather you rename `selectors` to something else (e.g. `selectorsPayload`) instead of prefixing a variable with `webCore` (especially since we're already inside WebCore).  Alternatively you could just inline this in each call and avoid the local in the first place.

> LayoutTests/inspector/css/getMatchedStylesForNodeLargeSelectors.html:26
> +                InspectorTest.expectEqual(authoredRules.length, 1, "Expected exactly one matching authored rule.");

NIT: the word "exactly" is kinda redundant here
NIT: we also usually prefer using "Should" instead of "Expected" (e.g. "Should have one matching author rule.")

> LayoutTests/inspector/css/getMatchedStylesForNodeLargeSelectors.html:40
> +            InspectorTest.expectEqual(rule.selectors[0].text, rule.selectorText, "Expected parsed selector and raw selector text to be the same.");

NIT: Could/Should we also verify that either value is equal to some known value?  This just verifies that the backend has the same value for both, not that the value is correct.  Should it be `"#test1"` or are the comments included (I'd think not) or something else?

> LayoutTests/inspector/css/getMatchedStylesForNodeLargeSelectors.html:56
> +            InspectorTest.expectEqual(rule.selectors[0].text, "#test2", "Expected first parsed selector to be `#test2`.");

Should we confirm that every other selector is a-yA-Y?

> LayoutTests/inspector/css/getMatchedStylesForNodeLargeSelectors.html:91
> +    a b c d e f g h i j k l m n o p q r s t u v w x y A B C D E F G H I J K L M N O P Q R S T U V W X Y,

Why no love for "z"? T_T

[Patrick Angle]

Comment on attachment 435803 [details]
Patch v1.2 - Resolve failing test in DEBUG, make sure all selectors make it to the frontend

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

>> Source/WebCore/inspector/InspectorStyleSheet.cpp:1121
>> +        if (ruleAtIndex->styleRule().isLastRuleSplitFromRule())
> 
> So is `isLastRuleSplitFromRule` only really needed to know when to stop iterating?  Wouldn't `isSplitFromRule` already take care of that (i.e. the item after the last `isSplitFromRule`)?

That would work if we were guaranteed to never have two split rules back to back in the style sheet, since we don't want to iterate into the next split authored rule. But yes this value only serves as an indicator of when to stop iterating (or in the case of `InspectorStyleSheet::ruleIndexByStyle` when to start incrementing again).

>> LayoutTests/inspector/css/getMatchedStylesForNodeLargeSelectors.html:40
>> +            InspectorTest.expectEqual(rule.selectors[0].text, rule.selectorText, "Expected parsed selector and raw selector text to be the same.");
> 
> NIT: Could/Should we also verify that either value is equal to some known value?  This just verifies that the backend has the same value for both, not that the value is correct.  Should it be `"#test1"` or are the comments included (I'd think not) or something else?

Yeah we probably should.

>> LayoutTests/inspector/css/getMatchedStylesForNodeLargeSelectors.html:56
>> +            InspectorTest.expectEqual(rule.selectors[0].text, "#test2", "Expected first parsed selector to be `#test2`.");
> 
> Should we confirm that every other selector is a-yA-Y?

Again probably a good idea.

>> LayoutTests/inspector/css/getMatchedStylesForNodeLargeSelectors.html:91
>> +    a b c d e f g h i j k l m n o p q r s t u v w x y A B C D E F G H I J K L M N O P Q R S T U V W X Y,
> 
> Why no love for "z"? T_T

a-yA-Y is 50 selector components. Made it easier for me to count 😅

[Patrick Angle]

Created attachment 435913 [details]
Patch v1.3 - Address review feedback

[Patrick Angle]

Created attachment 435934 [details]
Patch v1.4 - Refine new member names in StyleRule

[Devin Rousso]

Comment on attachment 435934 [details]
Patch v1.4 - Refine new member names in StyleRule

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

r=me, nice work! :)

> Source/WebCore/ChangeLog:31
> +        mark the last rule split from a given rule (`m_isLastRuleInSplitRule`). The second member prevents situations
> +        where we might otherwise accidentally continue iterating into a list of `StyleRule` where two large authored
> +        rules that had to be split are present next to each other in an ordered collection of `StyleRule`.

Can we add a test for this scenario too?  e.g. create another rule set that flips the a-yA-Y order as z-bZ-B instead =P

> Source/WebCore/css/StyleRule.h:127
> +    bool m_isSplitRule { false };
> +    bool m_isLastRuleInSplitRule { false };

NIT: I'd put these at the end for (potentially) better packing

> Source/WebCore/inspector/InspectorStyleSheet.cpp:1116
> +        auto rule = m_flatRules.at(i);

You're assuming that items in `m_flatRules` exist above.  Can/Should we assume that it exists here too?  Or do we need to add checks above?

> Source/WebCore/inspector/InspectorStyleSheet.cpp:1123
> +        rules.append(*rule.get());

Does `*rule` not work?

> Source/WebCore/inspector/InspectorStyleSheet.cpp:1301
> +    const auto treatSplitRulesAsSingleRule = true;

`constexpr auto`

> Source/WebCore/inspector/InspectorStyleSheet.cpp:1312
> +unsigned InspectorStyleSheet::ruleIndexByStyle(CSSStyleDeclaration* pageStyle, bool treatSplitRulesAsSingleRule) const

NIT: How about `combineSplitRules`?

> LayoutTests/inspector/css/getMatchedStylesForNodeLargeSelectors.html:61
> +            }

Style: unnecessary `{` `}`

> LayoutTests/inspector/css/getMatchedStylesForNodeLargeSelectors.html:93
> +    /* Intentional preceding selector comment */ #test1 /* Intentional following selector comment. */ {

Are the comments actually needed for the test?  If so, why?

> LayoutTests/inspector/css/getMatchedStylesForNodeLargeSelectors.html:271
> +    #unusedRule {
> +        /* The rule intentionally left blank. */
> +    }

Is this actually needed for the test?  If so, why?

[Patrick Angle]

Comment on attachment 435934 [details]
Patch v1.4 - Refine new member names in StyleRule

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

>> Source/WebCore/ChangeLog:31
>> +        rules that had to be split are present next to each other in an ordered collection of `StyleRule`.
> 
> Can we add a test for this scenario too?  e.g. create another rule set that flips the a-yA-Y order as z-bZ-B instead =P

Yep.

>> Source/WebCore/inspector/InspectorStyleSheet.cpp:1116
>> +        auto rule = m_flatRules.at(i);
> 
> You're assuming that items in `m_flatRules` exist above.  Can/Should we assume that it exists here too?  Or do we need to add checks above?

m_flatRules should contain rules by this point after we called `ensureFlatRules()` above, which collects the rules if the vector is empty. If the vector is empty after that for some reason, the `firstIndexOfSplitRule == notFound` check will result in an early return. `ensureFlatRules()` uses `collectFlatRules()` to actually collect the rules, and has logic that prevents a nullptr from being added as a RefPtr to the collection, so an entry in the vector should always exist. I'll add assertions and checks to codify this assumption.

>> LayoutTests/inspector/css/getMatchedStylesForNodeLargeSelectors.html:93
>> +    /* Intentional preceding selector comment */ #test1 /* Intentional following selector comment. */ {
> 
> Are the comments actually needed for the test?  If so, why?

While not strictly necessary, I found it to be a good check for my new code that the method I used for collecting selectors didn't accidentally introduce these comments into parsed selector. I put these here mostly for my own confidence while working on this patch - I'm not too attached them and they can go away.

>> LayoutTests/inspector/css/getMatchedStylesForNodeLargeSelectors.html:271
>> +    }
> 
> Is this actually needed for the test?  If so, why?

They were, but I've added some more expectations to the test cases to make them no longer necessary by making sure that source data (by way of the selector's `range`) exists for the rule's selectors.

Previous this was here because having the index of the source data be greater than the total number of nodes follows a different path for accounting for selectors for rules without source data (see `InspectorStyleSheet::buildObjectForSelectorList`). Without a following rule (and in the next patch two of these rules since we will have two back-to-back rules with giant selectors), the test would actually pass before this patch because it would follow the alternate code path that didn't need source data.

[Patrick Angle]

Created attachment 436009 [details]
Patch v1.5 - Address review feedback

[EWS]

Committed r281354 (240769@main): <https://commits.webkit.org/240769@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 436009 [details].