Bug 102116

The crash occurs on StyleRule.cpp:229 (ASSERT_NOT_REACHED()) for StyleRuleHost ("case Host:") introduced in r132618. IMO the code is wrong, and a new type of a CSSOM wrapper (CSSHostRule?) should be returned instead of calling ASSERT_NOT_REACHED() in this case. This crash will also occur on any CSSOM operations involving rules of a stylesheet with @host rules (resulting in building CSSOM wrappers for all stylesheet rules), so should be fixed sooner rather than later.

Created attachment 174363 [details] Patch

To quickly fix this issue, I used CSSUnknownRule. However it is not good way. So I filed a new bug 102344 to implement a new class CSSHostRule and to replace CSSUnknownRule with the class. Best regards, Takashi Sakamoto

Comment on attachment 174363 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174363&action=review > Source/WebCore/css/StyleRule.cpp:225 > + // use CSSHostRule instead of CSSUnknownRule. Please replace this entire comment by something like // FIXME: The current CSSOM editor's draft (http://dev.w3.org/csswg/cssom/) does not handle @host rules (see bug 102344). so that it will be clear why you don't fix this "properly" up front (and the FIXME format matches that used in the WebCore/css package). > LayoutTests/inspector/styles/styles-include-host-crash.html:11 > + WebInspector.settings.showShadowDOM.set(true); I wonder if you need this at all. The crash occurs at the moment when CSSOM wrappers are constructed for the stylesheet rules (at least one of which is an @host rule). You can just inspect any element that has a matching rule from a stylesheet with @host rules, e.g.: InspectorTest.selectNodeAndWaitForStyles("host", next); ...and in the "next" callback: InspectorTest.dumpSelectedElementStyles(true, false, true); (of course, you won't see the @host rule there, but at least the test should not crash). The test structure will match inspector/elements/elements-panel-styles.html.

Created attachment 174374 [details] Patch

Comment on attachment 174363 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174363&action=review Thank you for reviewing. >> Source/WebCore/css/StyleRule.cpp:225 >> + // use CSSHostRule instead of CSSUnknownRule. > > Please replace this entire comment by something like > > // FIXME: The current CSSOM editor's draft (http://dev.w3.org/csswg/cssom/) does not handle @host rules (see bug 102344). > > so that it will be clear why you don't fix this "properly" up front (and the FIXME format matches that used in the WebCore/css package). Thank you. Done. >> LayoutTests/inspector/styles/styles-include-host-crash.html:11 >> + WebInspector.settings.showShadowDOM.set(true); > > I wonder if you need this at all. The crash occurs at the moment when CSSOM wrappers are constructed for the stylesheet rules (at least one of which is an @host rule). > You can just inspect any element that has a matching rule from a stylesheet with @host rules, e.g.: > > InspectorTest.selectNodeAndWaitForStyles("host", next); > > ...and in the "next" callback: > > InspectorTest.dumpSelectedElementStyles(true, false, true); > (of course, you won't see the @host rule there, but at least the test should not crash). > > The test structure will match inspector/elements/elements-panel-styles.html. I see. I would like to reuse inspector/elements/elements-panel-styles.html.

Comment on attachment 174374 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174374&action=review > LayoutTests/inspector/elements/resources/elements-panel-styles.css:55 > +@host { You will need a separate test for this, since we are going to test an entirely different thing than the way the Elements panel renders element styles (and we usually create a new test for every bug). Feel free to copy elements-panel-styles.html into a file you created in your previous patch and embed the @host rule into an inline stylesheet (<style>@host { }</style>), right in the HTML. This way you will not need an external CSS file.

Comment on attachment 174374 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174374&action=review >> LayoutTests/inspector/elements/resources/elements-panel-styles.css:55 >> +@host { > > You will need a separate test for this, since we are going to test an entirely different thing than the way the Elements panel renders element styles (and we usually create a new test for every bug). Feel free to copy elements-panel-styles.html into a file you created in your previous patch and embed the @host rule into an inline stylesheet (<style>@host { }</style>), right in the HTML. This way you will not need an external CSS file. I see. Done.

Created attachment 174385 [details] Patch

Comment on attachment 174385 [details] Patch 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 > +@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; }

(In reply to comment #9) > Created an attachment (id=174385) [details] > Patch I also suggest adding the following pure CSSOM-based test (e.g. as fast/css/at-host-cssom-crash.html): <!DOCTYPE html> <head> <style> @host { div { padding: 2px; } } div { padding: 2px; } </style> </head> </body> <table><tr> <script> if (window.internals) testRunner.dumpAsText(); var styleSheets = document.styleSheets; for (var i = 0; i < styleSheets.length; ++i) styleSheets[i].rules(); </script> Test for <a href="https://bugs.webkit.org/show_bug.cgi?id=102116">102116</a>: ASSERT_NOT_REACHED() when building a CSSOM wrapper for StyleRuleHost<br> This test has PASSED (no crash). </body> </html>

Created attachment 174585 [details] Patch

Comment on attachment 174385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174385&action=review Thank you for reviewing. >> 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.) I misunderstood the reason why this crash occurs. Firstly I think, (1) load some html which has @host @-rules in some style element -> (2) open some element which matches @host @-rules by using WebInspector and see matched rules -> (3) crash. So "to match @host @-rules" in class StyleResolver, @host @-rules should be stored in scoped stylesheets. However looking at the stack trace, ASSERT_NOT_REACHED occurs at much earlier stage. I wil upadte this description. >> 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. You are right. I added this line since I was thinking of the case: "matchAuthorRules -> create a CSSOM wrapper -> crash by ASSERT_NOT_REACHED". But the stack trace said, "WebCore::StyleResolver::ensureFullCSSOMWrapperForInspector" not "WebCore::StyleResolver::sortAndTransferMatchedRules". And looking at your suggested at-host-cssom-crash.html, we can cause the crash independent of whether any @host @-rules is applied or not. I'm wrong. >> 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. I see. Done. >> 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() Done. >> LayoutTests/inspector/styles/styles-include-host-rules-crash.html:4 >> +@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; > } Sure. Done.

Comment on attachment 174585 [details] Patch r=me

Comment on attachment 174585 [details] Patch Clearing flags on attachment: 174585 Committed r134909: <http://trac.webkit.org/changeset/134909>

All reviewed patches have been landed. Closing bug.

Reopening to attach new patch.

I'm sorry. I took a mistake. I think, no need to reopen this.