1376295 - Stylo: Crash in geckoservo::glue::Servo_StyleSheet_SizeOfIncludingThis

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Nicholas Nethercote [inactive]]

This bug was filed from the Socorro interface and is 
report bp-e2c46edb-c096-4387-b995-83a8b0170624.
=============================================================

9 reports of this crash signature from 3 installations, but it looks like all 3 installations are the same user, because the machine/OS fields are all the same.

Crash address is always 0x8. Looks like it's happening right at the C++/Rust border. I don't know how or why NonZeroPtrMut is involved.

heycam, any ideas?

Comment 1

[Andrew McCreight [:mccr8]]

Presumably this is somebody who has enabled Stylo.

Comment 2

[Bobby Holley (:bholley)]

The most likely cause would seem to be that mSheet is null here: https://hg.mozilla.org/mozilla-central/annotate/1466b62da7b2/layout/style/ServoStyleSheet.cpp#l71

Brad, can that happen?

In general, this is probably a less urgent crash, because it only happens when measuring memory.

Comment 3

[Brad Werth [:bradwerth]]

It could happen today with an @import stylesheet that failed to load due to a bad URL. Bug 1371453 tracks that issue. I'll get a short-term null check and a comment to clean things up when Bug 1371453 is fixed.

Comment 4

[Andrew McCreight [:mccr8]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #2
> In general, this is probably a less urgent crash, because it only happens
> when measuring memory.

Keep in mind that we do regularly collect memory reports automatically (for inclusion in crash reports).

Comment 5

[Bobby Holley (:bholley)]

(In reply to Brad Werth [:bradwerth] from comment #3)
> It could happen today with an @import stylesheet that failed to load due to
> a bad URL. Bug 1371453 tracks that issue. I'll get a short-term null check
> and a comment to clean things up when Bug 1371453 is fixed.

Great, thanks Brad!

(In reply to Andrew McCreight [:mccr8] from comment #4)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #2
> > In general, this is probably a less urgent crash, because it only happens
> > when measuring memory.
> 
> Keep in mind that we do regularly collect memory reports automatically (for
> inclusion in crash reports).

Ahah! Ok, good to know.

Comment 6

[Brad Werth [:bradwerth]]

Attachment: Bug 1376295 Part 1: Allow null raw sheets in ServoStyleSheet memory calculations.

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

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8881418 [details]
Bug 1376295 Part 1: Allow null raw sheets in ServoStyleSheet memory calculations.

https://reviewboard.mozilla.org/r/152574/#review157678

I'm fine with the if block itself, but please revise the comment to mention the valid case.

::: layout/style/ServoStyleSheet.cpp:71
(Diff revision 1)
> -  n += Servo_StyleSheet_SizeOfIncludingThis(ServoStyleSheetMallocSizeOf, mSheet);
> +  // There are known cases where mSheet can be null. Bug 1371453 is one such
> +  // case. When that bug lands, change the if into an assert to flush out
> +  // other cases.

I don't think this comment is correct.

Bug 1371453 is a case where an import rule may not have a sheet, rather than a stylesheet with inner which doesn't have raw sheet.

And, I don't think we can remove the if here in the future. It seems to me there is always valid case where `mSheet` can be null, because we create stylesheet and inners when we start loading, but we assign `mSheet` only when we parse it, so if the memory measurement happens between that, we would have a null `mSheet`.

Comment 8

[Brad Werth [:bradwerth]]

Comment on attachment 8881418 [details]
Bug 1376295 Part 1: Allow null raw sheets in ServoStyleSheet memory calculations.

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e1f88e3e239
Part 1: Allow null raw sheets in ServoStyleSheet memory calculations. r=xidorn

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3e1f88e3e239