Closed Bug 1387953 Opened 7 years ago Closed 7 years ago

stylo: Crash in mozalloc_abort | abort | core::option::expect_failed | geckoservo::glue::Servo_HasAuthorSpecifiedRules

Categories

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

Unspecified
All
defect

Tracking

()

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

People

(Reporter: cpeterson, Assigned: manishearth)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is report bp-83cc9fe3-e3d2-4988-b33b-9bebc0170807. ============================================================= There were about 17 crash reports with this signature. The earliest build ID was 20170722112649.
So presumably the element is becoming unstyled somehow. However, Servo_HasAuthorSpecifiedRules basically just operates on the primary style. So we can just grab that directly off the frame (aFrame->StyleContext()->AsServo()), and then pass that to Rust, side-stepping this crash. Should be a trivial patch. Manish, can you write that up? http://searchfox.org/mozilla-central/rev/b52285fffc13f36eca6b47de735d4e4403b3859e/layout/base/nsPresContext.cpp#2282
Flags: needinfo?(manishearth)
Removing crashes from crash-stats (i.e. where we don't have somebody filing with STR) from stylo-site-issues, we can track crash-stats separately.
No longer blocks: stylo-site-issues
Untested, waiting on a clean build. Do we have any idea how to repro this crash?
Flags: needinfo?(manishearth)
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
I don't have STR, but three of the crash reports' URLs are direct links to (NSFW) video files (.mp4 or .webm URLs). Perhaps Firefox's full-page video player controls can trigger this crash? OTOH, there are also crash reports with non-video URLs like https://tweetdeck.twitter.com and about:blank.
Comment on attachment 8894639 [details] Bug 1387953 - Exit early before calling Servo_HasAuthorSpecifiedRules without element data; https://reviewboard.mozilla.org/r/165796/#review170980 (In reply to Manish Goregaokar [:manishearth] from comment #5) > Untested, waiting on a clean build. Do we have any idea how to repro this > crash? Probably not worth spending too much time on. I just realized that the HASR implementation still can rely on the style state of elements in the DOM, when walking up the parent chain [1], which means the approach here will only work of the unstyled element's parent has styles. So I'm included to just check HasServoData at the callsite, and warn + return |false| if there are no styles, which will cause us to use native styles in those cases (which I think is a reasonably-safe fallback). Sorry for the misdirection. :-( [1] http://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/servo/components/style/rule_tree/mod.rs#1240
Attachment #8894639 - Flags: review?(bobbyholley)
And to be clear, the bug here is that the Gtk widget machinery is somehow calling
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #9) > And to be clear, the bug here is that the Gtk widget machinery is somehow > calling Er, somehow calling into HasAuthorSpecifiedRules for an unstyled element during reflow. Not entirely sure how that happens, and doing the check I propose might just move the crash elsewhere, but it seems worth a shot.
Priority: P1 → P3
Comment on attachment 8894639 [details] Bug 1387953 - Exit early before calling Servo_HasAuthorSpecifiedRules without element data; https://reviewboard.mozilla.org/r/165796/#review171496 ::: servo/ports/geckolib/glue.rs:1628 (Diff revision 3) > - let data = > - element.borrow_data() > - .expect("calling Servo_HasAuthorSpecifiedRules on an unstyled element"); > + let data if let Some(data) = element.borrow_data() { > + data > + } else { > + return false; > + }; Nit: This would probably be better to do in the caller with a HasServoData() check, since it's gecko-specific, and thus doesn't really need to be in the servo tree, but either way. Please add a comment referencing the bug and indicating that this probably shouldn't happen, but appeared in some crash reports. Thanks!
Attachment #8894639 - Flags: review?(bobbyholley) → review+
I mean, geckolib code is gecko specific :)
(In reply to Manish Goregaokar [:manishearth] from comment #13) > I mean, geckolib code is gecko specific :) Yes, but there is some code that needs to be in Rust. In general given the choice, if there's something gecko-specific we can do in C++, that's strictly preferable IMO.
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #14) > (In reply to Manish Goregaokar [:manishearth] from comment #13) > > I mean, geckolib code is gecko specific :) > > Yes, but there is some code that needs to be in Rust. In general given the > choice, if there's something gecko-specific we can do in C++, that's > strictly preferable IMO. In particular, it means that somebody coming along and deciding to remove this check later can land the fix with less hassle.
Pushed by manishearth@gmail.com: https://hg.mozilla.org/integration/autoland/rev/2938b25e3b31 Exit early before calling Servo_HasAuthorSpecifiedRules without element data; r=bholley
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Seems like we want Stylo stability fixes on 56 before we start running experiments. Please nominate this for Beta approval when you get a chance.
Flags: needinfo?(manishearth)
Comment on attachment 8894639 [details] Bug 1387953 - Exit early before calling Servo_HasAuthorSpecifiedRules without element data; Approval Request Comment [Feature/Bug causing the regression]: Unknown [User impact if declined]: Occasional crashes on stylo beta [Is this code covered by automated tests?]: The crash is not (we don't know what exactly causes it), but the nearby code is [Has the fix been verified in Nightly?]: Yes, no longer seems to occur (nor is it possible, the assert has been removed) [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: No [Is the change risky?]: No [Why is the change risky/not risky?]: [String changes made/needed]:
Flags: needinfo?(manishearth)
Attachment #8894639 - Flags: approval-mozilla-beta?
Comment on attachment 8894639 [details] Bug 1387953 - Exit early before calling Servo_HasAuthorSpecifiedRules without element data; Fix a stylo crash. Beta56+. Should be in 56.0b3.
Attachment #8894639 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: