[webkit-reviews] review denied: [Bug 102116] ASSERT_NOT_REACHED() when building a CSSOM wrapper for StyleRuleHost : [Attachment 174385] Patch

[bugzilla-daemon at webkit.org]

Alexander Pavlov (apavlov) <apavlov at chromium.org> has denied Takashi Sakamoto
<tasak at google.com>'s request for review:
Bug 102116: ASSERT_NOT_REACHED() when building a CSSOM wrapper for
StyleRuleHost
https://bugs.webkit.org/show_bug.cgi?id=102116

Attachment 174385: Patch
https://bugs.webkit.org/attachment.cgi?id=174385&action=review

------- Additional Comments from Alexander Pavlov (apavlov)
<apavlov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=174385&action=review


> Source/WebCore/ChangeLog:8
> +	   @host @-rules are shown in matchedCSSRules in WebInspector, because

I don't quite understand what this message is supposed to mean. @host rules
will never appear in the web inspector until we add support for them, neither
will they be sent over the protocol, since the backend can only handle
CSSStyleRules and CSSMediaRules (in some form.) The root crash cause was given
in the original bug report: CSSOM wrappers are built on demand when the Web
Inspector backend is about to transmit CSS rule data to the frontend (e.g. when
the matched CSS rules for a node are requested,) and the code flow hits the
ASSERT_NOT_REACHED statement rather than return some kind of wrapper
(CSSUnknownRule in our case.)

> Source/WebCore/ChangeLog:9
> +	   @host @-rules are stored in scoped author stylesheets.

Scoped stylesheets do not matter in this case - the crash can get triggered
with any stylesheet containing an @host rule. Potentially, if ANY @host rule
makes its way into the user agent stylesheet, the crash will occur any time ANY
operation on its rules (event simple styleSheet.rules getter invocation)
occurs.

> Source/WebCore/ChangeLog:10
> +	   However, there is no implementaiton for CSSOMWrapper.

implementaiton -> implementation

I suggest the following simple wording:
Provide a CSSUnknownRule instance as a CSSOM wrapper for StyleRuleHost rules.

> Source/WebCore/ChangeLog:15
> +	   (WebCore::StyleRuleBase::createCSSOMWrapper):

This also needs a short explanation, like

Return a CSSUnknownRule instance for StyleRuleHost rules instead of calling
ASSERT_NOT_REACHED()

> LayoutTests/inspector/styles/styles-include-host-rules-crash.html:4
> + at host {

Could you also add a seemingly invalid @host rule, like this (present in the
original crashing case - don't know why it is declared this way)?

@host.div {
  opacity: 0;
}