Bug 22444 - Bad Default Constructor for Cairo FontPlatformData Causes Infinite Loop in FontCache.cpp
Summary: Bad Default Constructor for Cairo FontPlatformData Causes Infinite Loop in Fo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Windows XP
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-23 21:12 PST by Brent Fulgham
Modified: 2021-04-12 14:20 PDT (History)
2 users (show)

See Also:


Attachments
Patch to revert static objects from DEFINE_STATIC_LOCAL (992 bytes, patch)
2008-11-23 21:39 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Correct default constructor for Cairo FontPlatformData. (1.19 KB, patch)
2008-11-24 10:55 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Correct default constructor for Cairo FontPlatformData. (1.22 KB, patch)
2008-11-24 11:00 PST, Brent Fulgham
ap: review+
Details | Formatted Diff | Diff
Correcte default constructor for Cairo FontPlatformData. (1.22 KB, patch)
2008-11-24 11:18 PST, Brent Fulgham
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2008-11-23 21:12:10 PST
I tracked down the cause of a deadlock that began appearing after some of the changes starting at @r35364 through current.

Behavior:  Launching the WinLauncher executable executes to the point that the main frame is displayed, but the test rendering page is never displayed.  Breaking in the debugger repeatedly revealed that the program was locked attempting to set a missing font entry in the font cache:

[FontCache.cpp: 174]        gFontPlatformDataCache->set(key, result);

The debugger was actually stuck in the hash table implementation (JSC::hashable.h, static bool isDeletedBucket).

By working from an existing good source tree, I found that the problem occurs as soon as the new DEFINE_STATIC_LOCAL calls were introduced into the FontCache object.
Comment 1 Brent Fulgham 2008-11-23 21:18:17 PST
Further testing shows that the two 
Comment 2 Brent Fulgham 2008-11-23 21:29:47 PST
Further testing shows that the problem is with the two static emptyValue methods:

FontDataCacheKeyTraits::emptyValue
FontPlatformDataCacheKeyTraits::emptyValue

If either (or both) of these methods makes use of the DEFINE_STATIC_LOCAL macro the WebView will deadlock during rendering of the test page.  The other DEFINE_STATIC_LOCAL macro uses do not seem to create any problems.
Comment 3 Brent Fulgham 2008-11-23 21:34:00 PST
Test system:

Windows XP, 3.5 GB RAM.  2.9 GHZ Pentium D CPU.

I don't know if this bug is exhibited due to the lack of true dual cores on this test system, but I suspect the issue is related in some fashion to the change from static objects existing for the full life of the program, and the newer 'on-demand' style of initialization.

Comment 4 Brent Fulgham 2008-11-23 21:39:30 PST
Created attachment 25411 [details]
Patch to revert static objects from DEFINE_STATIC_LOCAL
Comment 5 Alexey Proskuryakov 2008-11-23 23:11:52 PST
An infinite loop in HashTable is usually caused by trashed memory. I wonder if MSVC could also have a bug WRT lifetime of static references.
Comment 6 Brent Fulgham 2008-11-24 09:18:23 PST
Compiler information:
Visual Studio 2005 (8.0.50727.762, SP.050727-7600)
Comment 7 Brent Fulgham 2008-11-24 09:53:19 PST
The specific problem seems to be that the HashTable implementation 'add' method can't figure out how to handle the key that was passed into the routine.  If breakpoints are placed on each of the ways of breaking out of the "while (1)" loop in hashtable.h (around line 636-653), none are ever hit.

The key that is passed in appears to be fully-constructed.

It first calls "isEmptyOrDeleted", which calls the troublesome "emptyValue" method.  It appears to keep returning the same address for its object.  However, changing the declaration back to a stack-allocated object causes the routine to perform properly.  The return value is used in an equality operation, which indicates that the two objects are not the same .

They are not the same because m_scaledFont is set to 0xbaadf00d in the 'emptyValue' object, while the passed in key is set to 0x00.


Comment 8 Brent Fulgham 2008-11-24 10:34:20 PST
Problem was traced to the FontPlatformData.h default constructor.  This did not initialize the Cairo-specific m_scaledFont member, resulting in it defaulting to 0xbaadf00d.  Previously, the stack-allocated static nature of the object triggered the object to be 0-initialized before use.

Patch to come.
Comment 9 Brent Fulgham 2008-11-24 10:55:00 PST
Created attachment 25434 [details]
Correct default constructor for Cairo FontPlatformData.
Comment 10 Brent Fulgham 2008-11-24 11:00:26 PST
Created attachment 25435 [details]
Correct default constructor for Cairo FontPlatformData.

Really!  I mean it this time.
Comment 11 Alexey Proskuryakov 2008-11-24 11:09:02 PST
Comment on attachment 25435 [details]
Correct default constructor for Cairo FontPlatformData.

r=me
Comment 12 Brent Fulgham 2008-11-24 11:18:14 PST
Created attachment 25437 [details]
Correcte default constructor for Cairo FontPlatformData.

Corrected extra blank line.
Comment 13 Alexey Proskuryakov 2008-11-24 11:19:27 PST
Comment on attachment 25437 [details]
Correcte default constructor for Cairo FontPlatformData.

r=me
Comment 14 Alexey Proskuryakov 2008-11-24 11:22:16 PST
Committed revision 38713.