Bug 135149

Summary: Reproducible ASSERT_NOT_REACHED in SelectorChecker::checkOne on my.yahoo.com
Product: WebKit Reporter: Tim Horton <thorton>
Component: CSSAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, benjamin, commit-queue, simon.fraser, thorton
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Tim Horton 2014-07-21 23:16:22 PDT 
Steps to Reproduce:

1. Load my.yahoo.com while logged into yahoo.
2. Wait.

Expected: No assert!
Actual: An assertion failure:

Notes: I'm using UI-side compositing in Minibrowser but it seems unlikely either of those are prerequisites for this failure.

SHOULD NEVER BE REACHED
/Users/thorton/src/WebKit/OpenSource/Source/WebCore/css/SelectorChecker.cpp(793) : bool WebCore::SelectorChecker::checkOne(const WebCore::SelectorChecker::SelectorCheckingContext &) const
1   0x10abecf10 WTFCrash
2   0x10d912b13 WebCore::SelectorChecker::checkOne(WebCore::SelectorChecker::SelectorCheckingContext const&) const
3   0x10d910c1e WebCore::SelectorChecker::matchRecursively(WebCore::SelectorChecker::SelectorCheckingContext const&, WebCore::PseudoId&) const
4   0x10d9112cf WebCore::SelectorChecker::matchRecursively(WebCore::SelectorChecker::SelectorCheckingContext const&, WebCore::PseudoId&) const
5   0x10d9112cf WebCore::SelectorChecker::matchRecursively(WebCore::SelectorChecker::SelectorCheckingContext const&, WebCore::PseudoId&) const
6   0x10d910b1a WebCore::SelectorChecker::match(WebCore::SelectorChecker::SelectorCheckingContext const&) const
7   0x10c5689a0 WebCore::ElementRuleCollector::ruleMatches(WebCore::RuleData const&)
8   0x10c566ec5 WebCore::ElementRuleCollector::collectMatchingRulesForList(WTF::Vector<WebCore::RuleData, 0ul, WTF::CrashOnOverflow> const*, WebCore::MatchRequest const&, WebCore::StyleResolver::RuleRange&)
9   0x10c566c76 WebCore::ElementRuleCollector::collectMatchingRules(WebCore::MatchRequest const&, WebCore::StyleResolver::RuleRange&)
10  0x10c5674c8 WebCore::ElementRuleCollector::matchAuthorRules(bool)
11  0x10da346ce WebCore::invalidateStyleRecursively(WebCore::Element&, WebCore::SelectorFilter&, WebCore::DocumentRuleSets const&)
[ ... a bunch of levels of recursion elided ... ]
27  0x10da347e7 WebCore::invalidateStyleRecursively(WebCore::Element&, WebCore::SelectorFilter&, WebCore::DocumentRuleSets const&)
28  0x10da3462d WebCore::StyleInvalidationAnalysis::invalidateStyle(WebCore::Document&)
29  0x10c42f368 WebCore::DocumentStyleSheetCollection::analyzeStyleSheetChange(WebCore::DocumentStyleSheetCollection::UpdateFlag, WTF::Vector<WTF::RefPtr<WebCore::CSSStyleSheet>, 0ul, WTF::CrashOnOverflow> const&, WebCore::DocumentStyleSheetCollection::StyleResolverUpdateType&, bool&)
30  0x10c42f4bf WebCore::DocumentStyleSheetCollection::updateActiveStyleSheets(WebCore::DocumentStyleSheetCollection::UpdateFlag)
31  0x10c3acb04 WebCore::Document::styleResolverChanged(WebCore::StyleResolverUpdateFlag)
Comment 1 Tim Horton 2014-07-21 23:21:25 PDT 
(lldb) p selector->pseudoClassType()
(WebCore::CSSSelector::PseudoClassType) $0 = PseudoClassWindowInactive

Which does indeed seem to be missing from that switch.
Comment 2 Alex Christensen 2014-07-22 14:57:20 PDT 
I'll look into this.  It is quite upsetting that this was not caught with a test :(
Comment 3 Alex Christensen 2014-07-22 17:05:01 PDT 
Created attachment 235327 [details]
Patch
Comment 4 Tim Horton 2014-07-22 17:19:15 PDT 
Comment on attachment 235327 [details]
Patch

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

> Source/WebCore/css/SelectorChecker.cpp:-792
> -        default:

A++
Comment 5 WebKit Commit Bot 2014-07-22 18:02:22 PDT 
Comment on attachment 235327 [details]
Patch

Clearing flags on attachment: 235327

Committed r171378: <http://trac.webkit.org/changeset/171378>
Comment 6 WebKit Commit Bot 2014-07-22 18:02:29 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 7 Benjamin Poulain 2014-07-23 01:10:31 PDT 
Comment on attachment 235327 [details]
Patch

I am only catching up with my mails now. This is amazing. The lack of tests for something this simple baffles me.

Can you please add PseudoClassWindowInactive as an unoptimizedPseudoClass to the CSS JIT?

In SelectorChecker, you should remove the branch "if (context.hasSelectionPseudo) -> check for :window-inactive".

Can you please also add a test for style resolution? It is common to find bugs that only appear in one of style-resolution/querySelector.
Comment 8 Alex Christensen 2014-07-23 10:40:29 PDT 
(In reply to comment #7)
> (From update of attachment 235327 [details])
> I am only catching up with my mails now. This is amazing. The lack of tests for something this simple baffles me.
> 
> Can you please add PseudoClassWindowInactive as an unoptimizedPseudoClass to the CSS JIT?
https://bugs.webkit.org/show_bug.cgi?id=135200
> 
> In SelectorChecker, you should remove the branch "if (context.hasSelectionPseudo) -> check for :window-inactive".
> 
> Can you please also add a test for style resolution? It is common to find bugs that only appear in one of style-resolution/querySelector.
There already was a test for style resolution, but not for querySelector, which is why this bug existed.