Closed Bug 1290825 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox49 + fixed
firefox50 + fixed
firefox51 --- fixed

People

(Reporter: cbook, Assigned: dbaron)

References

(Blocks 1 open bug, )

Details

(Keywords: assertion)

Attachments

(2 files)

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]
Attached file complete stack
[Tracking Requested - why for this release]:

bughunter shows that this affects 49/50 too (also 48 but thats nearly shipped now)
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.
Flags: needinfo?(dbaron)
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?
Flags: needinfo?(cam)
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
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}
There are a large number of cases that bug 922669 failed to reject.
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Flags: needinfo?(dbaron)
Flags: needinfo?(cam)
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
Attachment #8785483 - Flags: review?(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.
Attachment #8785483 - Flags: review?(bzbarsky) → review+
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
https://hg.mozilla.org/mozilla-central/rev/4ccc68de4527
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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
Attachment #8785483 - Flags: approval-mozilla-beta?
Attachment #8785483 - Flags: approval-mozilla-aurora?
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.
Attachment #8785483 - Flags: approval-mozilla-beta?
Attachment #8785483 - Flags: approval-mozilla-beta+
Attachment #8785483 - Flags: approval-mozilla-aurora?
Attachment #8785483 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.