1768237 - Crash in [@ mozilla::detail::RWLockImpl::writeLock] from gfxFontFamily::AddFontEntry

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/e2ee0395-b5bb-4512-9c9d-a59d70220505

MOZ_CRASH Reason: MOZ_RELEASE_ASSERT(pthread_rwlock_wrlock(&mRWLock) == 0) (pthread_rwlock_wrlock failed)

Top 10 frames of crashing thread:

0 firefox-bin mozilla::detail::RWLockImpl::writeLock mozglue/misc/RWLock_posix.cpp:57
1 libxul.so gfxFontFamily::AddFontEntry gfx/thebes/gfxFontEntry.h:879
2 libxul.so gfxPlatformFontList::AddWithLegacyFamilyName gfx/thebes/gfxPlatformFontList.cpp:455
3 libxul.so gfxFontFamily::CheckForLegacyFamilyNames gfx/thebes/gfxFontEntry.cpp:2057
4 libxul.so gfxPlatformFontList::FindAndAddFamiliesLocked gfx/thebes/gfxPlatformFontList.cpp:1593
5 libxul.so gfxFcPlatformFontList::FindAndAddFamiliesLocked gfx/thebes/gfxFcPlatformFontList.cpp:2197
6 libxul.so gfxFontGroup::BuildFontList gfx/thebes/gfxTextRun.cpp:1886
7 libxul.so nsFontCache::GetMetricsFor gfx/src/nsFontCache.cpp:92
8 libxul.so nsLayoutUtils::GetFontMetricsForFrame layout/base/nsLayoutUtils.cpp:4057
9 libxul.so nsTextControlFrame::CalcIntrinsicSize const layout/forms/nsTextControlFrame.cpp:199

Comment 1

[Andrew McCreight [:mccr8]]

This looks like it could be a regression. I'm not sure what that assert means. The earliest build I can see with this signature is 20220419214607, from this crash report: bp-b94c933f-32e6-4c8c-992a-68bc60220420

Comment 2

[Andrew McCreight [:mccr8]]

Here's the set of commits for that build. Bug 1756474 looks related.

Comment 3

[Jonathan Kew [:jfkthame]]

Yes, I see what's wrong -- the CheckForLegacyFamilyNames codepath leads to attempting to recursively lock the font-family, which isn't allowed. I'll look into this.

Comment 4

[Jonathan Kew [:jfkthame]]

There are actually at least two distinct issues here that manifest as a release-assert in RWLockImpl::writeLock, shown by different crash stacks: comment 0 has a stack that involves gfxFontFamily::CheckForLegacyFamilyNames, as do many of the other crashes, but there are also some like https://crash-stats.mozilla.org/report/index/223ac777-95d5-4d0b-aba4-831670220506:

0 	libmozglue.dylib 	mozilla::detail::RWLockImpl::writeLock() 	mozglue/misc/RWLock_posix.cpp:57 	context
1 	XUL 	gfxFontFamily::ReadOtherFamilyNames(gfxPlatformFontList*) 	gfx/thebes/gfxFontEntry.cpp:1943 	cfi
2 	XUL 	gfxFontFamily::HasOtherFamilyNames() 	gfx/thebes/gfxFontEntry.cpp:1551 	cfi
3 	XUL 	gfxMacFontFamily::LocalizedName(nsTSubstring<char>&) 	gfx/thebes/gfxMacPlatformFontList.mm:606 	cfi
4 	XUL 	gfxPlatformFontList::GetLocalizedFamilyName(FontFamily const&, nsTSubstring<char>&) 	gfx/thebes/gfxPlatformFontList.cpp:1814 	cfi
5 	XUL 	gfxPlatform::GetDefaultFontName(nsTSubstring<char> const&, nsTSubstring<char> const&) 	gfx/thebes/gfxPlatform.cpp:1734 	cfi
6 	XUL 	nsThebesFontEnumerator::GetDefaultFont(char const*, char const*, char16_t**) 	gfx/src/nsThebesFontEnumerator.cpp:210 	cfi

where the legacy-name code path is not involved.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1768237 - Don't take the lock in gfxFontFamily::HasOtherFamilyNames before calling ReadOtherFamilyNames, because it does the locking internally. r=lsalzman

This was a trivial error, and accounts for some of the crashes listed. It's wrong for HasOtherFamilyNames
to lock the gfxFontFamily record, because it then calls ReadOtherFamilyNames which does locking internally.
Other callsites of ReadOtherFamilyNames don't lock before calling it, we shouldn't do so here either.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1768237 - Clean up locking and checks for name/face duplication during CheckForLegacyFamilyNames. r=lsalzman

A number of the crash stacks here involve CheckForLegacyFamilyNames. I think what's happening in these
cases is that we've found a font family record via the mOtherFamilyNames table (which means it was the
result of a legacy-name or localized-name search already). That family is locked in CheckForLegacyFamilyNames,
but then if we find another version of the name in one of its faces, we may end up finding the same record
within AddWithLegacyFamilyName and trying to lock it recursively.

Cleaning up the locking in gfxFontFamily (we don't need to hold a write-lock in CheckForLegacyFamilyNames,
read-lock is sufficient) and suppressing the use of CheckForLegacyFamilyNames for family records that
were created via that codepath in the first place should prevent this happening.

Depends on D145970

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/26992be27222
Don't take the lock in gfxFontFamily::HasOtherFamilyNames before calling ReadOtherFamilyNames, because it does the locking internally. r=lsalzman
https://hg.mozilla.org/integration/autoland/rev/90ec539a76eb
Clean up locking and checks for name/face duplication during CheckForLegacyFamilyNames. r=lsalzman

Comment 8

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/26992be27222
https://hg.mozilla.org/mozilla-central/rev/90ec539a76eb

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:jfkthame, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 10

[Jonathan Kew [:jfkthame]]

Comment on attachment 9275866 [details]
Bug 1768237 - Don't take the lock in gfxFontFamily::HasOtherFamilyNames before calling ReadOtherFamilyNames, because it does the locking internally. r=lsalzman

Beta/Release Uplift Approval Request

• User impact if declined: Occasional crashes due to invalid attempts to recursively take a lock
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: (No known STR for testing, only crash-stats reports...)
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Relevant thread-safety annotations are present to help check the correctness of the change
• String changes made/needed:
• Is Android affected?: Yes

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9275866 [details]
Bug 1768237 - Don't take the lock in gfxFontFamily::HasOtherFamilyNames before calling ReadOtherFamilyNames, because it does the locking internally. r=lsalzman

Approved for 101.0b6.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9b21b4923105
https://hg.mozilla.org/releases/mozilla-beta/rev/779485a979fe