1192699 - merge gfxDWriteFontList::DelayedInitFontList into gfxDWriteFontList::InitFontList

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[John Daggett (:jtd)]

Under DirectWrite we initialize the system fontlist in two stages, gfxDWriteFontList::InitFontList() and then gfxDWriteFontList::DelayedInitFontList. Whenever the system fontlist is first used, the code lazily attempts to lazily build the fontlist by calling DelayedInitFontList. This was an attempt to deal with startup issues but the root cause was a bug in DirectWrite, so this two-stage initialization doesn't really achieve anything. To do *anything* at startup, we need a font, which means we need this list initialized.

I think we've got some codepath such that GetDefaultFont may be called after InitFontList has been called but before DelayedInitFontList is called. We could analyze this a bit more but I think the best thing would be to move the code in DelayedInitFontList into InitFontList to avoid any possible chance of the system fontlist being empty. 

Details on the startup problem:

When DirectWrite support was added in gfx, we ran into a problem with long delays at startup. After a lot of fiddling around, it turns out that at startup if the DirectWrite code doesn't receive a response from the system font cache process, it manually iterates over all fonts (!!!). Any call to GetSystemFontCollection() is capable of inducing this delay. Microsoft has fixed this in later versions of DirectWrite but the telemetry data indicates that we still hit this sometimes.

Comment 1

[John Daggett (:jtd)]

Attachment: patch, eliminate two-stage fontlist initialization for DirectWrite backend

Merge the two initialization phases together and trim out unneeded code. Add some extra Windows-specific info to the no fonts abort crashstack annotations.

Comment 2

[John Daggett (:jtd)]

Comment on attachment 8646178 [details] [diff] [review]
patch, eliminate two-stage fontlist initialization for DirectWrite backend

Guessing Bas is on break, switching review to Kato-san

Comment 3

[Makoto Kato [:m_kato]]

Comment on attachment 8646178 [details] [diff] [review]
patch, eliminate two-stage fontlist initialization for DirectWrite backend

Review of attachment 8646178 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxTextRun.cpp
@@ +1918,5 @@
> +#ifdef XP_WIN
> +        bool dwriteEnabled = gfxWindowsPlatform::GetPlatform()->DWriteEnabled();
> +        double upTime = (double) GetTickCount();
> +        fontInitInfo.AppendPrintf(" backend: %s system-uptime: %9.3f sec",
> +                                  dwriteEnabled ? "directwrite" : "gdi", upTime);

upTime is ms, so use upTime / 1000

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/830ebfb44928

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/830ebfb44928

Comment 6

[John Daggett (:jtd)]

Comment on attachment 8646178 [details] [diff] [review]
patch, eliminate two-stage fontlist initialization for DirectWrite backend

Approval Request Comment
[Feature/regressing bug #]: 1173579
[User impact if declined]: periodic crashes at startup under Windows 7
[Describe test coverage new/current, TreeHerder]: landed last week on central
[Risks and why]: This appears to have eliminated most of the remaining GetDefaultFont aborts at startup. It only affects the DirectWrite fontlist and is essentially a merge of the previous two-stage system fontlist initialization that was used.
[String/UUID change made/needed]: none

Comment 7

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

Comment on attachment 8646178 [details] [diff] [review]
patch, eliminate two-stage fontlist initialization for DirectWrite backend

Let's uplift to Aurora as this is a startup crash fix. After we have stabilized this patch for a few days we can uplift to Beta as well.

Comment 8

[John Daggett (:jtd)]

pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/fbc314dea701

Comment 9

[John Daggett (:jtd)]

(In reply to John Daggett (:jtd) from comment #8)
> pushed to aurora:
> https://hg.mozilla.org/releases/mozilla-aurora/rev/fbc314dea701

This translates to builds including and after 2015-08-19, so hopefully nothing will show up in 42.0a2 crashreports after the 20150818 build.

https://crash-stats.mozilla.com/search/?product=Firefox&version=42.0a2&signature=~GetDefaultFont&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature

Comment 10

[John Daggett (:jtd)]

(In reply to Ritu Kothari (:ritu) from comment #7)
> Comment on attachment 8646178 [details] [diff] [review]
> patch, eliminate two-stage fontlist initialization for DirectWrite backend
> 
> Let's uplift to Aurora as this is a startup crash fix. After we have
> stabilized this patch for a few days we can uplift to Beta as well.

approval-ping?

Comment 11

[John Daggett (:jtd)]

No crash reports for GetDefaultFont since this was landed on aurora:

https://crash-stats.mozilla.com/signature/?product=Firefox&version=42.0a2&signature=mozalloc_abort%28char+const*+const%29+|+NS_DebugBreak+|+gfxFontGroup%3A%3AGetDefaultFont%28%29&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&page=1#reports

Comment 12

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

Comment on attachment 8646178 [details] [diff] [review]
patch, eliminate two-stage fontlist initialization for DirectWrite backend

This patch has stabilized in Aurora and crash-stats page does not show any crashes on 42 since 0819 build. Let's uplift to Beta41.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for beta uplift.

Comment 14

[John Daggett (:jtd)]

Pushed adjusted patch to beta.
https://hg.mozilla.org/releases/mozilla-beta/rev/df5e0e3f43ed

Comment 15

[Kent James (:rkent)]

The crash signature from comment 11 is the #1 crash stack for Thunderbird OSX 38.3.0. The changes apply with minor edits to m-esr38 so I am considering taking this in THUNDERBIRD_38_VERBRANCH for Thunderbird 38.4.0

Comment 16

[Kent James (:rkent)]

Comment on attachment 8646178 [details] [diff] [review]
patch, eliminate two-stage fontlist initialization for DirectWrite backend

Pushed to THUNDERBIRD_38_VERBRANCH https://hg.mozilla.org/releases/mozilla-esr38/rev/cedb6fecc222 a=rkent

Comment 17

[Wayne Mery (:wsmwk)]

(In reply to Kent James (:rkent) from comment #15)
> The crash signature from comment 11 is the #1 crash stack for Thunderbird
> OSX 38.3.0. 
Indeed, it is 4% of Mac crashes.

> The changes apply with minor edits to m-esr38 so I am
> considering taking this in THUNDERBIRD_38_VERBRANCH for Thunderbird 38.4.0

THanks. Noting we'll want to check crash-status a week or two after release.

Comment 18

[Jonathan Kew [:jfkthame]]

(In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #17)
> (In reply to Kent James (:rkent) from comment #15)
> > The crash signature from comment 11 is the #1 crash stack for Thunderbird
> > OSX 38.3.0. 
> Indeed, it is 4% of Mac crashes.

Note that *this* bug is not relevant to Mac crashes; the patch here is strictly Windows-only. Mac crashes with a similar stack must be a separate bug.

Comment 19

[Kent James (:rkent)]

(In reply to Jonathan Kew (:jfkthame) from comment #18)

> Note that *this* bug is not relevant to Mac crashes; the patch here is
> strictly Windows-only. Mac crashes with a similar stack must be a separate
> bug.

Any suggestion of which bug? The issue seems to be worse in Thunderbird than Firefox, so we'll be more agressive about taking a patch in our branch.

Comment 20

[Jonathan Kew [:jfkthame]]

(In reply to Kent James (:rkent) from comment #19)
> (In reply to Jonathan Kew (:jfkthame) from comment #18)
> 
> > Note that *this* bug is not relevant to Mac crashes; the patch here is
> > strictly Windows-only. Mac crashes with a similar stack must be a separate
> > bug.
> 
> Any suggestion of which bug? The issue seems to be worse in Thunderbird than
> Firefox, so we'll be more agressive about taking a patch in our branch.

Sorry, I'm not sure offhand. There's bug 1173579, which sounds similar; that was originally filed as a Windows bug, but comments also talk about Mac crashes. I'm not clear on the current status of that.

And there was bug 1070983, which was closed after bug 1189158 landed. That was landed for mozilla-42, and uplifted to 41; so if you're dealing with a mozilla-38-based branch, perhaps you want to uplift bug 1189158 there too?

But I haven't been following recent font-list work very closely; maybe jdaggett will recognize this and be able to offer further suggestions.

Comment 21

[John Daggett (:jtd)]

Kent, if you point me at a URL for Thunderbird gfx crashers I can take a look. I worked on fixing a number of crasher bugs during Q3:

1100949 structured expection handling
1197650 duplicate null-check crasher
1192699 eliminate two-stage fontlist init under DirectWrite
1189158 shutdown font loader thread in separate event
1189129 annotate no default font aborts

I think this one and 1189158 were the most common crashers.

Comment 22

[Kent James (:rkent)]

John, one helpful thing you could do would be to look at the patch in bug 1189158 and see if it could be uplifted to mozilla-esr38 (THUNDERBIRD_38_VERBRANCH). That looked like it might be an important patch, but the changes were too extensive for me to be comfortable adapting it with my limited knowledge of the graphics system. I can do the adaptation if you think it is safe.

Otherwise, perhaps Wayne could better suggest important TB graphics crashers.

Comment 23

[John Daggett (:jtd)]

(In reply to Kent James (:rkent) from comment #22)
> John, one helpful thing you could do would be to look at the patch in bug
> 1189158 and see if it could be uplifted to mozilla-esr38
> (THUNDERBIRD_38_VERBRANCH). That looked like it might be an important patch,
> but the changes were too extensive for me to be comfortable adapting it with
> my limited knowledge of the graphics system. I can do the adaptation if you
> think it is safe.
> 
> Otherwise, perhaps Wayne could better suggest important TB graphics crashers.

Yes, you *definitely* want to take the fix for 1189158!

Comment 24

[Wayne Mery (:wsmwk)]

(In reply to Kent James (:rkent) from comment #22)
> John, one helpful thing you could do would be to look at the patch in bug
> 1189158 and see if it could be uplifted to mozilla-esr38
> (THUNDERBIRD_38_VERBRANCH). That looked like it might be an important patch,
> but the changes were too extensive for me to be comfortable adapting it with
> my limited knowledge of the graphics system. I can do the adaptation if you
> think it is safe.
> 
> Otherwise, perhaps Wayne could better suggest important TB graphics crashers.
> Yes, you *definitely* want to take the fix for 1189158!

If it will help fix our #13 crash bug 1173579, I agree 

* #8 crash, which I filed a couple days ago, is bug 1222487. It has no fix
* #48 also has no fix yet https://crash-stats.mozilla.com/report/list?product=Thunderbird&range_value=7&range_unit=days&date=2015-11-09&signature=mozilla%3A%3Alayers%3A%3AContentClientDoubleBuffered%3A%3AFinalizeFrame&version=Thunderbird%3A38.3.0 has two new bug reports Bug 1216909 - BD24 AVR:NULL firefox.exe!xul.dll!mozilla::layers::ContentClientDoubleBuffered::FinalizeFrame and Bug 1200021 - mozilla::layers::ContentClientDoubleBuffered::FinalizeFrame 

We disabled HWA in version 38.2.0 and since then we have almost no graphics crashes.  However, development and beta branches have HWA enabled.  But branch crash rates are very low and they do not mirror production releases, and therefore we pretty much do not have useful data from there.  

FWIW current beta has ~3 nvwg crashes https://crash-stats.mozilla.com/topcrasher/products/Thunderbird/versions/42.0b2/date_range_type/report/crash_type/browser/os_name/None/result_count/100?days=14

If we decide to re-enable HWA for thunderbird 45 we will need to be extremely agressive against graphics issues - version 38.1.0 held 3 of the top 6 ranks including #1, and about 5 more in the top 50. https://crash-stats.mozilla.com/search/?product=Thunderbird&version=38.1.0&date=%3E2015-08-15&date=%3C2015-09-01&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature