1675714 - Startup Crash in [@ gfxDWriteFontFamily::LocalizedName]

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

All crash reports are for late beta (83.0b7+) - depending on gfx.e10s.font-list.shared being false?
All crashes are hit in the first 5 minutes after startup.

Crash report: https://crash-stats.mozilla.org/report/index/c2e3d059-b7e3-4fb1-bfe4-453250201103

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 xul.dll gfxDWriteFontFamily::LocalizedName gfx/thebes/gfxDWriteFontList.cpp:289
1 xul.dll gfxPlatformFontList::GetFontList gfx/thebes/gfxPlatformFontList.cpp:845
2 xul.dll gfxPlatform::GetFontList gfx/thebes/gfxPlatform.cpp:1795
3 xul.dll EnumerateFontsTask::Run gfx/src/nsThebesFontEnumerator.cpp:119
4 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1197
5 xul.dll mozilla::ipc::MessagePumpForNonMainThreads::Run ipc/glue/MessagePump.cpp:332
6 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:327
7 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:309
8 xul.dll static nsThread::ThreadFunc xpcom/threads/nsThread.cpp:442
9 nss3.dll _PR_NativeRunThread nsprpub/pr/src/threads/combined/pruthr.c:399

Comment 1

[Jonathan Kew [:jfkthame]]

We're crashing with a null-deref at https://hg.mozilla.org/releases/mozilla-beta/file/7b3b0961c32b79f75fb3e5f8f3ad4d63817a2dde/gfx/thebes/gfxDWriteFontList.cpp#l289, where we call

OSPreferences::GetInstance()->GetSystemLocale(locale);

which suggests we must be getting a null pointer from OSPreferences::GetInstance().

Initially, this didn't make any sense to me, as GetInstance() looks at first glance like it should be infallible. But then I realized this is happening on a non-main thread (FontEnumThread), and in every case I looked at, the main thread is in the midst of XPCOM shutdown at the same time. And the OSPreferences instance has been registered with ClearOnShutdown.

So what I think must be happening is that the FontEnumThread's attempt to use OSPreferences is racing with XPCOM-shutdown's deletion of the instance, and sometimes it loses the race. We should null-check the OSPreferences instance before using it, and ensure we're holding a strong ref here for safety.

The other issue here is that even the OSPreferences::GetInstanceAddRefed() accessor could be risky, because it uses GetInstance() internally and if it's called off-main-thread during shutdown, it'd be possible for the instance to be destroyed immediately after we check its existence but before the accessor takes a new strong reference to it.

(I think it's unlikely this is actually a regression from bug 1661247, which didn't significantly modify this code for the gfx.e10s.font-list.shared=false case. My guess is that it was triggered by changes in how shutdown is handled, but didn't immediately show up because of the font-list pref which meant this code wasn't being exercised on Nightly/early Beta.)

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1675714 - Null-check the OSPreferences instance before potential off-main-thread usage. r=lsalzman

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1675714 - Improve reliability of OSPreferences instance getters. r=lsalzman

Depends on D96201

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d9fdd572c1e1
Null-check the OSPreferences instance before potential off-main-thread usage. r=lsalzman
https://hg.mozilla.org/integration/autoland/rev/f57ef99c9c04
Improve reliability of OSPreferences instance getters. r=lsalzman

Comment 5

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/d9fdd572c1e1
https://hg.mozilla.org/mozilla-central/rev/f57ef99c9c04

Comment 6

[Jonathan Kew [:jfkthame]]

Comment on attachment 9186262 [details]
Bug 1675714 - Null-check the OSPreferences instance before potential off-main-thread usage. r=lsalzman

Beta/Release Uplift Approval Request

• User impact if declined: Possible shutdown-time crash for Windows users
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: (Unclear how to reliably reproduce this, it looks like a race condition during shutdown)
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Trivial patch to null-check the OSPreferences service pointer. (Just uplifting the first patch should be sufficient to stop the crashes for now.)
• String changes made/needed:

Comment 7

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9186262 [details]
Bug 1675714 - Null-check the OSPreferences instance before potential off-main-thread usage. r=lsalzman

Volume is not big but this is a startup crash and it is new in 83, taking for our RC build.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/b0a27eced655