1416282 - Crash in selectors::builder::complex_selector_specificity::simple_selector_specificity<T>

Status: RESOLVED WONTFIX

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

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is
report bp-384b6ec3-cac8-4575-ad38-a2f860171106.
=============================================================

Seen while looking at nightly crash stats - also present in 57: http://bit.ly/2iLfUOk. Many of the URLs seem to end in .aspx

2 URLs:
http://www.cmykonline.com.au/Menus.aspx 
https://www.youtube.com/watch?v=Pxhh44rbuqM 
https://imei24.com/blacklist_check/ 
https://www.digitalgujarat.gov.in/OfficeApp/PostApp/CitizenSide/StudentEntry_Citizen_Tribal.aspx 
http://www.charbi.com/net/media.aspx 
https://www.digitalgujarat.gov.in/OfficeApp/PostApp/CitizenSide/StudentEntry_Citizen_Tribal.aspx

Top 10 frames of crashing thread:

0 xul.dll std::panicking::rust_panic_with_hook src/libstd/panicking.rs:617
1 xul.dll std::panicking::begin_panic<str*> src/libstd/panicking.rs:572
2 xul.dll selectors::builder::complex_selector_specificity::simple_selector_specificity<style::gecko::selector_parser::SelectorImpl> z:/build/build/src/obj-firefox/toolkit/library/rust/<panic macros>:3
3 xul.dll selectors::parser::SelectorList<style::gecko::selector_parser::SelectorImpl>::parse<style::gecko::selector_parser::SelectorImpl, style::selector_parser::SelectorParser> servo/components/selectors/parser.rs:200
4 xul.dll geckoservo::glue::Servo_StyleSheet_FromUTF8Bytes servo/ports/geckolib/glue.rs:1067
5 xul.dll mozilla::ServoStyleSheet::ParseSheet layout/style/ServoStyleSheet.cpp:213
6 xul.dll mozilla::css::Loader::ParseSheet layout/style/Loader.cpp:1647
7 xul.dll mozilla::css::Loader::LoadInlineStyle layout/style/Loader.cpp:1913
8 xul.dll nsStyleLinkElement::DoUpdateStyleSheet dom/base/nsStyleLinkElement.cpp:548
9 xul.dll nsStyleLinkElement::UpdateStyleSheet dom/base/nsStyleLinkElement.cpp:334

=============================================================

Comment 1

[Astley Chen (inactive)]

Given that the crash volume is quite low on 57.0b, it shouldn't be a blocker for release 57.

Comment 2

[Cameron McCormack (:heycam)]

I haven't managed to reproduce.  I guess we're reaching this unreachable() in simple_selector_specificity:

https://searchfox.org/mozilla-central/rev/fe75164c55321e011d9d13b6d05c1e00d0a640be/servo/components/selectors/builder.rs#272

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Me neither, but... That should never ever reached, and looking at the code is not obvious to me why or how it should :)

We can try to print more information or something in the unreachable!() message, or in push_simple_selector...

Comment 4

[Marcia Knous [:marcia]]

Looks as if most of the crashes are Windows 7 (97%).

Comment 5

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Emilio, do you have time to take another look here? This is showing up in a low volume in crash-stats but we are still seeing it in 60 nightly.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

I took a look again. I still don't know how that code can be reached, and none of the URLs there could reproduce the problem (and there are urls there that don't look suspicious, like about:blank). So this needs to be some sort of memory corruption / hardware issue, which I think is not actionable as things stand unfortunately :(

Comment 7

[Mats Palmgren (inactive)]

> So this needs to be some sort of memory corruption / hardware issue

That seems unlikely to me given that we have ~3500 reported crashes
in total for these two signatures.

Could we add more diagnostic asserts around this crash?
(including more details if possible)

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1416282: Add diagnostics.

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

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

I wonder if anyone can have a look at the crash reports and see if there is any url which can trigger this.

In a debug build, this path should already be protected by the assertion in push_simple_selector, but it doesn't seem we have caught one from fuzzing? Maybe we want to upgrade that assertion for a while (in Nightly?) as well so that there is a chance we know what code path is the potential combinator comes from.

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8984705 [details]
Bug 1416282: Add diagnostics.

https://reviewboard.mozilla.org/r/250556/#review256856

::: servo/components/selectors/builder.rs:294
(Diff revision 1)
> -            Component::Combinator(..) => unreachable!(),
> +            Component::Combinator(ref combinator) => {
> +                unreachable!(
> +                    "Found combinator {:?} in simple selectors vector? {:?}",
> +                    combinator,
> +                    builder,
> +                );
> +            }

I'm concerned about this may be dumpping too much sensitive information for the purpose here. If we are going to revert it in very short time, it's probably fine I guess... Otherwise we should probably only dump the combinator.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #9)
> I wonder if anyone can have a look at the crash reports and see if there is
> any url which can trigger this.

I did and found nothing.

> In a debug build, this path should already be protected by the assertion in
> push_simple_selector, but it doesn't seem we have caught one from fuzzing?
> Maybe we want to upgrade that assertion for a while (in Nightly?) as well so
> that there is a chance we know what code path is the potential combinator
> comes from.

To be honest, I still don't know how this can happen without more garbage being around or a messup in the builder code...

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #10)
> Comment on attachment 8984705 [details]
> Bug 1416282: Add diagnostics.
> 
> https://reviewboard.mozilla.org/r/250556/#review256856
> 
> ::: servo/components/selectors/builder.rs:294
> (Diff revision 1)
> > -            Component::Combinator(..) => unreachable!(),
> > +            Component::Combinator(ref combinator) => {
> > +                unreachable!(
> > +                    "Found combinator {:?} in simple selectors vector? {:?}",
> > +                    combinator,
> > +                    builder,
> > +                );
> > +            }
> 
> I'm concerned about this may be dumpping too much sensitive information for
> the purpose here. If we are going to revert it in very short time, it's
> probably fine I guess... Otherwise we should probably only dump the
> combinator.

Can you elaborate? This only prints the parsed representation of the selector, which is roughly what we've done in similar places like bug 1374247. How's that sensitive?

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #11)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #10)
> > I'm concerned about this may be dumpping too much sensitive information for
> > the purpose here. If we are going to revert it in very short time, it's
> > probably fine I guess... Otherwise we should probably only dump the
> > combinator.
> 
> Can you elaborate? This only prints the parsed representation of the
> selector, which is roughly what we've done in similar places like bug
> 1374247. How's that sensitive?

That means you may be collecting a potentially complete selector, right?

Bug 1374247 is less concerned because all selectors reported there are likely only from CSS files we ship, which is basically identical to everyone. But here you are collecting (potentially) complete selector from content, which may be able to identify websites / pages users visit, which is appearantly a more sensitive data than a single combinator or a selector in an internal stylesheet.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #12)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #11)
> > (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #10)
> > > I'm concerned about this may be dumpping too much sensitive information for
> > > the purpose here. If we are going to revert it in very short time, it's
> > > probably fine I guess... Otherwise we should probably only dump the
> > > combinator.
> > 
> > Can you elaborate? This only prints the parsed representation of the
> > selector, which is roughly what we've done in similar places like bug
> > 1374247. How's that sensitive?
> 
> That means you may be collecting a potentially complete selector, right?
> 
> Bug 1374247 is less concerned because all selectors reported there are
> likely only from CSS files we ship, which is basically identical to
> everyone. But here you are collecting (potentially) complete selector from
> content, which may be able to identify websites / pages users visit, which
> is appearantly a more sensitive data than a single combinator or a selector
> in an internal stylesheet.

Hmm, ok, that's fair. In any case ideally we're going to need only a single crash report, so I think I'll land it for now and back out as soon as possible. If the diagnostic ends up not being useful we can remove it asap as well.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9f0fd50fb6d9
Add diagnostics. r=xidorn

Comment 15

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/9f0fd50fb6d9

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Just realized that there's no crash with this signature for anything later than FF60... Though I'm not aware of any related fix.

Comment 17

[[Back August 16] Ryan VanderMeulen]

I still see crashes on the 61 betas with this signature. We'll see what 62 does on Beta.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f654e4a9a64
Back out diagnostics for not being helpful enough at diagnosing.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e11449a47b08
Assert earlier to try to get a more helpful stack.

Comment 20

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/7f654e4a9a64
https://hg.mozilla.org/mozilla-central/rev/e11449a47b08

Comment 21

[Release mgmt bot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:svoisen, maybe it's time to close this bug?

Comment 22

[Release mgmt bot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:svoisen, maybe it's time to close this bug?

Comment 23

[Sean Voisen (:svoisen)]

Emilio: Any reason to leave open?

Comment 24

[Emilio Cobos Álvarez (:emilio)]

This only has a couple, win7-only crashes, most of them startup ones... so this is likely to be just some broken binary or some bug external to firefox. I don't think this is actionable by us.