Crash under FontCache::purgeInactiveFontData(): Thread[0] EXC_BAD_ACCESS (SIGSEGV) (KERN_INVALID_ADDRESS at 0x0000000000000000) [ 0] 0x00007fff8e263884 WebCore`WebCore::FontCache::purgeInactiveFontData(unsigned int) + 308 at FontCache.cpp:429:17 425 426 while (purgeCount) { 427 Vector<RefPtr<Font>, 20> fontsToDelete; 428 for (auto& font : cachedFonts().values()) { -> 429 if (!font->hasOneRef()) 430 continue; 431 fontsToDelete.append(WTFMove(font)); 432 if (!--purgeCount) 433 break; 0x00007fff8e263870: cmpq %rax, %r15 0x00007fff8e263873: je 0x5ebb10 ; <+960> [inlined] WTF::Vector<WTF::RefPtr<WebCore::Font>, 20ul, WTF::CrashOnOverflow, 16ul>::size() const at Vector.h:654 0x00007fff8e263879: nopl (%rax) 0x00007fff8e263880: movq 0x38(%r15), %rcx -> 0x00007fff8e263884: cmpl $0x1, (%rcx) 0x00007fff8e263887: jne 0x5ebab4 ; <+868> [inlined] WTF::HashTableConstIterator<WebCore::FontPlatformData, WTF::KeyValuePair<WebCore::FontPlatformData, WTF::RefPtr<WebCore::Font> >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::FontPlatformData, WTF::RefPtr<WebCore::Font> > >, WebCore::FontDataCacheKeyHash, WTF::HashMap<WebCore::FontPlatformData, WTF::RefPtr<WebCore::Font>, WebCore::FontDataCacheKeyHash, WebCore::FontDataCacheKeyTraits, WTF::HashTraits<WTF::RefPtr<WebCore::Font> > >::KeyValuePairTraits, WebCore::FontDataCacheKeyTraits>::operator++() at HashTable.h:266 0x00007fff8e26388d: leaq 0x38(%r15), %r12 0x00007fff8e263891: movl -0xd4(%rbp), %eax 0x00007fff8e263897: movq -0xd8(%rbp), %r13 [ 1] 0x00007fff8e894ef7 WebCore`WebCore::MemoryPressureHandler::releaseNoncriticalMemory() + 55 at MemoryPressureHandler.cpp:82:9 78 void MemoryPressureHandler::releaseNoncriticalMemory() 79 { 80 { 81 ReliefLogger log("Purge inactive FontData"); -> 82 FontCache::singleton().purgeInactiveFontData(); 83 } 84 85 { 86 ReliefLogger log("Clear WidthCaches"); [ 2] 0x00007fff8e8954bc WebCore`WebCore::MemoryPressureHandler::releaseMemory(WebCore::Critical, WebCore::Synchronous) + 60 at MemoryPressureHandler.cpp:166:5 162 { 163 if (critical == Critical::Yes) 164 releaseCriticalMemory(synchronous); 165 -> 166 releaseNoncriticalMemory(); 167 168 platformReleaseMemory(critical); 169 170 { [ 3] 0x00007fff8e895d5f WebCore`WebCore::MemoryPressureHandler::respondToMemoryPressure(WebCore::Critical, WebCore::Synchronous) [inlined] std::__1::function<void (WebCore::Critical, WebCore::Synchronous)>::operator()(WebCore::Critical, WebCore::Synchronous) const + 17 at functional:1824:12 [ 3] 0x00007fff8e895d4e WebCore`WebCore::MemoryPressureHandler::respondToMemoryPressure(WebCore::Critical, WebCore::Synchronous) + 78 at MemoryPressureHandlerCocoa.mm:207
rdar://problem/25373970
Created attachment 276868 [details] WIP patch
Created attachment 276869 [details] WIP patch
Created attachment 276871 [details] Patch
Created attachment 276897 [details] Patch
Created attachment 276934 [details] Patch
Different crash trace with the same root cause (having a null Font pointer in the FontCache): Thread[0] EXC_BAD_ACCESS (SIGSEGV) (KERN_INVALID_ADDRESS at 0x0000000000000000) [ 0] 0x00000001140969b7 WebCore`WebCore::FontCache::inactiveFontCount() [inlined] WTF::RefCountedBase::hasOneRef() const at RefCounted.h:55:27 0x00000001140969a1: je 0x5dfa0e ; <+302> at FontCache.cpp:493 0x00000001140969a3: nopw %cs:(%rax,%rax) 0x00000001140969b0: movl %r14d, %r13d 0x00000001140969b3: movq 0x38(%rax), %rcx -> 0x00000001140969b7: cmpl $0x1, (%rcx) 0x00000001140969ba: sete %cl 0x00000001140969bd: movzbl %cl, %r14d 0x00000001140969c1: movq %rax, %rbx 0x00000001140969c4: addq $0x40, %rbx [ 0] 0x00000001140969b7 WebCore`WebCore::FontCache::inactiveFontCount() + 215 at FontCache.cpp:490 486 FontLocker fontLocker; 487 #endif 488 unsigned count = 0; 489 for (auto& font : cachedFonts().values()) { -> 490 if (font->hasOneRef()) 491 ++count; 492 } 493 return count; 494 } [ 1] 0x0000000113ac3c1d WebCore`WebCore::FontCache::purgeInactiveFontDataIfNeeded() + 61 at FontCache.cpp:409:30 405 unsigned inactiveFontDataLimit = underMemoryPressure ? cMaxUnderMemoryPressureInactiveFontData : cMaxInactiveFontData; 406 407 if (cachedFonts().size() < inactiveFontDataLimit) 408 return; -> 409 unsigned inactiveCount = inactiveFontCount(); 410 if (inactiveCount <= inactiveFontDataLimit) 411 return; 412 413 unsigned targetFontDataLimit = underMemoryPressure ? cTargetUnderMemoryPressureInactiveFontData : cTargetInactiveFontData;
Comment on attachment 276934 [details] Patch Could we at least add a test that would have hit an assertion we could have added to the code before fixing this?
(In reply to comment #8) > Comment on attachment 276934 [details] > Patch > > Could we at least add a test that would have hit an assertion we could have > added to the code before fixing this? This is really not easy to test because we would both need to: 1. Write a test where we end up with a null cgFont. I do not know how to make this happen but maybe Myles knows. When I discussed this with Myles, my understanding was that it is still unclear when or why CTFontCopyGraphicsFont() returns null. 2. Have the test trigger pruning of the FontCache (We would add an Internals API for this).
(In reply to comment #9) > (In reply to comment #8) > > Comment on attachment 276934 [details] > > Patch > > > > Could we at least add a test that would have hit an assertion we could have > > added to the code before fixing this? > > This is really not easy to test because we would both need to: > 1. Write a test where we end up with a null cgFont. I do not know how to > make this happen but maybe Myles knows. When I discussed this with Myles, my > understanding was that it is still unclear when or why > CTFontCopyGraphicsFont() returns null. I found that CTFontCopyGraphicsFont() returns null if the CTFontRef we pass it is null. We still need to figure out when we pass a null CFFontRef to CTFontCopyGraphicsFont() though. > 2. Have the test trigger pruning of the FontCache (We would add an Internals > API for this).
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > Comment on attachment 276934 [details] > > > Patch > > > > > > Could we at least add a test that would have hit an assertion we could have > > > added to the code before fixing this? > > > > This is really not easy to test because we would both need to: > > 1. Write a test where we end up with a null cgFont. I do not know how to > > make this happen but maybe Myles knows. When I discussed this with Myles, my > > understanding was that it is still unclear when or why > > CTFontCopyGraphicsFont() returns null. > > I found that CTFontCopyGraphicsFont() returns null if the CTFontRef we pass > it is null. We still need to figure out when we pass a null CFFontRef to > CTFontCopyGraphicsFont() though. I found for example that we have a client API called _web_drawAtPoint in WebKitNSStringExtras that takes a NSFont* in and passes that directly to FontPlatformData() without null check. So if the client were to pass a null font, we would see the issue. Similar issue with centerTruncateString / centerTruncateString in WebStringTruncator API. There may be others. > > > 2. Have the test trigger pruning of the FontCache (We would add an Internals > > API for this).
Comment on attachment 276934 [details] Patch r=me. No comments.
Comment on attachment 276934 [details] Patch I will investigate with Myles as well how / when we end up with a null cgFont so that we can correct the problem at the source and never construct a FontPlatformData without a cgFont.
(In reply to comment #9) > (In reply to comment #8) > > Comment on attachment 276934 [details] > > Patch > > > > Could we at least add a test that would have hit an assertion we could have > > added to the code before fixing this? > > This is really not easy to test because we would both need to: > 1. Write a test where we end up with a null cgFont. I do not know how to > make this happen but maybe Myles knows. When I discussed this with Myles, my > understanding was that it is still unclear when or why > CTFontCopyGraphicsFont() returns null. It will only return nullptr if the input is nullptr or if LastResort is unavailable (in which case the user has huge problems anyway) > 2. Have the test trigger pruning of the FontCache (We would add an Internals > API for this).
(In reply to comment #14) > (In reply to comment #9) > > (In reply to comment #8) > > > Comment on attachment 276934 [details] > > > Patch > > > > > > Could we at least add a test that would have hit an assertion we could have > > > added to the code before fixing this? > > > > This is really not easy to test because we would both need to: > > 1. Write a test where we end up with a null cgFont. I do not know how to > > make this happen but maybe Myles knows. When I discussed this with Myles, my > > understanding was that it is still unclear when or why > > CTFontCopyGraphicsFont() returns null. > > It will only return nullptr if the input is nullptr or if LastResort is > unavailable (in which case the user has huge problems anyway) > > > 2. Have the test trigger pruning of the FontCache (We would add an Internals > > API for this). Because CoreText doesn't now how to draw glyphs directly, any time anyone wants to draw using a CTFont, this same code path is used to create a CGFont.
(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #9) > > > (In reply to comment #8) > > > > Comment on attachment 276934 [details] > > > > Patch > > > > > > > > Could we at least add a test that would have hit an assertion we could have > > > > added to the code before fixing this? > > > > > > This is really not easy to test because we would both need to: > > > 1. Write a test where we end up with a null cgFont. I do not know how to > > > make this happen but maybe Myles knows. When I discussed this with Myles, my > > > understanding was that it is still unclear when or why > > > CTFontCopyGraphicsFont() returns null. > > > > It will only return nullptr if the input is nullptr or if LastResort is > > unavailable (in which case the user has huge problems anyway) > > > > > 2. Have the test trigger pruning of the FontCache (We would add an Internals > > > API for this). > > Because CoreText doesn't now how to draw glyphs directly, any time anyone > wants to draw using a CTFont, this same code path is used to create a CGFont. Inside WebCore, we use this constructor in 3 places. Two of them have null checks around them. The last one would only return null if we are out of memory, in which case we would crash anyway.
(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #14) > > > (In reply to comment #9) > > > > (In reply to comment #8) > > > > > Comment on attachment 276934 [details] > > > > > Patch > > > > > > > > > > Could we at least add a test that would have hit an assertion we could have > > > > > added to the code before fixing this? > > > > > > > > This is really not easy to test because we would both need to: > > > > 1. Write a test where we end up with a null cgFont. I do not know how to > > > > make this happen but maybe Myles knows. When I discussed this with Myles, my > > > > understanding was that it is still unclear when or why > > > > CTFontCopyGraphicsFont() returns null. > > > > > > It will only return nullptr if the input is nullptr or if LastResort is > > > unavailable (in which case the user has huge problems anyway) > > > > > > > 2. Have the test trigger pruning of the FontCache (We would add an Internals > > > > API for this). > > > > Because CoreText doesn't now how to draw glyphs directly, any time anyone > > wants to draw using a CTFont, this same code path is used to create a CGFont. > > Inside WebCore, we use this constructor in 3 places. Two of them have null > checks around them. The last one would only return null if we are out of > memory, in which case we would crash anyway. Sorry, this is false. Please disregard this comment.
Comment on attachment 276934 [details] Patch Clearing flags on attachment: 276934 Committed r199890: <http://trac.webkit.org/changeset/199890>
All reviewed patches have been landed. Closing bug.