1290825 - Assertion failure: !aSelector || !aSelector->IsPseudoElement(), at nsCSSRuleProcessor.cpp:2367

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Carsten Book [:Tomcat]]

Found via bughunter and reproduced with latest m-c debug tinderbox build.

affects nightly to beta, so requesting tracking 

Steps to reproduce:
-> Load https://hotelscan.com/de/search?ref=dis:ok:cpl:g:trafficDNA&geoid=x5nltcj3hott&utm_source=ok&utm_medium=cpc&utm_campaign=6529262&utm_content=21891305&utm_term=347_m75

-->> 

Assertion failure: !aSelector || !aSelector->IsPseudoElement(), at c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/layout/style/nsCSSRuleProces
#01: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x1f1032a]
#02: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x1f10878]
#03: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x1f12c3c]
#04: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x1f9f0cd]
#05: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x1fbf4b1]
#06: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x1fcb54a]
#07: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x2033fac]
#08: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x2033ea1]
#09: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x2033e1a]
#10: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x2033d7c]
#11: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x2006159]
#12: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x202cf13]
#13: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x200d3c9]
#14: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x200f280]
#15: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x200e6d1]
#16: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x200ef13]
#17: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x200efd1]
#18: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x202cfd7]
#19: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x200d3c9]
#20: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x200f280]
#21: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x200e6d1]
#22: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x200ef13]
#23: imgLoader::SupportImageWithMimeType[c:\bughunter\firefox-50.0a1.en-US.win32\firefox\xul.dll +0x200efd1]

Comment 1

[Carsten Book [:Tomcat]]

Attachment: complete stack

Comment 2

[Carsten Book [:Tomcat]]

[Tracking Requested - why for this release]:

bughunter shows that this affects 49/50 too (also 48 but thats nearly shipped now)

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

dbaron, can you help find an owner for this bug? I just noticed that tomcat requested we track it for 49, and I'm not sure who to ask. Thanks.

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

heycam, what do you think (pinging you as a module peer).  Is this important to fix in 49 or 50? Can you help find an owner to investigate?

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Prior to the fatal assertion, there are also a bunch of:
###!!! ASSERTION: Shouldn't have negations: '!selector->mNegations', file /home/dbaron/builds/ssd/mozilla-central/mozilla/layout/style/nsCSSRuleProcessor.cpp, function AddRule, line 3415

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

All of the assertions (fatal and non-fatal) are related to the rule that we serialize as:

.discount:not(.discount-0) ~ h3, .discount-background::after:not(.discount-0) ~ h3 { padding-right: 4rem ! important;} /* https://hotelscan.com/v3/static/css/app.min.css?v=002c029ee144b88b65497db89dae11e632b2aecb:1 */

The raw source of the rule is:

.discount:not(.discount-0) ~ h3,.discount-background::after:not(.discount-0) ~ h3{padding-right:4rem !important}

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

There are a large number of cases that bug 922669 failed to reject.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Reject various things that aren't user-action pseudo classes when they follow pseudo-elements

The existing code, from bug 922669, in ParsePseudoSelector that allows
things to come after a pseudo-element requires that the first character
after the pseudo-element be a colon.  However, this doesn't forbid
things like ::-moz-color-swatch:hover#foo, which need to be errors in
ParseSelector; those tests are added here.  Furthermore, the
error-checking in ParsePseudoSelector doesn't prevent the pseudo-element
from being followed by a :not() or by an additional pseudo-element; to
fix that this patch moves the error tests out of the pseudo-class
condition and also has it test !isPseudoClass.

Without the patch, the tests produced the following failures:
TEST-UNEXPECTED-FAIL | layout/style/test/test_selectors.html | selector ::-moz-color-swatch:not(.foo) was a parser error - got "1402", expected "auto"
TEST-UNEXPECTED-FAIL | layout/style/test/test_selectors.html | selector '::-moz-color-swatch:not(.foo)' plus EOF is parse error
followed by a crash due to:
Assertion failure: !(IsPseudoElement() && (mIDList || mAttrList)) (If pseudo-elements can have id or attribute selectors after them, specificity calculation must be updated), at /home/dbaron/builds/ssd/mozilla-central/mozilla/layout/style/StyleRule.cpp:503 in CalcWeightWithoutNegations
from the "::-moz-color-swatch:hover#foo" test.

With that test commented out (and still without the code changes), there
is instead an additional pair of failures from the following test:
TEST-UNEXPECTED-FAIL | layout/style/test/test_selectors.html | selector .foo::after:not(.bar) ~ h3 was a parser error - got "1406", expected "auto"
TEST-UNEXPECTED-FAIL | layout/style/test/test_selectors.html | selector '.foo::after:not(.bar) ~ h3' plus EOF is parse error
along with a failure due to an unexpected assertion:
###!!! ASSERTION: Shouldn't have negations: '!selector->mNegations', file /home/dbaron/builds/ssd/mozilla-central/mozilla/layout/style/nsCSSRuleProcessor.cpp, function AddRule, line 3415

With the patch, the tests pass.

MozReview-Commit-ID: KxAFSQtPVhu

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bebbe861941f&group_state=expanded

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8785483 [details] [diff] [review]
Reject various things that aren't user-action pseudo classes when they follow pseudo-elements

>+    test_balanced_unparseable("::-moz-color-swatch:not(.foo)");

Why a moz-prefixed pseudo-element here instead of ::before or ::after?

r=me modulo that; at least deserves a comment.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ccc68de45278a1f881e24e74759f15e3379bb86
Bug 1290825 - Reject various things that aren't user-action pseudo classes when they follow pseudo-elements.  r=bz

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4ccc68de4527

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8785483 [details] [diff] [review]
Reject various things that aren't user-action pseudo classes when they follow pseudo-elements

Approval Request Comment
[Feature/regressing bug #]: bug 922669
[User impact if declined]: potential for crashes
[Describe test coverage new/current, TreeHerder]: We have automated tests that the selectors that should have been rejected are in fact rejected.
[Risks and why]: There's a slight risk that it might break Web content that's using selectors that were incorrectly accepted.  This risk seems rather low, though, given that such selectors are quite obscure, and given that if they were, they may well have been seeing stranger misbehavior or crashes as a result of using those selectors.
[String/UUID change made/needed]: no

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8785483 [details] [diff] [review]
Reject various things that aren't user-action pseudo classes when they follow pseudo-elements

May eliminate some crashes, risk here for edge cases sounds less bad here than the already broken behavior. This should be in 49 beta 9 build.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/25ca86f29088

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/8b30b5b52d39