1461180 - Crash in nsFont::CalcDifference

Status: RESOLVED INCOMPLETE

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

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-231999d9-dd74-4680-b40a-abf630180510.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll nsFont::CalcDifference gfx/src/nsFont.cpp:70
1 xul.dll nsStyleFont::CalcDifference layout/style/nsStyleStruct.cpp:210
2 xul.dll nsStyleContext::CalcStyleDifference layout/style/nsStyleContext.cpp:235
3 xul.dll Gecko_CalcStyleDifference layout/style/ServoBindings.cpp:384
4 xul.dll style::gecko::restyle_damage::GeckoRestyleDamage::compute_style_difference servo/components/style/gecko/restyle_damage.rs:53
5 xul.dll style::matching::PrivateMatchMethods::accumulate_damage_for<style::gecko::wrapper::GeckoElement> servo/components/style/matching.rs:319
6 xul.dll style::traversal::compute_style<style::gecko::wrapper::GeckoElement> servo/components/style/traversal.rs:573
7 xul.dll geckoservo::glue::traverse_subtree servo/ports/geckolib/glue.rs:284
8 xul.dll geckoservo::glue::Servo_TraverseSubtree servo/ports/geckolib/glue.rs:342
9 xul.dll mozilla::ServoStyleSet::StyleDocument layout/style/ServoStyleSet.cpp:953

=============================================================

this is a low volume crash signature that's showing up since firefox 58 cross-platform, mostly in the content process.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Note that 61 and 62 are (currently) only showing null-derefs.  there are wildptr 60bN crashes; no 60.0 crashes so far.  So something may be better in 61... (and perhaps/maybe 60 release)

Comment 2

[Daniel Veditz [:dveditz]]

Philipp: Why does this block bug 1414926?

Emilio: any idea what might have changed in Stylo to turn what looks like a dangerous crash <=60 into a safe null deref? If so we probably want to port that to ESR-60.

Comment 3

[[:philipp]]

(In reply to Daniel Veditz [:dveditz] from comment #2)
> Philipp: Why does this block bug 1414926?
sorry, not much too much thinking has gone into this. i was just hooking up that bug because the crashing code has last been touched by https://hg.mozilla.org/releases/mozilla-release/rev/7279b117245b 
but perhaps it's not a real regression but a signature change from bug 1353657

Comment 4

[Markus Stange [:mstange]]

(In reply to [:philipp] from comment #3)
> but perhaps it's not a real regression but a signature change from bug
> 1353657

Oh, yes, that sounds very likely.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Stylo in general changed a bunch of stuff, including how lifetimes of style structs are managed and how the structs are allocated.

That being said, I can't think (without external memory corruption or what not) on something that would cause this crash.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Randell Jesup [:jesup] from comment #1)
> Note that 61 and 62 are (currently) only showing null-derefs.  there are
> wildptr 60bN crashes; no 60.0 crashes so far.  So something may be better in
> 61... (and perhaps/maybe 60 release)

We did get 1 wildptr 61bN crash: https://crash-stats.mozilla.com/report/index/0f1264d1-9fdf-4f83-8362-6729d0180521
All other 61 and 62 crashes are still nullptrs, so something changed in 61, but it's looking like non-nullptr are still possible.  (it is on the same line, so likely it's not a bitflip error)

Comment 7

[Maire Reavy [:mreavy]]

This does look like a signature change of the graphics bug 1353657 (ref: comment 3).  --> Moving to graphics:text

Comment 8

[Daniel Veditz [:dveditz]]

I still see some 62 nightly null crashes, but no 62 beta crashes.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

The 62 alpha crashes are from 3 installations, and given volumes I don't see the lack of wildptr addresses in 62 indicative of anything.  in 61b with far higher crash numbers, I see mostly nullptr but several wildptr crashes.  I think this should still be a sec-high, and csectype-wildptr.

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

It seems interesting that the crash is always on the line:
      (alternateValues != aOther.alternateValues) ||
which is not the first TArray comparison in nsFont::CalcDifference.

Comment 11

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Though loading the minidump for bp-5bc1bcf4-e4d0-4fdb-ad60-dd2060180606, Visual Studio 2017 thinks it's actually aOther.fontVariationSettings.mHdr that's bad.  And that also seems consistent with the way it associates a big chunk of the function with a big chunk of the disassembly rather than having finer-grained connections.

Comment 12

[[:philipp]]

this is rising in volume after 61 went to release. 

we have some affected users on sumo, who repeatedly crash on particular sites (also with different other signatures) in case they could help with troubleshooting:
https://support.mozilla.org/en-US/questions/1224807
https://support.mozilla.org/en-US/questions/1224157

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

Lee any thoughts on next steps?

Comment 14

[Daniel Veditz [:dveditz]]

While the vast majority (~95%) of these crashes are null de-refs, wildptr and UAF signatures persist into 63 beta and 64 nightly (e.g. bp-571345fe-a5f6-4649-b58e-4f3ae0180909).

CalcDifferences seems to be operating on a struct with simple comparisons (even on some of the members which are things like nsTArray) and then returning an enum so maybe this particular signature isn't so scary even when it is a UAF. If something is freed or bogus would worse effects be showing up elsewhere, perhaps tracked in other bugs?

Comment 15

[Lee Salzman [:lsalzman]]

This ideally belongs in Layout: Text given that it seems to be an issue with styles either not having a font, or an invalid font. Jonathan?

Comment 16

[Jonathan Kew [:jfkthame]]

It looks like we must have some kind of corruption of an nsFont struct, or one of its constituent arrays; but I don't have any idea how that would arise. I suppose UAF may be the likeliest scenario. My hunch is that it may yet turn out to be stylo-related, as suggested in early comments here. Emilio, any idea of how we could try to move forward here?

Comment 17

[Emilio Cobos Álvarez [:emilio]]

Not really, I don't have great ideas, the code that fills the array mentioned in comment 11 looks sound to me. Maybe cam can run his tool to find memory corruption issues on some of the reports here to shed some light?

Comment 18

[Maire Reavy [:mreavy]]

Adding Jessie (new graphics engineering manager) to all sec-crit and sec-high graphics bugs

Comment 19

[Cameron McCormack (:heycam)]

I ran the 10 most recent crashes (yesterday) through chkimg and none had detected memory corruption around the crashing IP.

Comment 20

[Sean Voisen (:svoisen)]

Sounds like we might be stalled on this for now given comment 19. And looks like no crashes in 67.

@emilio: I know you were looking at font loading code recently. Any other ideas 4 months later?

Comment 21

[Emilio Cobos Álvarez [:emilio]]

Not really, unfortunately.

Comment 22

[Sean Voisen (:svoisen)]

Marking stalled and will keep it in our queue to revisit.

Comment 23

[David Lawrence [:dkl]]

Removing employee no longer with company from CC list of private bugs.

Comment 24

[Daniel Holbert [:dholbert]]

Downgrading to S3, given low crash volume* and given the fact that we marked this as stalled.

For the past 2 weeks:

• nsFont::CalcDifference has 9 crashes (only 3 of which were in supported versions, i.e. 102esr or 107 release)
• nsFont::nsFont has 3 crashes, all for versions 102-103
• nsFont::~nsFont has 4 crashes, 3 of which are for Thunderbird 102 (the last being Firefox88)

So we're roughly 1 crash per day, with a substantial portion coming from old/unsupported versions.

Comment 25

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:emilio, could you consider increasing the severity of this security bug?

For more information, please visit auto_nag documentation.

Comment 26

[Emilio Cobos Álvarez [:emilio]]

See above

Comment 27

[Tom Ritter [:tjr]]

We are working to close out Stalled bugs that cannot be actioned, and this one is getting about 1-3 crashes per release, so not a common issue.

Comment 28

[Daniel Holbert [:dholbert]]

For this bug's nsFont::~nsFont crash-signature at least: the past 6 months' crashes for that signature were all for major-versions 102 and 99. That one also had no crashes after v102 when I looked at a 2-week sample of crash data in December of last year, too, in comment 24.

So that signature in particular was likely fixed post-102 (or moved to a different crash signature).

Comment 29

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

Since the bug is closed, the stalled keyword is now meaningless.
For more information, please visit BugBot documentation.