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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Bobby Holley (:bholley)]

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

Comment 1

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: style: Remove bogus assertion.

Review commit: https://reviewboard.mozilla.org/r/158588/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/158588/

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1381682: Crashtest.

Review commit: https://reviewboard.mozilla.org/r/158590/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/158590/

Comment 6

[Cameron McCormack (:heycam)]

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.

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8887692 [details]
Bug 1381682: Crashtest.

https://reviewboard.mozilla.org/r/158590/#review163916

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8887692 [details]
Bug 1381682: Crashtest.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158590/diff/1-2/

Comment 9

[Emilio Cobos Álvarez (:emilio)]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f0faf30c690
Crashtest. r=heycam

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/0f0faf30c690