crash in gfxDWriteFontFileLoader::CreateCustomFontFile

RESOLVED FIXED in Firefox 46

Status

()

--
critical
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: philipp, Assigned: bobowen)

Tracking

({crash, regression})

46 Branch
mozilla48
x86
Windows NT
crash, regression
Points:
---

Firefox Tracking Flags

(firefox45 unaffected, firefox46 fixed, firefox47 fixed, firefox48 fixed)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
This bug was filed from the Socorro interface and is 
report bp-0715bf31-5d41-4701-8f13-68e272160315.
=============================================================

Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	gfxDWriteFontFileLoader::CreateCustomFontFile(FallibleTArray<unsigned char>&, IDWriteFontFile**, IDWriteFontFileStream**) 	gfx/thebes/gfxDWriteCommon.cpp
1 	xul.dll 	gfxDWriteFontList::MakePlatformFont(nsAString_internal const&, unsigned short, short, unsigned char, unsigned char const*, unsigned int) 	gfx/thebes/gfxDWriteFontList.cpp
2 	xul.dll 	gfxUserFontEntry::LoadPlatformFont(unsigned char const*, unsigned int&) 	gfx/thebes/gfxUserFontSet.cpp

this crash signature is new in firefox 46 builds and seems to have been introduced by the work done in bug 1156742.
(Assignee)

Comment 1

3 years ago
I suspect this is a morphing of the previous crash:
https://crash-stats.mozilla.com/signature/?signature=gfxDWriteFontList%3A%3AMakePlatformFont
This has disappeared in 46.

So, my guess is that gfxWindowsPlatform::GetPlatform()->GetDWriteFactory() is returning null and the same thing was causing the crash before here:
http://hg.mozilla.org/releases/mozilla-release/annotate/b6609650a911/gfx/thebes/gfxDWriteFontList.cpp#l822

Bas - does that make sense?
Do we just null check and return E_FAIL or something?
Flags: needinfo?(bas)
That certainly could happen in some cases when getting an invalid webfont, yes.
Flags: needinfo?(bas)
(Assignee)

Comment 3

3 years ago
Created attachment 8731788 [details] [diff] [review]
Add null check for IDWriteFactory in gfxDWriteFontFileLoader::CreateCustomFontFile

(In reply to Bas Schouten (:bas.schouten) from comment #2)
> That certainly could happen in some cases when getting an invalid webfont,
> yes.

Sorry Bas, I don't quite understand.
How would an invalid webfont, cause gfxWindowsPlatform::GetPlatform()->GetDWriteFactory() to return null?

Also looks like there are other crashes caused by this return null, like in bug 1244512.

Either way here's the null check.
Attachment #8731788 - Flags: review?(bas)
(Assignee)

Updated

3 years ago
Assignee: nobody → bobowen.code
Status: NEW → ASSIGNED
Attachment #8731788 - Flags: review?(bas) → review+
(In reply to Bob Owen (:bobowen) from comment #3)
> Created attachment 8731788 [details] [diff] [review]
> Add null check for IDWriteFactory in
> gfxDWriteFontFileLoader::CreateCustomFontFile
> 
> (In reply to Bas Schouten (:bas.schouten) from comment #2)
> > That certainly could happen in some cases when getting an invalid webfont,
> > yes.
> 
> Sorry Bas, I don't quite understand.
> How would an invalid webfont, cause
> gfxWindowsPlatform::GetPlatform()->GetDWriteFactory() to return null?
> 
> Also looks like there are other crashes caused by this return null, like in
> bug 1244512.
> 
> Either way here's the null check.

Sorry, I misunderstood your comment. The crash logs indicate a device reset happened on those machines. But I don't really see how that could lead to a nullptr DWriteFactory... That's rather strange really, but let's take this patch non-the-less, it's almost certain to somehow be the cause. Could you add a gfxCriticalError to that line?

Comment 6

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/85e76ff91cb9
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Assignee)

Comment 7

3 years ago
Comment on attachment 8731788 [details] [diff] [review]
Add null check for IDWriteFactory in gfxDWriteFontFileLoader::CreateCustomFontFile

Note: the patch that landed is slightly different.

Approval Request Comment
[Feature/regressing bug #]:
This was probably an existing crash that was moved by bug 1156742.

[User impact if declined]:
Crash.

[Describe test coverage new/current, TreeHerder]:
This would be covered by existing Web Font tests.

[Risks and why]:
Low - simple null check.

[String/UUID change made/needed]:
None
Attachment #8731788 - Flags: approval-mozilla-beta?
Attachment #8731788 - Flags: approval-mozilla-aurora?

Comment 8

3 years ago
Comment on attachment 8731788 [details] [diff] [review]
Add null check for IDWriteFactory in gfxDWriteFontFileLoader::CreateCustomFontFile

Even though I did not see this crash on 47, the change seems simple enough to uplift to both Aurora47, Beta46.
Attachment #8731788 - Flags: approval-mozilla-beta?
Attachment #8731788 - Flags: approval-mozilla-beta+
Attachment #8731788 - Flags: approval-mozilla-aurora?
Attachment #8731788 - Flags: approval-mozilla-aurora+

Comment 9

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/dadf228376c8
status-firefox47: affected → fixed

Comment 10

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/bdfc2038590c
status-firefox46: affected → fixed
Moving from Core::Untriaged to Core::General https://bugzilla.mozilla.org/show_bug.cgi?id=1407598
Component: Untriaged → General
You need to log in before you can comment on or make changes to this bug.