Bug 156822 - Crash under FontCache::purgeInactiveFontData()
Summary: Crash under FontCache::purgeInactiveFontData()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-04-20 16:53 PDT by Chris Dumez
Modified: 2016-04-22 12:24 PDT (History)
4 users (show)

See Also:


Attachments
WIP patch (9.77 KB, patch)
2016-04-20 16:56 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP patch (10.08 KB, patch)
2016-04-20 16:59 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (12.80 KB, patch)
2016-04-20 17:08 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (17.46 KB, patch)
2016-04-20 21:32 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (17.19 KB, patch)
2016-04-21 09:05 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2016-04-20 16:53:24 PDT
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
Comment 1 Chris Dumez 2016-04-20 16:53:44 PDT
rdar://problem/25373970
Comment 2 Chris Dumez 2016-04-20 16:56:18 PDT
Created attachment 276868 [details]
WIP patch
Comment 3 Chris Dumez 2016-04-20 16:59:14 PDT
Created attachment 276869 [details]
WIP patch
Comment 4 Chris Dumez 2016-04-20 17:08:53 PDT
Created attachment 276871 [details]
Patch
Comment 5 Chris Dumez 2016-04-20 21:32:12 PDT
Created attachment 276897 [details]
Patch
Comment 6 Chris Dumez 2016-04-21 09:05:23 PDT
Created attachment 276934 [details]
Patch
Comment 7 Chris Dumez 2016-04-21 12:03:23 PDT
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 8 Darin Adler 2016-04-21 18:48:36 PDT
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?
Comment 9 Chris Dumez 2016-04-21 18:55:34 PDT
(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).
Comment 10 Chris Dumez 2016-04-21 19:00:17 PDT
(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).
Comment 11 Chris Dumez 2016-04-21 19:19:46 PDT
(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 12 Myles C. Maxfield 2016-04-22 11:28:35 PDT
Comment on attachment 276934 [details]
Patch

r=me. No comments.
Comment 13 Chris Dumez 2016-04-22 11:44:09 PDT
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.
Comment 14 Myles C. Maxfield 2016-04-22 11:53:59 PDT
(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).
Comment 15 Myles C. Maxfield 2016-04-22 12:02:30 PDT
(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.
Comment 16 Myles C. Maxfield 2016-04-22 12:09:13 PDT
(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.
Comment 17 Myles C. Maxfield 2016-04-22 12:19:21 PDT
(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 18 WebKit Commit Bot 2016-04-22 12:24:22 PDT
Comment on attachment 276934 [details]
Patch

Clearing flags on attachment: 276934

Committed r199890: <http://trac.webkit.org/changeset/199890>
Comment 19 WebKit Commit Bot 2016-04-22 12:24:32 PDT
All reviewed patches have been landed.  Closing bug.