merge gfxDWriteFontList::DelayedInitFontList into gfxDWriteFontList::InitFontList

RESOLVED FIXED in Firefox 41

Status

()

Core
Graphics: Text
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jtd, Assigned: jtd)

Tracking

Trunk
mozilla43
Unspecified
Windows
Points:
---

Firefox Tracking Flags

(firefox41 fixed, firefox42 fixed, firefox43 fixed, thunderbird_esr3842+ fixed)

Details

(Whiteboard: [tbird topcrash Mac])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8646178 [details] [diff] [review]
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.
Attachment #8646178 - Flags: review?(bas)
(Assignee)

Comment 2

2 years ago
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
Attachment #8646178 - Flags: review?(bas) → review?(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
Attachment #8646178 - Flags: review?(m_kato) → review+

Comment 4

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/830ebfb44928
https://hg.mozilla.org/mozilla-central/rev/830ebfb44928
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
(Assignee)

Comment 6

2 years ago
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
Attachment #8646178 - Flags: approval-mozilla-beta?
Attachment #8646178 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

2 years ago
status-firefox41: --- → affected

Comment 7

2 years ago
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.
Attachment #8646178 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 8

2 years ago
pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/fbc314dea701
(Assignee)

Updated

2 years ago
status-firefox42: affected → fixed
(Assignee)

Comment 9

2 years ago
(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
(Assignee)

Comment 10

2 years ago
(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?
Flags: needinfo?(rkothari)
(Assignee)

Comment 11

2 years ago
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 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.
Flags: needinfo?(rkothari)
Attachment #8646178 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Needs rebasing for beta uplift.
Flags: needinfo?(jdaggett)
(Assignee)

Comment 14

2 years ago
Pushed adjusted patch to beta.
https://hg.mozilla.org/releases/mozilla-beta/rev/df5e0e3f43ed
Flags: needinfo?(jdaggett)
(Assignee)

Updated

2 years ago
status-firefox41: affected → fixed

Comment 15

2 years ago
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

Updated

2 years ago
status-thunderbird_esr38: --- → affected
tracking-thunderbird_esr38: --- → +

Comment 16

2 years ago
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

Updated

2 years ago
status-thunderbird_esr38: affected → fixed
tracking-thunderbird_esr38: + → 42+

Comment 17

2 years ago
(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.
Whiteboard: [tbird topcrash Mac]
(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

2 years ago
(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.
(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.
Flags: needinfo?(jdaggett)
(Assignee)

Comment 21

2 years ago
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.
Flags: needinfo?(jdaggett)

Comment 22

2 years ago
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.
Flags: needinfo?(vseerror)
Flags: needinfo?(jdaggett)
(Assignee)

Comment 23

2 years ago
(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!
Flags: needinfo?(jdaggett)

Comment 24

2 years ago
(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
Flags: needinfo?(vseerror)
You need to log in before you can comment on or make changes to this bug.