Bug 156822 – Crash under FontCache::purgeInactiveFontData()

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Chris Dumez]

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

[Chris Dumez]

rdar://problem/25373970

[Chris Dumez]

Created attachment 276868 [details]
WIP patch

[Chris Dumez]

Created attachment 276869 [details]
WIP patch

[Chris Dumez]

Created attachment 276871 [details]
Patch

[Chris Dumez]

Created attachment 276897 [details]
Patch

[Chris Dumez]

Created attachment 276934 [details]
Patch

[Chris Dumez]

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;

[Darin Adler]

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?

[Chris Dumez]

(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).

[Chris Dumez]

(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).

[Chris Dumez]

(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).

[Myles C. Maxfield]

Comment on attachment 276934 [details]
Patch

r=me. No comments.

[Chris Dumez]

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.

[Myles C. Maxfield]

(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).

[Myles C. Maxfield]

(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.

[Myles C. Maxfield]

(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.

[Myles C. Maxfield]

(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.

[WebKit Commit Bot]

Comment on attachment 276934 [details]
Patch

Clearing flags on attachment: 276934

Committed r199890: <http://trac.webkit.org/changeset/199890>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.