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
https://hg.mozilla.org/mozilla-central/rev/2938b25e3b31
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.