Closed
Bug 1416282
Opened 5 years ago
Closed 2 years ago
Crash in selectors::builder::complex_selector_specificity::simple_selector_specificity<T>
Categories
(Core :: CSS Parsing and Computation, defect, P2)
Tracking
()
People
(Reporter: marcia, Unassigned)
Details
(Keywords: crash, regression, testcase-wanted)
Crash Data
Attachments
(1 file)
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•5 years ago
|
||
Given that the crash volume is quite low on 57.0b, it shouldn't be a blocker for release 57.
Priority: -- → P2
Comment 2•5 years ago
|
||
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•5 years ago
|
||
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
Reporter | ||
Comment 4•5 years ago
|
||
Looks as if most of the crashes are Windows 7 (97%).
status-firefox59:
--- → fix-optional
status-firefox60:
--- → affected
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)
Comment 6•5 years ago
|
||
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)
Reporter | ||
Updated•4 years ago
|
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>]
Comment 7•4 years ago
|
||
> 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 hidden (mozreview-request) |
Comment 9•4 years ago
|
||
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•4 years ago
|
||
mozreview-review |
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+
Comment 11•4 years ago
|
||
(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•4 years ago
|
||
(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•4 years ago
|
||
(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.
Updated•4 years ago
|
Keywords: leave-open
Comment 14•4 years ago
|
||
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/9f0fd50fb6d9 Add diagnostics. r=xidorn
Comment 15•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9f0fd50fb6d9
Comment 16•4 years ago
|
||
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•4 years ago
|
||
I still see crashes on the 61 betas with this signature. We'll see what 62 does on Beta.
Comment 18•4 years ago
|
||
Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/mozilla-inbound/rev/7f654e4a9a64 Back out diagnostics for not being helpful enough at diagnosing.
Comment 19•4 years ago
|
||
Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/mozilla-inbound/rev/e11449a47b08 Assert earlier to try to get a more helpful stack.
Comment 20•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7f654e4a9a64 https://hg.mozilla.org/mozilla-central/rev/e11449a47b08
Comment 21•3 years ago
|
||
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)
Comment 22•3 years ago
|
||
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)
Comment 23•2 years ago
|
||
Emilio: Any reason to leave open?
Flags: needinfo?(svoisen)
Flags: needinfo?(emilio)
Comment 24•2 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(emilio)
Resolution: --- → WONTFIX
Updated•2 years ago
|
Keywords: leave-open
Updated•6 months ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•