Closed Bug 1257124 Opened 4 years ago Closed 4 years ago

crash in gfxDWriteFontFileLoader::CreateCustomFontFile

Categories

(Core :: General, defect, critical)

46 Branch
x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox45 --- unaffected
firefox46 --- fixed
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: philipp, Assigned: bobowen)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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.
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)
(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: 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?
https://hg.mozilla.org/mozilla-central/rev/85e76ff91cb9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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 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+
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.