1485425 - Crash in ComputeCrc32 in dwrite.dll via gfxDWriteFontEntry::CreateFontFace

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[Wayne Mery (:wsmwk)]

Both Firefox and Thunderbird.  All Windows

bp-8b5fc57d-0c2e-498d-bbbf-8d3480180816 firefox 61
=============================================================
Top 10 frames of crashing thread:

0 dwrite.dll ComputeCrc32 
1 dwrite.dll ComputeContiguousElementCheckSum 
2 dwrite.dll CacheReader::ValidateCheckSum 
3 dwrite.dll ClientSideCacheContext::FindInSharedCache 
4 dwrite.dll ClientSideCacheContext::FindElement 
5 dwrite.dll ElementTaskList::GetElementData 
6 dwrite.dll IBaseCacheContext::GetElementData 
7 dwrite.dll FontFaceElement::FontFaceElement 
8 dwrite.dll DWriteFontFace::DWriteFontFace 
9 dwrite.dll DWriteFontFace::Create 
10 	dwrite.dll	DWriteFont::CreateFontFace(IDWriteFontFace**)
11 	xul.dll	gfxDWriteFontEntry::CreateFontFace(IDWriteFontFace**, gfxFontStyle const*, DWRITE_FONT_SIMULATIONS)
12 	xul.dll	gfxDWriteFontEntry::HasVariations()
13 	xul.dll	gfxDWriteFontEntry::CreateFontInstance(gfxFontStyle const*)
14 	xul.dll	gfxFontEntry::FindOrMakeFont(gfxFontStyle const*, gfxCharacterMap*)
=============================================================
gfxDWriteFontEntry
    // initialize mFontFace if this hasn't been done before
    if (!mFontFace) {
        HRESULT hr;
        if (mFont) {
            hr = mFont->CreateFontFace(getter_AddRefs(mFontFace));

bp-f14116a3-e314-4778-b79a-f49a10180818 Firefox 62
bp-ca7fdd46-7764-46fd-a5c7-7956a0180718 Thunderbird 52.9.1
bp-bb51c88e-29c2-4e11-945f-335d30180821 Thunderbird 60.0

Comment 1

[Nicolas Silva [:nical]]

Tentatively dispatching to Lee (because fonts), don't hesitate to reassign.

Comment 2

[Wayne Mery (:wsmwk)]

related to bug 1468845?

Comment 3

[Jonathan Kew [:jfkthame]]

Looks to me like this has been going on "forever", e.g.:

https://crash-stats.mozilla.com/report/index/1df94535-2854-4fb3-b66d-2189d0180817  --  Firefox 37.0b2 (20150302192546)

This report is from Win10, but has a Firefox install age of 2 weeks: someone installed a really old version on a Win10 machine for whatever reason, and that hits the same crash.

Similarly, here's one from Firefox 21.0 on Win7:

https://crash-stats.mozilla.com/report/index/fa1891a0-8269-4fc7-86fa-865c40180820

My guess is this might be triggered by a corrupt font file installed on the user's system, resulting in some kind of confusion in DirectWrite's cache.

Comment 4

[Wayne Mery (:wsmwk)]

https://support.mozilla.org/en-US/questions/1231323 bp-6ae069f3-ba3b-4726-b103-3837e0180828

bp-bb51c88e-29c2-4e11-945f-335d30180821  perhaps from windows update?

Comment 5

[Lee Salzman [:lsalzman]]

Don't know offhand how to resolve this, so unassigned myself for now.

Comment 6

[Wayne Mery (:wsmwk)]

1       6.1.7601 SP1	90	40.00 %
2	10.0.17134	69	30.67 %
3	10.0.15063	42	18.67 %
4	6.3.9600	13	5.78 %

Most from 10 users or less.

Only 12 thunderbird crashes in 6 months.

Comment 7

[Lee Salzman [:lsalzman]]

Jonathan, is this something we could make fallible with SEH as well?

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1485425 - Protect more DWrite API usage in gfxDWriteFontList::ReadFaceNamesForFamily with MOZ_SEH_TRY/EXCEPT. r=lsalzman

The majority of the crashes I looked at here are happening under ReadFaceNamesForFamily.
We already have an SEH guard around part of the method, but it doesn't cover all the potentially
problematic DWrite calls, so this patch moves more of the code inside the try/except block
to attempt to handle exceptions safely.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1485425 - Add MOZ_SEH_TRY/EXCEPT guards around more DWrite API calls that might throw exceptions. r=lsalzman

A few of the crashes here are happening via other codepaths, so we should protect those as well.

None of this changes any behavior except in the case where a disk error or similar failure happens
within DWrite calls; the goal here is to handle such failures safely, just treating the affected
font as unusable and allowing us to fall back to something else rather than crash.

Depends on D165524

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/7c3227d88e4c
Protect more DWrite API usage in gfxDWriteFontList::ReadFaceNamesForFamily with MOZ_SEH_TRY/EXCEPT. r=lsalzman
https://hg.mozilla.org/mozilla-central/rev/718e2972c621
Add MOZ_SEH_TRY/EXCEPT guards around more DWrite API calls that might throw exceptions. r=lsalzman

Comment 11

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/7c3227d88e4c
https://hg.mozilla.org/mozilla-central/rev/718e2972c621

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Doesn't look like we'll see much in the way of impact until this patch rides to Beta. Is this something you wanted to try uplifting to Beta for 109 still?

Comment 13

[Jonathan Kew [:jfkthame]]

We might as well try, I suppose, given that nothing untoward seems to have happened on Nightly.

Comment 14

[Jonathan Kew [:jfkthame]]

Comment on attachment 9309834 [details]
Bug 1485425 - Protect more DWrite API usage in gfxDWriteFontList::ReadFaceNamesForFamily with MOZ_SEH_TRY/EXCEPT. r=lsalzman

Beta/Release Uplift Approval Request

• User impact if declined: Crashes due to exceptions inside DirectWrite in case of disk read errors or similar failures
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: (No way to test manually, unless we can fake some flaky hardware!)
• 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 some DWrite calls, to avoid immediately crashing; should have no impact unless such an exception occurs.
• String changes made/needed:
• Is Android affected?: No

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9309834 [details]
Bug 1485425 - Protect more DWrite API usage in gfxDWriteFontList::ReadFaceNamesForFamily with MOZ_SEH_TRY/EXCEPT. r=lsalzman

Approved for 109.0b8.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3e48327f8136
https://hg.mozilla.org/releases/mozilla-beta/rev/96e5e9d0b055

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Crash volume on ESR is pretty low so I don't think this is a priority to uplift there, but that's also not a strongly-held opinion of mine if you wanted to nominate it.

Comment 18

[Wayne Mery (:wsmwk)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #17)

Crash volume on ESR is pretty low so I don't think this is a priority to uplift there, but that's also not a strongly-held opinion of mine if you wanted to nominate it.

Didn't find any additional crash signatures with signficant numbers. So based on stats, I agree.