1369302 - [SkiaPDF] Crash in DWriteFontTypeface::onGetAdvancedTypefaceMetrics(), fDWriteFontFamily is nullptr

Status: RESOLVED FIXED

(Core :: Printing: Output, defect, P1)

Description

[Farmer Tseng[:fatseng](inactive)]

I'm enabling SkiaPDF for printing in Windows platform.
Since fDWriteFontFamily is nullptr, I hit crash while executing
hr = fDWriteFontFamily->GetFamilyNames(&familyNames);

Comment 1

[Farmer Tseng[:fatseng](inactive)]

Attachment: enable skiapdf

Comment 2

[Farmer Tseng[:fatseng](inactive)]

STR(Windows only):
1. apply attachment 8873360 [details] [diff] [review] 
2. add "ac_add_options --enable-skia-pdf" to mozconfig
3. build and launch firefox
4. print webpage

Actual Results:
Hit crash.

Comment 3

[Farmer Tseng[:fatseng](inactive)]

Hi Mason,
I am enabling SkiaPDF in Windows.
Unfortunately, I hit crash while generating PDF.
It crash in DWriteFontTypeface::onGetAdvancedTypefaceMetrics() because fDWriteFontFamily is nullptr.
Do you know the way to get IDWriteFontFamily?
Or if any way to get IDwriteFont?

Comment 4

[Astley Chen (inactive)]

jwatt, it seems the skia pdf rendering is broken on Windows, do you have any suggestion for the fix?

Comment 5

[Mason Chang [Inactive] [:mchang]]

I don't think skia pdf rendering was ever enabled possibly?

As for DWrite fonts crashing, we probably aren't creating the scaled font for dwrite correctly [1]. We added support for dwrite fonts in Firefox and Skia in bug 842894. We probably aren't creating the correct skia font somewhere.

[1] http://searchfox.org/mozilla-central/source/gfx/thebes/gfxDWriteFonts.cpp#690

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to Mason Chang [:mchang] from comment #5)
> I don't think skia pdf rendering was ever enabled possibly?

Skia PDF is built on all platforms for Nightly:

https://dxr.mozilla.org/mozilla-central/rev/5801aa478de12a62b2b2982659e787fcc4268d67/toolkit/moz.configure#999

However, only the Mac widget code (nsDeviceContextSpecX) currently has any MOZ_ENABLE_SKIA_PDF guarded code to use Skia PDF. Farmer's attached patch is a start at making the Windows widget code (nsDeviceContextSpecWin) use Skia PDF too.

Farmer's WIP patch isn't fully working yet and crashes in the way described in comment 0. He needs some guidance about what is missing or needs to be done differently in his patch. I think probably yourself (or maybe Lee or Bas?) would be more familiar with Windows and better able to take a look at the patch and help out than I am. Can you take a look?

Comment 7

[Mason Chang [Inactive] [:mchang]]

(In reply to Jonathan Watt [:jwatt] from comment #6)
> (In reply to Mason Chang [:mchang] from comment #5)
> > I don't think skia pdf rendering was ever enabled possibly?
> 
> Skia PDF is built on all platforms for Nightly:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 5801aa478de12a62b2b2982659e787fcc4268d67/toolkit/moz.configure#999
> 
> However, only the Mac widget code (nsDeviceContextSpecX) currently has any
> MOZ_ENABLE_SKIA_PDF guarded code to use Skia PDF. Farmer's attached patch is
> a start at making the Windows widget code (nsDeviceContextSpecWin) use Skia
> PDF too.
> 
> Farmer's WIP patch isn't fully working yet and crashes in the way described
> in comment 0. He needs some guidance about what is missing or needs to be
> done differently in his patch. I think probably yourself (or maybe Lee or
> Bas?) would be more familiar with Windows and better able to take a look at
> the patch and help out than I am. Can you take a look?

Can he see if we're actually creating the Skia font correctly from comment 5 please?

Comment 8

[Farmer Tseng[:fatseng](inactive)]

Hi Mason,
Per checking, we created the scaled font for dwrite correctly [1]. The root cause is that we pass fontfamily as nullptr to DWriteFontTypeface while calling SkCreateTypefaceFromDWriteFont [2][3]. Therefore, I hit crash when DWriteFontTypeface::onGetAdvancedTypefaceMetrics was called [4].

I tried many ways to get IDWriteFontFamily or IDwriteFont via IDWriteFontFace. Unfortunately,  the methods have failed.

[1] http://searchfox.org/mozilla-central/source/gfx/thebes/gfxDWriteFonts.cpp#690
[2]ScaledFontDWrite::GetSkTypeface()--->SkCreateTypefaceFromDWriteFont(...)--->DWriteFontTypeface::Create(aFactory, aFontFace, aStyle, aForceGDI)
[3] http://searchfox.org/mozilla-central/source/gfx/skia/skia/src/ports/SkTypeface_win_dw.h#81
[4] http://searchfox.org/mozilla-central/source/gfx/skia/skia/src/ports/SkTypeface_win_dw.cpp#360

Comment 9

[Mason Chang [Inactive] [:mchang]]

(In reply to Farmer Tseng[:fatseng] from comment #8)
> Hi Mason,
> Per checking, we created the scaled font for dwrite correctly [1]. The root
> cause is that we pass fontfamily as nullptr to DWriteFontTypeface while
> calling SkCreateTypefaceFromDWriteFont [2][3]. Therefore, I hit crash when
> DWriteFontTypeface::onGetAdvancedTypefaceMetrics was called [4].
> 
> I tried many ways to get IDWriteFontFamily or IDwriteFont via
> IDWriteFontFace. Unfortunately,  the methods have failed.
> 
> [1]
> http://searchfox.org/mozilla-central/source/gfx/thebes/gfxDWriteFonts.cpp#690
> [2]ScaledFontDWrite::GetSkTypeface()--->SkCreateTypefaceFromDWriteFont(...)--
> ->DWriteFontTypeface::Create(aFactory, aFontFace, aStyle, aForceGDI)
> [3]
> http://searchfox.org/mozilla-central/source/gfx/skia/skia/src/ports/
> SkTypeface_win_dw.h#81
> [4]
> http://searchfox.org/mozilla-central/source/gfx/skia/skia/src/ports/
> SkTypeface_win_dw.cpp#360

I dug through the history and debugger a bit. It seems like during normal Skia operation, we never call onGetAdvancedTypefaceMetrics and we never need the font family. IIRC, there isn't a way to get the font family from the IDWriteFont (I know, super confusing). There is some history in https://bugzilla.mozilla.org/show_bug.cgi?id=1309917.

Do you know why this method is being called? Also, IIRC some of the problems came from sometimes the gfxDWriteFont was being created by Cairo then used by Skia, which shouldn't be happening. Are we ever creating fonts via Cairo here?

Comment 10

[Farmer Tseng[:fatseng](inactive)]

(In reply to Mason Chang [:mchang] from comment #9)
> 
> I dug through the history and debugger a bit. It seems like during normal
> Skia operation, we never call onGetAdvancedTypefaceMetrics and we never need
> the font family. IIRC, there isn't a way to get the font family from the
> IDWriteFont (I know, super confusing). There is some history in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1309917.
> 
> Do you know why this method is being called? 
It looks like SkiaPDF wants to store some information in the file, like font name [1].
I ever add a null check for fDWriteFontFamily, it still could generate a PDF. Maybe it is a temporary solution.

> Also, IIRC some of the problems
> came from sometimes the gfxDWriteFont was being created by Cairo then used
> by Skia, which shouldn't be happening. Are we ever creating fonts via Cairo
> here?
No, my patch doesn't modify font creating.

[1] http://searchfox.org/mozilla-central/source/gfx/skia/skia/src/ports/SkTypeface_win_dw.cpp#368

Comment 11

[Farmer Tseng[:fatseng](inactive)]

Attachment: Bug 1369302 - Set the font name to "Unknown" while fDWriteFontFamily is null.

Review commit: https://reviewboard.mozilla.org/r/148348/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148348/

Comment 12

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8877009 [details]
Bug 1369302 - Set the font name to "Unknown" while fDWriteFontFamily is null.

I add a null check to avoid the crash while generating SkiaPDF.

Comment 13

[Mason Chang [Inactive] [:mchang]]

(In reply to Farmer Tseng[:fatseng] from comment #10)
> (In reply to Mason Chang [:mchang] from comment #9)
> > 
> > I dug through the history and debugger a bit. It seems like during normal
> > Skia operation, we never call onGetAdvancedTypefaceMetrics and we never need
> > the font family. IIRC, there isn't a way to get the font family from the
> > IDWriteFont (I know, super confusing). There is some history in
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1309917.
> > 
> > Do you know why this method is being called? 
> It looks like SkiaPDF wants to store some information in the file, like font
> name [1].
> I ever add a null check for fDWriteFontFamily, it still could generate a
> PDF. Maybe it is a temporary solution.
> 

Why is this method being called but it isn't during normal text rendering?

> > Also, IIRC some of the problems
> > came from sometimes the gfxDWriteFont was being created by Cairo then used
> > by Skia, which shouldn't be happening. Are we ever creating fonts via Cairo
> > here?
> No, my patch doesn't modify font creating.

Sorry, when I was implementing dwrite fonts in Skia, gecko would be creating cairo fonts sometimes and skia fonts sometimes and we'd mix them. Is that happening in the pdf case? You don't have to change font creation, but this could already be happening.

Comment 14

[Farmer Tseng[:fatseng](inactive)]

(In reply to Mason Chang [:mchang] from comment #13)
> (In reply to Farmer Tseng[:fatseng] from comment #10)
> > (In reply to Mason Chang [:mchang] from comment #9)
> > > 
> > > I dug through the history and debugger a bit. It seems like during normal
> > > Skia operation, we never call onGetAdvancedTypefaceMetrics and we never need
> > > the font family. IIRC, there isn't a way to get the font family from the
> > > IDWriteFont (I know, super confusing). There is some history in
> > > https://bugzilla.mozilla.org/show_bug.cgi?id=1309917.
> > > 
> > > Do you know why this method is being called? 
> > It looks like SkiaPDF wants to store some information in the file, like font
> > name [1].
> > I ever add a null check for fDWriteFontFamily, it still could generate a
> > PDF. Maybe it is a temporary solution.
> > 
> 
> Why is this method being called but it isn't during normal text rendering?

I think that draw target is a difference.  
After applying my patch, texts are rendered to Skia PDF while issuing printing.
And it crashed on printing nightly start page.
You mentioned "normal text rendering."  I assume you are talking about show text on a screen.
I am not familiar with gfx. I don't know how texts render on a screen.
However, I list down the call flow while this method is being called.
I am not sure if I answer your question correctly.

Call flow:
DrawTargetSkia::DrawGlyphs[1]-->SkCanvas::onDrawPosText[2]-->SkPDFDevice::drawPosText[3]-->SkPDFDevice::internalDrawText[4]-->SkPDFFont::GetMetrics[5]-->DWriteFontTypeface::onGetAdvancedTypefaceMetrics[6]

> 
> > > Also, IIRC some of the problems
> > > came from sometimes the gfxDWriteFont was being created by Cairo then used
> > > by Skia, which shouldn't be happening. Are we ever creating fonts via Cairo
> > > here?
> > No, my patch doesn't modify font creating.
> 
> Sorry, when I was implementing dwrite fonts in Skia, gecko would be creating
> cairo fonts sometimes and skia fonts sometimes and we'd mix them. Is that
> happening in the pdf case? You don't have to change font creation, but this
> could already be happening.

It isn't happening in the PDF case.
I tried to print the nightly start page and set a break point on creating the scaled font[7].
The code never is reached while printing.


[1] http://searchfox.org/mozilla-central/source/gfx/2d/DrawTargetSkia.cpp#1457
[2] http://searchfox.org/mozilla-central/source/gfx/skia/skia/src/core/SkCanvas.cpp#2556
[3] http://searchfox.org/mozilla-central/source/gfx/skia/skia/src/pdf/SkPDFDevice.cpp#1436
[4] http://searchfox.org/mozilla-central/source/gfx/skia/skia/src/pdf/SkPDFDevice.cpp#1272
[5] http://searchfox.org/mozilla-central/source/gfx/skia/skia/src/pdf/SkPDFFont.cpp#155
[6] http://searchfox.org/mozilla-central/source/gfx/skia/skia/src/ports/SkTypeface_win_dw.cpp#336

[7] http://searchfox.org/mozilla-central/source/gfx/thebes/gfxDWriteFonts.cpp#686

Comment 15

[Farmer Tseng[:fatseng](inactive)]

As IRC with mchang, I will check following things:
1. Check when we just DrawGlyphs not in a pdf - http://searchfox.org/mozilla-central/source/gfx/2d/DrawTargetSkia.cpp#146 and see where the divergence happens
2. Check Skia PDF from mac to see if it always has the font name

Comment 16

[Farmer Tseng[:fatseng](inactive)]

(In reply to Farmer Tseng[:fatseng] from comment #15)
> As IRC with mchang, I will check following things:
> 1. Check when we just DrawGlyphs not in a pdf -
> http://searchfox.org/mozilla-central/source/gfx/2d/DrawTargetSkia.cpp#146
> and see where the divergence happens
There is a divergence from Skia device.

[Skia PDF]
DrawTargetSkia::DrawGlyphs[1]
|->SkCanvas::onDrawPosText[2]
   |->SkPDFDevice::drawPosText[3]
      |->SkPDFDevice::internalDrawText[4]
         |->SkPDFFont::GetMetrics[5]
            |->DWriteFontTypeface::onGetAdvancedTypefaceMetrics[6]

[Normal text rendering]
DrawTargetSkia::DrawGlyphs[1]
|->SkCanvas::onDrawPosText[2]
   |->SkBitmapDevice::drawPosText[13]
      |->SkDraw::drawPosText[14]

[1] http://searchfox.org/mozilla-central/source/gfx/2d/DrawTargetSkia.cpp#1457
[2] http://searchfox.org/mozilla-central/source/gfx/skia/skia/src/core/SkCanvas.cpp#2556
[3] http://searchfox.org/mozilla-central/source/gfx/skia/skia/src/pdf/SkPDFDevice.cpp#1436
[4] http://searchfox.org/mozilla-central/source/gfx/skia/skia/src/pdf/SkPDFDevice.cpp#1272
[5] http://searchfox.org/mozilla-central/source/gfx/skia/skia/src/pdf/SkPDFFont.cpp#155
[6] http://searchfox.org/mozilla-central/source/gfx/skia/skia/src/ports/SkTypeface_win_dw.cpp#336
 
[13] http://searchfox.org/mozilla-central/source/gfx/skia/skia/src/core/SkBitmapDevice.cpp#373
[14] http://searchfox.org/mozilla-central/source/gfx/skia/skia/src/core/SkDraw.cpp#1644

Comment 17

[Farmer Tseng[:fatseng](inactive)]

Attachment: Skia PDF font name (from mac)

Comment 18

[Farmer Tseng[:fatseng](inactive)]

(In reply to Farmer Tseng[:fatseng] from comment #15)
> 2. Check Skia PDF from mac to see if it always has the font name
Per check, the Skia PDF from mac always has the font name.
I set breakpoint on SkFontHost_mac.cpp[1]. I found that it could get the font name.
We also could see the font name from PDF file, see attachment 8879053 [details]

[1] http://searchfox.org/mozilla-central/source/gfx/skia/skia/src/ports/SkFontHost_mac.cpp#1544

Comment 19

[Mason Chang [Inactive] [:mchang]]

(In reply to Farmer Tseng[:fatseng] from comment #18)
> (In reply to Farmer Tseng[:fatseng] from comment #15)
> > 2. Check Skia PDF from mac to see if it always has the font name
> Per check, the Skia PDF from mac always has the font name.
> I set breakpoint on SkFontHost_mac.cpp[1]. I found that it could get the
> font name.
> We also could see the font name from PDF file, see attachment 8879053 [details]
> [details]
> 
> [1]
> http://searchfox.org/mozilla-central/source/gfx/skia/skia/src/ports/
> SkFontHost_mac.cpp#1544

If this is the only use for the font names, then I guess we can bypass this check, or set the font name to "Unknown". Did you verify that this is the only use for the font names?

Comment 20

[Farmer Tseng[:fatseng](inactive)]

Attachment: SkiaPDF Font Name(Windows)

Comment 21

[Farmer Tseng[:fatseng](inactive)]

(In reply to Mason Chang [:mchang] from comment #19)
> (In reply to Farmer Tseng[:fatseng] from comment #18)
> > (In reply to Farmer Tseng[:fatseng] from comment #15)
> > > 2. Check Skia PDF from mac to see if it always has the font name
> > Per check, the Skia PDF from mac always has the font name.
> > I set breakpoint on SkFontHost_mac.cpp[1]. I found that it could get the
> > font name.
> > We also could see the font name from PDF file, see attachment 8879053 [details]
> > [details]
> > 
> > [1]
> > http://searchfox.org/mozilla-central/source/gfx/skia/skia/src/ports/
> > SkFontHost_mac.cpp#1544
> 
> If this is the only use for the font names, then I guess we can bypass this
> check, or set the font name to "Unknown". Did you verify that this is the
> only use for the font names?

I already verified that this is the only use for storing the font name in PDF file.
I set the font name to "Unknow". It was no harm to generate PDF file in Windows. Please see attachment 8879430 [details].
Even though I browsed the PDF in MAC, it still showed font correctly.

Comment 22

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8877009 [details]
Bug 1369302 - Set the font name to "Unknown" while fDWriteFontFamily is null.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148348/diff/1-2/

Comment 23

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8877009 [details]
Bug 1369302 - Set the font name to "Unknown" while fDWriteFontFamily is null.

https://reviewboard.mozilla.org/r/148348/#review155880

::: gfx/skia/skia/src/ports/SkTypeface_win_dw.cpp:371
(Diff revision 2)
> -    SkSMallocWCHAR familyName(familyNameLen+1);
> +      SkSMallocWCHAR familyName(familyNameLen+1);
> -    hr = familyNames->GetString(0, familyName.get(), familyNameLen+1);
> +      hr = familyNames->GetString(0, familyName.get(), familyNameLen+1);
>  
> -    hr = sk_wchar_to_skstring(familyName.get(), familyNameLen, &info->fFontName);
> +      hr = sk_wchar_to_skstring(familyName.get(), familyNameLen, &info->fFontName);
> +    } else {
> +      hr = sk_wchar_to_skstring(L"Unknown", -1, &info->fFontName);

Why -1 length here? Shouldn't it be strlen(Unknown)?

Comment 24

[Farmer Tseng[:fatseng](inactive)]

(In reply to Mason Chang [:mchang] from comment #23)
> Comment on attachment 8877009 [details]
> Bug 1369302 - Set the font name to "Unknown" while fDWriteFontFamily is null.
> 
> https://reviewboard.mozilla.org/r/148348/#review155880
> 
> ::: gfx/skia/skia/src/ports/SkTypeface_win_dw.cpp:371
> (Diff revision 2)
> > -    SkSMallocWCHAR familyName(familyNameLen+1);
> > +      SkSMallocWCHAR familyName(familyNameLen+1);
> > -    hr = familyNames->GetString(0, familyName.get(), familyNameLen+1);
> > +      hr = familyNames->GetString(0, familyName.get(), familyNameLen+1);
> >  
> > -    hr = sk_wchar_to_skstring(familyName.get(), familyNameLen, &info->fFontName);
> > +      hr = sk_wchar_to_skstring(familyName.get(), familyNameLen, &info->fFontName);
> > +    } else {
> > +      hr = sk_wchar_to_skstring(L"Unknown", -1, &info->fFontName);
> 
> Why -1 length here? Shouldn't it be strlen(Unknown)?

The name length would be passed to WideCharToMultiByte.
It could be -1 as the document says "If this parameter is -1, the function processes the entire input string, including the terminating null character. Therefore, the resulting character string has a terminating null character, and the length returned by the function includes this character."

https://msdn.microsoft.com/zh-tw/library/windows/desktop/dd374130(v=vs.85).aspx

I would write the comment on the patch.

Comment 25

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8877009 [details]
Bug 1369302 - Set the font name to "Unknown" while fDWriteFontFamily is null.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148348/diff/2-3/

Comment 26

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8877009 [details]
Bug 1369302 - Set the font name to "Unknown" while fDWriteFontFamily is null.

Hi Mason,
Would you please review my patch?

Comment 27

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8877009 [details]
Bug 1369302 - Set the font name to "Unknown" while fDWriteFontFamily is null.

https://reviewboard.mozilla.org/r/148348/#review156274

Comment 28

[u459114]

Comment on attachment 8877009 [details]
Bug 1369302 - Set the font name to "Unknown" while fDWriteFontFamily is null.

https://reviewboard.mozilla.org/r/148348/#review156556

::: gfx/skia/skia/src/ports/SkTypeface_win_dw.cpp:377
(Diff revision 3)
> +      // setting -1 in WideCharToMultiByte. The document says "If this
> +      // parameter is -1, the function processes the entire input string,
> +      // including the terminating null character. Therefore, the resulting
> +      // character string has a terminating null character, and the length
> +      // returned by the function includes this character."
> +      // https://msdn.microsoft.com/zh-tw/library/windows/desktop/dd374130(v=vs.85).aspx

as time goes by, the line you posted might be broken.

If you insist add a link into comment, please considerate this link:
https://msdn.microsoft.com/library/windows/desktop/dd319072(v=vs.85).aspx

Or, you can comment right behind the '-1'
hr = sk_wchar_to_skstring(L"Unknown", -1 /* == strlent(Unknown) + 1*/, 
                          &info->fFontName);

Comment 29

[Farmer Tseng[:fatseng](inactive)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f72063338cf2d0835909c33d51374dc4156c77a&selectedJob=109106762

Comment 30

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8877009 [details]
Bug 1369302 - Set the font name to "Unknown" while fDWriteFontFamily is null.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148348/diff/3-4/

Comment 31

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8877009 [details]
Bug 1369302 - Set the font name to "Unknown" while fDWriteFontFamily is null.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148348/diff/4-5/

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eaf5690af79d
Set the font name to "Unknown" while fDWriteFontFamily is null. r=mchang

Comment 33

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

https://hg.mozilla.org/mozilla-central/rev/eaf5690af79d