Bug 135149 - Reproducible ASSERT_NOT_REACHED in SelectorChecker::checkOne on my.yahoo.com
Summary: Reproducible ASSERT_NOT_REACHED in SelectorChecker::checkOne on my.yahoo.com
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-07-21 23:16 PDT by Tim Horton
Modified: 2014-07-23 10:40 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.49 KB, patch)
2014-07-22 17:05 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.