stylo: mozreview debug_assertion failure: pseudos can't generate sibling invalidations...

RESOLVED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: bholley, Assigned: emilio)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
(Reporter)

Description

9 months ago
Discovered this locally when loading https://reviewboard.mozilla.org/r/149970/diff/11#index_header in a debug build. Not sure exactly how benign it is, since this assertion was added since I last looked at this code.

DEBUG:style::invalidation::element::invalidator:  > Invalidation matched, next: Invalidation(.diff-container), (NextSibling)
DEBUG:style::invalidation::element::invalidator: StyleTreeInvalidator::invalidate_descendants(<span class="reviewer-name reviewer-ship-it review-granted"> (0x7f0fcef24ee0))
DEBUG:style::invalidation::element::invalidator:  > [Invalidation(:first-child.diff-container), Invalidation(:not(.diff-container)), Invalidation(:first-child.diff-container), Invalidation(:not(.diff-container))]
DEBUG:style::invalidation::element::invalidator: TreeStyleInvalidator::process_invalidation(<_moz_generated_content_after> (0x7f0fe0a38d30), Invalidation(:first-child.diff-container))
DEBUG:style::invalidation::element::invalidator: TreeStyleInvalidator::process_invalidation(<_moz_generated_content_after> (0x7f0fe0a38d30), Invalidation(:not(.diff-container)))
DEBUG:style::invalidation::element::invalidator:  > Invalidation matched, next: Invalidation(.diff-container), (NextSibling)
DEBUG:style::invalidation::element::invalidator: TreeStyleInvalidator::process_invalidation(<_moz_generated_content_after> (0x7f0fe0a38d30), Invalidation(:first-child.diff-container))
DEBUG:style::invalidation::element::invalidator: TreeStyleInvalidator::process_invalidation(<_moz_generated_content_after> (0x7f0fe0a38d30), Invalidation(:not(.diff-container)))
DEBUG:style::invalidation::element::invalidator:  > Invalidation matched, next: Invalidation(.diff-container), (NextSibling)
DEBUG:style::invalidation::element::invalidator: StyleTreeInvalidator::invalidate_descendants(<_moz_generated_content_after> (0x7f0fe0a38d30))
DEBUG:style::invalidation::element::invalidator:  > [Invalidation(:first-child.diff-container), Invalidation(:not(.diff-container)), Invalidation(:first-child.diff-container), Invalidation(:not(.diff-container))]
thread '<unnamed>' panicked at 'pseudos can't generate sibling invalidations, since using them in other position that isn't the rightmost part of the selector is invalid (for now at least)', /files/mozilla/mc/q/servo/components/style/invalidation/element/invalidator.rs:269
note: Run with `RUST_BACKTRACE=1` for a backtrace.
Redirecting call to abort() to mozalloc_abort
(Reporter)

Updated

9 months ago
Flags: needinfo?(emilio+bugs)
(Assignee)

Comment 1

9 months ago
It's bening... This assertion was written assuming that we'd only match ::before/::after against the corresponding pseudo, but that's of course not what we do, so it matches :first-child too... Worst case we invalidate that element's style, which seems fine.

I guess I can just drop the assertion, since pseudos are NAC roots, and thus they have no siblings to invalidate.
(Assignee)

Comment 2

9 months ago
I'm lying, in this case the relevant selector is:

.with-commit-msg-filediff :not(.diff-container) + .diff-container .sidebyside .filename-row th a:after.

We're invalidating the :not(.diff-container), presumably because some ancestor changed the "with-commit-msg-filediff" class. Some descendant of that element happens to be a ::before pseudo-element, so it matches :not(.diff-container), and creates the sibling invalidation.

No big deal, since it's dropped on the floor, but it can't match anything anyway. I can build a crashtest tomorrow and check-in the assertion-removal with a comment pointing to the other comment that mentions that we could save work gathering invalidations just for pseudos (which incidentally would make the assertion hold).

Leaving ni? for that.

Updated

9 months ago
Assignee: nobody → emilio+bugs
Priority: -- → P2
Duplicate of this bug: 1381724
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Flags: needinfo?(emilio+bugs)

Comment 6

9 months ago
mozreview-review
Comment on attachment 8887691 [details]
style: Remove bogus assertion.

https://reviewboard.mozilla.org/r/158588/#review163914

::: servo/components/style/invalidation/element/invalidator.rs:267
(Diff revision 1)
>          // Roots of NAC subtrees can indeed generate sibling invalidations, but
>          // they can be just ignored, since they have no siblings.
> -        debug_assert!(child.implemented_pseudo_element().is_none() ||

Can you mention in the comment here that we can end up checking invalidations against elements that normally would not match these selectors, such as element-backed pseudos, or other NAC (when the invalidation came from a rule in a document level style sheet), but that we just over-invalidate here rather than bothering to check these conditions.
Attachment #8887691 - Flags: review?(cam) → review+

Comment 7

9 months ago
mozreview-review
Comment on attachment 8887692 [details]
Bug 1381682: Crashtest.

https://reviewboard.mozilla.org/r/158590/#review163916
Attachment #8887692 - Flags: review?(cam) → review+
Summary: stylo: mozreview debug_assertion failureL pseudos can't generate sibling invalidations... → stylo: mozreview debug_assertion failure: pseudos can't generate sibling invalidations...
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8887691 - Attachment is obsolete: true
(Assignee)

Comment 9

9 months ago
Servo bits landed at https://github.com/servo/servo/pull/17781, the test should autoland when autoland is reopened.

Comment 11

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0f0faf30c690
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.