Closed Bug 1391577 Opened 7 years ago Closed 7 years ago

stylo: Crash in mozalloc_abort | abort | style::invalidation::element::invalidator::TreeStyleInvalidator<T>::process_invalidation<T>

Categories

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

Unspecified
All
defect

Tracking

()

VERIFIED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: bbouvier, Assigned: emilio)

References

(Blocks 2 open bugs, )

Details

(Keywords: crash, regression)

Crash Data

Attachments

(3 files, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-cf74c5f1-b522-4842-b227-c27870170818.
=============================================================

I've got an insta crash on the following website, always with the same backtrace it seems: http://velvetyne.fr/

It's a panic in rust, looks like a signature from Stylo code.

Please let me know if you need more information.
I can reproduce it. Chris, this might be interesting to you!
Flags: needinfo?(cpeterson)
Keywords: regression
Thanks. I can reliably reproduce this crash on Windows by loading http://velvetyne.fr/. If I disable Stylo, then the website loads without crashing.

Emilio, is this crash related to your "better-style-invalidation" changes in https://hg.mozilla.org/mozilla-central/rev/bdc22078e9f8?
Flags: needinfo?(cpeterson) → needinfo?(emilio+bugs)
OS: Linux → All
Priority: -- → P1
Summary: Crash in mozalloc_abort | abort | style::invalidation::element::invalidator::TreeStyleInvalidator<T>::process_invalidation<T> → stylo: Crash in mozalloc_abort | abort | style::invalidation::element::invalidator::TreeStyleInvalidator<T>::process_invalidation<T>
Awesome, so this is the "Someone messed up selector parsing" assertion, thanks for finding a repro!
Assignee: nobody → emilio+bugs
Flags: needinfo?(emilio+bugs)
Depends on: 1373800
Comment on attachment 8898918 [details]
style: Skip state pseudo-classes when finding a pseudo-element.

https://reviewboard.mozilla.org/r/170252/#review175612

::: servo/components/style/invalidation/element/invalidator.rs:603
(Diff revision 1)
>                              .iter_raw_parse_order_from(next_combinator_offset - 1)
> -                            .next()
> -                            .unwrap();
> +                            .flat_map(|component| {
> +                                match *component {

Code might be clearer if the previous structure is kept, and you insert this before the .next():

  .skip_while(|c| matches!(c, Component::NonTSPseudoClass(..))

?
Attachment #8898918 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #7)
> Comment on attachment 8898918 [details]
> style: Skip state pseudo-classes when finding a pseudo-element.
> 
> https://reviewboard.mozilla.org/r/170252/#review175612
> 
> ::: servo/components/style/invalidation/element/invalidator.rs:603
> (Diff revision 1)
> >                              .iter_raw_parse_order_from(next_combinator_offset - 1)
> > -                            .next()
> > -                            .unwrap();
> > +                            .flat_map(|component| {
> > +                                match *component {
> 
> Code might be clearer if the previous structure is kept, and you insert this
> before the .next():
> 
>   .skip_while(|c| matches!(c, Component::NonTSPseudoClass(..))
> 
> ?

Agreed :)
Attachment #8898918 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/b45d12e72a83
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
In favor of beta 56 dogfooding, please create an uplift request to beta. Thanks.
Flags: needinfo?(emilio+bugs)
Approval Request Comment
[Feature/Bug causing the regression]: bug 1373800
[User impact if declined]: Crashes with stylo enabled
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Not risky
[Why is the change risky/not risky?]: Fairly trivial and hard to reach case.
[String changes made/needed]: none
Flags: needinfo?(emilio+bugs)
Attachment #8899474 - Flags: approval-mozilla-beta?
Comment on attachment 8899474 [details] [diff] [review]
Patch for uplift.

Crash fix for stylo issue, includes a new test. Let's uplift for 56 beta 5.
Attachment #8899474 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Emilio Cobos Álvarez [:emilio] from comment #15)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Emilio's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
http://velvetyne.fr/ does not crash anymore. Nightly 57 x64 20170822100529 @ Debian Testing.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.