Bug 67906 - Use of static RefPtr in FontPlatformData causes global destructor ordering problems
Summary: Use of static RefPtr in FontPlatformData causes global destructor ordering pr...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-11 14:13 PDT by Marshall Greenblatt
Modified: 2011-09-20 15:38 PDT (History)
2 users (show)

See Also:

Attachments
Proposed fix for bug 67906. (1.62 KB, patch)
2011-09-11 14:18 PDT, Marshall Greenblatt 		no flags Details | Formatted Diff | Diff
Proposed fix for bug 67906. (1.49 KB, patch)
2011-09-11 14:24 PDT, Marshall Greenblatt 		abarth: review-
Details | Formatted Diff | Diff
Proposed Fix #3 (1.56 KB, patch)
2011-09-12 06:23 PDT, Marshall Greenblatt 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Marshall Greenblatt 2011-09-11 14:13:34 PDT 
From WebKit/Source/WebCore/platform/graphics/chromium/FontPlatformDataChromiumWin.cpp:

FontPlatformData::RefCountedHFONT* FontPlatformData::hashTableDeletedFontValue()
{
    static RefPtr<RefCountedHFONT> deletedValue =
        RefCountedHFONT::create(reinterpret_cast<HFONT>(-1));
    return deletedValue.get();
}

The problem with using static RefPtr is that, on Windows, the destructor of |deletedValue| will not be called until doexit() is executed on the main application thread. In single-process applications (like Chromium Embedded Framework) the main application thread may not be the same as the WebKit thread. This problem was exposed by WebKit revision 92254 which adds asserts to RefCounted to make sure ref/deref happens on the right thread.

According to the WebKit experts the correct solution is to intentionally leak the static RefCountedHFONT object using leakRef().
Comment 1 Marshall Greenblatt 2011-09-11 14:18:54 PDT 
Created attachment 107007 [details]
Proposed fix for bug 67906.

Please review the attached bug_67906.patch.
Comment 2 Marshall Greenblatt 2011-09-11 14:24:31 PDT 
Created attachment 107008 [details]
Proposed fix for bug 67906.

Please review the attached patch.
Comment 3 Adam Barth 2011-09-11 20:38:48 PDT 
Comment on attachment 107008 [details]
Proposed fix for bug 67906.

This looks fine, but we should use DECLARE_STATIC_LOCAL though (and I would merge those too lines together).
Comment 4 Marshall Greenblatt 2011-09-12 06:23:37 PDT 
Created attachment 107045 [details]
Proposed Fix #3

Changed to use DEFINE_STATIC_LOCAL.
Comment 5 Marshall Greenblatt 2011-09-12 06:28:58 PDT 
(In reply to comment #4)
> Created an attachment (id=107045) [details]
> Proposed Fix #3
> 
> Changed to use DEFINE_STATIC_LOCAL.

Note: The above patch doesn't use leakRef() in combination with DEFINE_STATIC_LOCAL. This is because DEFINE_STATIC_LOCAL is already leaking a RefPtr that is holding a reference to the object, so I don't believe it's necessary to additionally call leakRef().
Comment 6 Marshall Greenblatt 2011-09-20 13:50:25 PDT 
(In reply to comment #3)
> (From update of attachment 107008 [details])
> This looks fine, but we should use DECLARE_STATIC_LOCAL though (and I would merge those too lines together).

Hi Adam, can you review the updated patch? Thanks.
Comment 7 Adam Barth 2011-09-20 14:02:32 PDT 
Comment on attachment 107045 [details]
Proposed Fix #3

Looks great.  Sorry for dropping the ball.
Comment 8 WebKit Review Bot 2011-09-20 15:38:43 PDT 
Comment on attachment 107045 [details]
Proposed Fix #3

Clearing flags on attachment: 107045

Committed r95576: <http://trac.webkit.org/changeset/95576>
Comment 9 WebKit Review Bot 2011-09-20 15:38:49 PDT 
All reviewed patches have been landed.  Closing bug.