1619180 - Crash in [@ GlyphDataElement<T>::TryGetExistingGlyph] EXCEPTION_IN_PAGE_ERROR_READ

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect, P2)

Description

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Reporting from a post on reddit: http://reddit.com/r/firefox/comments/fbvprm/7301_tabs_crashing_for_no_reason/

This bug is for crash report bp-0646bf14-01b0-458f-940d-0120a0200302.

Top 10 frames of crashing thread:

0 dwrite.dll GlyphDataElement<GlyphMetricsLayout, GlyphMetricsRasterizationParameters, GlyphMetricsRasterizationState>::TryGetExistingGlyph 
1 dwrite.dll GlyphDataElement<GlyphMetricsLayout, GlyphMetricsRasterizationParameters, GlyphMetricsRasterizationState>::GetExistingGlyphs 
2 dwrite.dll GlyphDataElement<GlyphMetricsLayout, GlyphMetricsRasterizationParameters, GlyphMetricsRasterizationState>::GetGlyphData 
3 dwrite.dll GetGlyphMetrics 
4 dwrite.dll FontFaceElement::GetGlyphMetrics 
5 dwrite.dll FontFaceElement::GetGlyphAdvances 
6 dwrite.dll DWriteFontFace::GetGdiCompatibleGlyphAdvances 
7 xul.dll gfxDWriteFont::MeasureGlyphWidth gfx/thebes/gfxDWriteFonts.cpp:524
8 xul.dll gfxDWriteFont::GetGlyphWidth gfx/thebes/gfxDWriteFonts.cpp:494
9 xul.dll hb_font_get_glyph_h_advances_default gfx/harfbuzz/src/hb-font.cc:240

Comment 1

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I found bug 1392911 which looks similar, but it was a startup crash in Thunderbird and it seems to have died off a year or so back.

Comment 2

[Aria Beingessner [:Gankra]]

Could someone with security authorization lookup some URLs associated with these crashes so we can try to reproduce it?

Comment 3

[Jessie [:jbonisteel] pls NI]

Jeff - do you have that level of access?

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Here are some urls:
https://www.reddit.com/r/firefox/
https://www.reddit.com/r/FirefoxCSS/
https://www.cadena3.com/
https://www.idnes.cz/

Comment 5

[Jeff Muizelaar [:jrmuizel]]

These are mostly EXCEPTION_IN_PAGE_ERROR_READ / STATUS_DEVICE_DATA_ERROR with other crashes like:
1 EXCEPTION_IN_PAGE_ERROR_READ / STATUS_DEVICE_DATA_ERROR 1577 82.91 %
2 EXCEPTION_IN_PAGE_ERROR_READ / STATUS_IN_PAGE_ERROR 148 7.78 %
3 EXCEPTION_IN_PAGE_ERROR_READ / STATUS_IO_DEVICE_ERROR 102 5.36 %
4 EXCEPTION_IN_PAGE_ERROR_READ / STATUS_DEVICE_HARDWARE_ERROR 40 2.10 %
5 EXCEPTION_IN_PAGE_ERROR_READ / STATUS_NO_SUCH_DEVICE 8 0.42 %

This suggests disk errors.

Comment 6

[Aria Beingessner [:Gankra]]

Can we do anything about disk errors?

Comment 7

[Jonathan Kew [:jfkthame]]

In theory, presumably we could wrap the relevant Windows API calls with __try / __except, and just return zero for the glyph width if an exception occurs. If we also mark the gfxFont as invalid and its gfxFontEntry as having an empty cmap, we should give up trying to use it, and fall back to something else, I think.

Comment 8

[Aria Beingessner [:Gankra]]

I'm kind of torn on how to prioritize this, so I'm gonna bump it up to P2 and we can bat it back down if it's not a big deal / too much work to handle.

It feels like this is a minor-ish quality-of-implementation thing to survive these kinds of errors, but I might be completely off-base.

Comment 9

[Jonathan Kew [:jfkthame]]

Seems like it should be straightforward to do this in gfxDWriteFont::MeasureGlyphWidth, if it works to use __try / __except to handle the error there; I do wonder, though, how likely it is that we'll just end up crashing somewhere else if we're running on flaky hardware.

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1619180 - Speculative patch, add exception handling around gfxDWriteFont::MeasureGlyphWidth. r=jrmuizel

Comment 11

[Jonathan Kew [:jfkthame]]

Something like this might help, though it's kinda speculative -- I don't have a machine that's exhibiting crashes like this to actually test the result. But it seems unlikely to make things worse, at least.

(I notice we already do this kind of thing elsewhere, e.g. here.)

Comment 12

[Jeff Muizelaar [:jrmuizel]]

What happens if it's the UI font that we get this on?

Comment 13

[Jonathan Kew [:jfkthame]]

Umm... not really sure, presumably we'd end up displaying UI with a fallback font?

Comment 14

[Jonathan Kew [:jfkthame]]

By the way, I tried manually forcing "failure" to occur for the Segoe UI font, and as expected, ended up with the browser UI in Times New Roman as a fallback. Doesn't look great, but at least it's still a functioning browser.

(In practice, it's very unlikely people are hitting this with the system's primary UI font, as if there's a bad sector (or whatever) in that font, it'd probably tend to make the whole OS unusable pretty fast.)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/680ae49f7365
Speculative patch, add exception handling around gfxDWriteFont::MeasureGlyphWidth. r=jrmuizel

Comment 16

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/680ae49f7365

Comment 17

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

The patch landed in nightly and beta is affected.
:jfkthame, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 18

[Jonathan Kew [:jfkthame]]

Comment on attachment 9132070 [details]
Bug 1619180 - Speculative patch, add exception handling around gfxDWriteFont::MeasureGlyphWidth. r=jrmuizel

Beta/Release Uplift Approval Request

• User impact if declined: Potential crash within DirectWrite
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: Unable to explicitly test this; we believe the issue may be caused by hardware errors leading to crashes within dwrite.dll, but there are no specific STR.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just adds exception handling around a DirectWrite call, similar to what we have in other places already; no change in behavior unless the exception happens.
• String changes made/needed: none

Comment 19

[Jonathan Kew [:jfkthame]]

I don't see any recent crash reports from Nightly, so it's impossible to tell how much this actually helps; we probably don't have any Nightly users with affected machines. But the patch at least can't make things worse than the current situation. Worst-case would be that the browser displays a fallback font for some content (or even UI), or possibly crashes on a different DirectWrite API after surviving this call.

So IMO it'd be worth uplifting this so as to get it into the beta channel; there's no benefit in having longer on Nightly first given the lack of affected Nightly users.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9132070 [details]
Bug 1619180 - Speculative patch, add exception handling around gfxDWriteFont::MeasureGlyphWidth. r=jrmuizel

Speculative fix for a directwrite crash adding a better fallback if loading fails. Approved for 75.0b6.

Comment 21

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f574758bb5ce