1257124 - crash in gfxDWriteFontFileLoader::CreateCustomFontFile

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[[:philipp]]

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.

Comment 1

[Bob Owen (:bobowen)]

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?

Comment 2

[Bas Schouten (:bas.schouten)]

That certainly could happen in some cases when getting an invalid webfont, yes.

Comment 3

[Bob Owen (:bobowen)]

Attachment: 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.

Comment 4

[Bas Schouten (:bas.schouten)]

(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 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85e76ff91cb9

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/85e76ff91cb9

Comment 7

[Bob Owen (:bobowen)]

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

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dadf228376c8

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Firefox Bug Husbandry Bot]

Moving from Core::Untriaged to Core::General https://bugzilla.mozilla.org/show_bug.cgi?id=1407598