Closed
Bug 1376295
Opened 7 years ago
Closed 7 years ago
Stylo: Crash in geckoservo::glue::Servo_StyleSheet_SizeOfIncludingThis
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox54 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: bradwerth)
References
Details
(Keywords: crash)
Crash Data
Attachments
(1 file)
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?
Flags: needinfo?(cam)
Comment 1•7 years ago
|
||
Presumably this is somebody who has enabled Stylo.
Summary: Crash in geckoservo::glue::Servo_StyleSheet_SizeOfIncludingThis → Stylo: Crash in geckoservo::glue::Servo_StyleSheet_SizeOfIncludingThis
Comment 2•7 years ago
|
||
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.
Flags: needinfo?(cam) → needinfo?(bwerth)
Assignee | ||
Comment 3•7 years ago
|
||
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.
Assignee: nobody → bwerth
Depends on: 1371453
Comment 4•7 years ago
|
||
(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•7 years ago
|
||
(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 hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8881418 -
Flags: review?(xidorn+moz)
Comment 7•7 years ago
|
||
mozreview-review |
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`.
Attachment #8881418 -
Flags: review?(xidorn+moz) → review+
Comment hidden (mozreview-request) |
Pushed by bwerth@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3e1f88e3e239 Part 1: Allow null raw sheets in ServoStyleSheet memory calculations. r=xidorn
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3e1f88e3e239
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(bwerth)
Updated•7 years ago
|
status-firefox54:
--- → unaffected
status-firefox55:
--- → unaffected
status-firefox-esr52:
--- → unaffected
You need to log in
before you can comment on or make changes to this bug.
Description
•