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

Status: RESOLVED FIXED

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

Description

[Chris Peterson [:cpeterson]]

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.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 3

[Bobby Holley (:bholley)]

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.

Comment 4

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1387953 - Exit early before calling Servo_HasAuthorSpecifiedRules without element data;

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

Comment 5

[Manish Goregaokar [:manishearth]]

Untested, waiting on a clean build. Do we have any idea how to repro this crash?

Comment 6

[Chris Peterson [:cpeterson]]

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 7

[Manish Goregaokar [:manishearth]]

Comment on attachment 8894639 [details]
Bug 1387953 - Exit early before calling Servo_HasAuthorSpecifiedRules without element data;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165796/diff/1-2/

Comment 8

[Bobby Holley (:bholley)]

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

Comment 9

[Bobby Holley (:bholley)]

And to be clear, the bug here is that the Gtk widget machinery is somehow calling

Comment 10

[Bobby Holley (:bholley)]

(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.

Comment 11

[Manish Goregaokar [:manishearth]]

Comment on attachment 8894639 [details]
Bug 1387953 - Exit early before calling Servo_HasAuthorSpecifiedRules without element data;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165796/diff/2-3/

Comment 12

[Bobby Holley (:bholley)]

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!

Comment 13

[Manish Goregaokar [:manishearth]]

I mean, geckolib code is gecko specific :)

Comment 14

[Bobby Holley (:bholley)]

(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.

Comment 15

[Bobby Holley (:bholley)]

(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.

Comment 16

[Manish Goregaokar [:manishearth]]

Comment on attachment 8894639 [details]
Bug 1387953 - Exit early before calling Servo_HasAuthorSpecifiedRules without element data;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165796/diff/3-4/

Comment 17

[Manish Goregaokar [:manishearth]]

try @ https://treeherder.mozilla.org/#/jobs?repo=try&revision=09956c52626716b290dd1a146aea950034636ef8

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2938b25e3b31
Exit early before calling Servo_HasAuthorSpecifiedRules without element data; r=bholley

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2938b25e3b31

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 21

[Manish Goregaokar [: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]:

Comment 22

[Gerry Chang [:gchang]]

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.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8ca8d29fa8ab