app_server: fix deadlock in GlyphLayoutEngine

We want to first try fallbacks with the same style as the main font, if
available, but that introduces the chance of two threads trying to
acquire the same locks in different order, so keep at most the main font
and one fallback locked, and acquire them in a fixed order by address.

Fixes: #17850
Change-Id: Ic352fadc46eb257d2bca4804962b11ab1eb9fa12
Reviewed-on: https://review.haiku-os.org/c/haiku/+/5557
Tested-by: Commit checker robot <no-reply+buildbot@haiku-os.org>
Reviewed-by: waddlesplash <waddlesplash@gmail.com>

src/servers/app/ServerFont.cpp

@@ -687,12 +687,13 @@ ServerFont::GetHasGlyphs(const char* string, int32 numBytes, int32 numChars,
 	FontCacheEntry* entry = NULL;
 	FontCacheReference cacheReference;
 	BObjectList<FontCacheReference> fallbacks(21, true);
-	int32 fallbacksCount = -1;
 
 	entry = GlyphLayoutEngine::FontCacheEntryFor(*this, false);
-	if (entry == NULL || !cacheReference.SetTo(entry, false))
+	if (entry == NULL)
 		return B_ERROR;
 
+	cacheReference.SetTo(entry);
+
 	uint32 charCode;
 	int32 charIndex = 0;
 	const char* start = string;
@@ -700,20 +701,11 @@ ServerFont::GetHasGlyphs(const char* string, int32 numBytes, int32 numChars,
 		hasArray[charIndex] = entry->CanCreateGlyph(charCode);
 
 		if (hasArray[charIndex] == false) {
-			if (fallbacksCount < 0) {
+			if (fallbacks.IsEmpty())
-				GlyphLayoutEngine::PopulateAndLockFallbacks(
+				GlyphLayoutEngine::PopulateFallbacks(fallbacks, *this, false);
-					fallbacks, *this, false, false);
-				fallbacksCount = fallbacks.CountItems();
-			}
 
-			for (int32 index = 0; index < fallbacksCount; index++) {
+			if (GlyphLayoutEngine::GetFallbackReference(fallbacks, charCode) != NULL)
-				FontCacheEntry* fallbackEntry
+				hasArray[charIndex] = true;
-					= fallbacks.ItemAt(index)->Entry();
-				if (fallbackEntry->CanCreateGlyph(charCode)) {
-					hasArray[charIndex] = true;
-					break;
-				}
-			}
 		}
 
 		charIndex++;

src/servers/app/font/GlyphLayoutEngine.h

@@ -1,5 +1,5 @@
 /*
- * Copyright 2007-2014, Haiku. All rights reserved.
+ * Copyright 2007-2022, Haiku. All rights reserved.
  * Distributed under the terms of the MIT License.
  *
  * Authors:
@@ -28,63 +28,129 @@ public:
 	FontCacheReference()
 		:
 		fCacheEntry(NULL),
+		fFallbackReference(NULL),
+		fLocked(false),
 		fWriteLocked(false)
 	{
 	}
 
 	~FontCacheReference()
-	{
-		Unset();
-	}
-
-	bool SetTo(FontCacheEntry* entry, bool writeLock)
-	{
-		ASSERT(entry != NULL);
-
-		if (entry == fCacheEntry) {
-			if (writeLock == fWriteLocked)
-				return true;
-			UnlockAndDisown();
-		} else if (fCacheEntry != NULL)
-			Unset();
-
-		if (writeLock) {
-			if (!entry->WriteLock()) {
-				FontCache::Default()->Recycle(entry);
-				return false;
-			}
-		} else if (!entry->ReadLock()) {
-			FontCache::Default()->Recycle(entry);
-			return false;
-		}
-
-		fCacheEntry = entry;
-		fWriteLocked = writeLock;
-		return true;
-	}
-
-	FontCacheEntry* UnlockAndDisown()
-	{
-		if (fCacheEntry == NULL)
-			return NULL;
-
-		if (fWriteLocked)
-			fCacheEntry->WriteUnlock();
-		else
-			fCacheEntry->ReadUnlock();
-
-		FontCacheEntry* entry = fCacheEntry;
-		fCacheEntry = NULL;
-		fWriteLocked = false;
-		return entry;
-	}
-
-	void Unset()
 	{
 		if (fCacheEntry == NULL)
 			return;
 
-		FontCache::Default()->Recycle(UnlockAndDisown());
+		fFallbackReference = NULL;
+		Unlock();
+		if (fCacheEntry != NULL)
+			FontCache::Default()->Recycle(fCacheEntry);
+	}
+
+	void SetTo(FontCacheEntry* entry)
+	{
+		ASSERT(entry != NULL);
+		ASSERT(fCacheEntry == NULL);
+
+		fCacheEntry = entry;
+	}
+
+	bool ReadLock()
+	{
+		ASSERT(fCacheEntry != NULL);
+		ASSERT(fWriteLocked == false);
+
+		if (fLocked)
+			return true;
+
+		if (!fCacheEntry->ReadLock()) {
+			_Cleanup();
+			return false;
+		}
+
+		fLocked = true;
+		return true;
+	}
+
+	bool WriteLock()
+	{
+		ASSERT(fCacheEntry != NULL);
+
+		if (fWriteLocked)
+			return true;
+
+		if (fLocked) {
+			if (!fCacheEntry->ReadUnlock()) {
+				_Cleanup();
+				return false;
+			}
+		}
+		if (!fCacheEntry->WriteLock()) {
+			_Cleanup();
+			return false;
+		}
+
+		fLocked = true;
+		fWriteLocked = true;
+		return true;
+	}
+
+	bool Unlock()
+	{
+		ASSERT(fCacheEntry != NULL);
+
+		if (!fLocked)
+			return true;
+
+		if (fWriteLocked) {
+			if (!fCacheEntry->WriteUnlock()) {
+				_Cleanup();
+				return false;
+			}
+		} else {
+			if (!fCacheEntry->ReadUnlock()) {
+				_Cleanup();
+				return false;
+			}
+		}
+
+		fLocked = false;
+		fWriteLocked = false;
+		return true;
+	}
+
+	bool SetFallback(FontCacheReference* fallback)
+	{
+		ASSERT(fCacheEntry != NULL);
+		ASSERT(fallback != NULL);
+		ASSERT(fallback->Entry() != NULL);
+		ASSERT(fallback->Entry() != fCacheEntry);
+
+		if (fFallbackReference == fallback)
+			return true;
+
+		if (fFallbackReference != NULL) {
+			fFallbackReference->Unlock();
+			fFallbackReference = NULL;
+		}
+
+		// We need to create new glyphs with the engine of the fallback font
+		// and store them in the main font cache (not just transfer them from
+		// one cache to the other). So we need both to be write-locked.
+		if (fallback->Entry() < fCacheEntry) {
+			if (fLocked && !Unlock())
+				return false;
+			if (!fallback->WriteLock()) {
+				WriteLock();
+				return false;
+			}
+			fFallbackReference = fallback;
+			return WriteLock();
+		}
+		if (fLocked && !fWriteLocked && !Unlock())
+			return false;
+		if (!WriteLock() || !fallback->WriteLock())
+			return false;
+		fFallbackReference = fallback;
+		return true;
 	}
 
 	inline FontCacheEntry* Entry() const
@@ -97,8 +163,25 @@ public:
 		return fWriteLocked;
 	}
 
+private:
+
+	void _Cleanup()
+	{
+		if (fFallbackReference != NULL) {
+			fFallbackReference->Unlock();
+			fFallbackReference = NULL;
+		}
+		if (fCacheEntry != NULL)
+			FontCache::Default()->Recycle(fCacheEntry);
+		fCacheEntry = NULL;
+		fLocked = false;
+		fWriteLocked = false;
+	}
+
 private:
 			FontCacheEntry*		fCacheEntry;
+			FontCacheReference*	fFallbackReference;
+			bool				fLocked;
 			bool				fWriteLocked;
 };
 
@@ -120,10 +203,13 @@ public:
 									const BPoint* offsets = NULL,
 									FontCacheReference* cacheReference = NULL);
 
-	static	void				PopulateAndLockFallbacks(
+	static	void				PopulateFallbacks(
 									BObjectList<FontCacheReference>& fallbacks,
-									const ServerFont& font, bool forceVector,
+									const ServerFont& font, bool forceVector);
-									bool writeLock);
+
+	static FontCacheReference*	GetFallbackReference(
+									BObjectList<FontCacheReference>& fallbacks,
+									uint32 charCode);
 
 private:
 	static	const GlyphCache*	_CreateGlyph(
@@ -198,7 +284,8 @@ GlyphLayoutEngine::LayoutGlyphs(GlyphConsumer& consumer,
 
 		if (entry == NULL)
 			return false;
-		if (!pCacheReference->SetTo(entry, false))
+		pCacheReference->SetTo(entry);
+		if (!pCacheReference->ReadLock())
 			return false;
 	} // else the entry was already used and is still locked
 
@@ -292,42 +379,35 @@ GlyphLayoutEngine::_CreateGlyph(FontCacheReference& cacheReference,
 {
 	FontCacheEntry* entry = cacheReference.Entry();
 
-	// Avoid loading and locking the fallbacks if our font can create the glyph.
+	// Avoid loading the fallbacks if our font can create the glyph.
 	if (entry->CanCreateGlyph(charCode)) {
-		if (cacheReference.SetTo(entry, true))
+		if (cacheReference.WriteLock())
 			return entry->CreateGlyph(charCode);
 		return NULL;
 	}
 
-	if (fallbacks.IsEmpty()) {
+	if (fallbacks.IsEmpty())
-		// We need to create new glyphs with the engine of the fallback font
+		PopulateFallbacks(fallbacks, font, forceVector);
-		// and store them in the main font cache (not just transfer them from
+
-		// one cache to the other). So we need both to be write-locked.
+	FontCacheReference* fallbackReference = GetFallbackReference(fallbacks, charCode);
-		// The main font is unlocked first, in case it is also in the fallback
+	if (fallbackReference != NULL) {
-		// list, so that we always keep the same order to avoid deadlocks.
+		if (cacheReference.SetFallback(fallbackReference))
-		cacheReference.UnlockAndDisown();
+			return entry->CreateGlyph(charCode, fallbackReference->Entry());
-		PopulateAndLockFallbacks(fallbacks, font, forceVector, true);
+		if (cacheReference.Entry() == NULL)
-		if (!cacheReference.SetTo(entry, true)) {
 			return NULL;
-		}
 	}
 
-	int32 count = fallbacks.CountItems();
+	// No one knows how to draw this, so use the missing glyph symbol.
-	for (int32 index = 0; index < count; index++) {
+	if (cacheReference.WriteLock())
-		FontCacheEntry* fallbackEntry = fallbacks.ItemAt(index)->Entry();
+		return entry->CreateGlyph(charCode);
-		if (fallbackEntry->CanCreateGlyph(charCode))
+	return NULL;
-			return entry->CreateGlyph(charCode, fallbackEntry);
-	}
-
-	return entry->CreateGlyph(charCode);
-		// No one knows how to draw this, so use the missing glyph symbol.
 }
 
 
 inline void
-GlyphLayoutEngine::PopulateAndLockFallbacks(
+GlyphLayoutEngine::PopulateFallbacks(
 	BObjectList<FontCacheReference>& fallbacksList,
-	const ServerFont& font, bool forceVector, bool writeLock)
+	const ServerFont& font, bool forceVector)
 {
 	ASSERT(fallbacksList.IsEmpty());
 
@@ -347,12 +427,9 @@ GlyphLayoutEngine::PopulateAndLockFallbacks(
 	if (!gFontManager->Lock())
 		return;
 
-	static const int nStyles = 3;
 	static const int nFallbacks = B_COUNT_OF(fallbacks);
-	FontCacheEntry* fallbackCacheEntries[nStyles * nFallbacks];
-	int entries = 0;
 
-	for (int c = 0; c < nStyles; c++) {
+	for (int c = 0; c < 3; c++) {
 		const char* fontStyle;
 		if (c == 0)
 			fontStyle = font.Style();
@@ -365,39 +442,42 @@ GlyphLayoutEngine::PopulateAndLockFallbacks(
 
 			FontStyle* fallbackStyle = gFontManager->GetStyle(fallbacks[i],
 				fontStyle, 0xffff, 0);
-
 			if (fallbackStyle == NULL)
 				continue;
 
 			ServerFont fallbackFont(*fallbackStyle, font.Size());
 
-			FontCacheEntry* entry = FontCacheEntryFor(
+			FontCacheEntry* entry = FontCacheEntryFor(fallbackFont, forceVector);
-				fallbackFont, forceVector);
-
 			if (entry == NULL)
 				continue;
 
-			fallbackCacheEntries[entries++] = entry;
+			FontCacheReference* cacheReference = new(std::nothrow) FontCacheReference();
+			if (cacheReference != NULL) {
+				cacheReference->SetTo(entry);
+				fallbacksList.AddItem(cacheReference);
+			} else
+				FontCache::Default()->Recycle(entry);
 
 		}
 
 	}
 
 	gFontManager->Unlock();
+}
 
-	// Finally lock the entries and save their references
+
-	for (int i = 0; i < entries; i++) {
+inline FontCacheReference*
-		FontCacheReference* cacheReference =
+GlyphLayoutEngine::GetFallbackReference(
-			new(std::nothrow) FontCacheReference();
+	BObjectList<FontCacheReference>& fallbacks, uint32 charCode)
-		if (cacheReference != NULL) {
+{
-			if (cacheReference->SetTo(fallbackCacheEntries[i], writeLock))
+	int32 count = fallbacks.CountItems();
-				fallbacksList.AddItem(cacheReference);
+	for (int32 index = 0; index < count; index++) {
-			else
+		FontCacheReference* fallbackReference = fallbacks.ItemAt(index);
-				delete cacheReference;
+		FontCacheEntry* fallbackEntry = fallbackReference->Entry();
-		}
+		if (fallbackEntry != NULL && fallbackEntry->CanCreateGlyph(charCode))
+			return fallbackReference;
 	}
-
+	return NULL;
-	return;
 }