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

NEW
Unassigned

Status

()

defect
P2
critical
2 years ago
3 months ago

People

(Reporter: marcia, Unassigned, NeedInfo)

Tracking

(4 keywords)

Trunk
x86
Windows 7
Points:
---

Firefox Tracking Flags

(firefox-esr60 affected, firefox57 wontfix, firefox58 wontfix, firefox59 wontfix, firefox60 wontfix, firefox61 fix-optional, firefox62 ?)

Details

(crash signature)

Attachments

(1 attachment)

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&lt;str*&gt; src/libstd/panicking.rs:572
2 xul.dll selectors::builder::complex_selector_specificity::simple_selector_specificity&lt;style::gecko::selector_parser::SelectorImpl&gt; z:/build/build/src/obj-firefox/toolkit/library/rust/&lt;panic macros&gt;:3
3 xul.dll selectors::parser::SelectorList&lt;style::gecko::selector_parser::SelectorImpl&gt;::parse&lt;style::gecko::selector_parser::SelectorImpl, style::selector_parser::SelectorParser&gt; 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

=============================================================
Given that the crash volume is quite low on 57.0b, it shouldn't be a blocker for release 57.
Priority: -- → P2
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
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...
Keywords: testcase-wanted
Looks as if most of the crashes are Windows 7 (97%).
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.
Flags: needinfo?(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 :(
Flags: needinfo?(emilio)
Crash Signature: [@ selectors::builder::complex_selector_specificity::simple_selector_specificity<T>] → [@ selectors::builder::complex_selector_specificity::simple_selector_specificity<T>] [@ static void selectors::builder::complex_selector_specificity::simple_selector_specificity<T>]
> 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)
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 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.
Attachment #8984705 - Flags: review?(xidorn+moz) → review+
(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?
(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.
(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.
Just realized that there's no crash with this signature for anything later than FF60... Though I'm not aware of any related fix.
I still see crashes on the 61 betas with this signature. We'll see what 62 does on Beta.
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f654e4a9a64
Back out diagnostics for not being helpful enough at diagnosing.
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e11449a47b08
Assert earlier to try to get a more helpful stack.

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

Flags: needinfo?(svoisen)
You need to log in before you can comment on or make changes to this bug.