Bug 54905 - Remove global initializer in CookieStorageCFNet.cpp
Summary: Remove global initializer in CookieStorageCFNet.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 51836
  Show dependency treegraph
 
Reported: 2011-02-21 13:44 PST by Pratik Solanki
Modified: 2011-02-21 16:15 PST (History)
3 users (show)

See Also:


Attachments
Patch (2.17 KB, patch)
2011-02-21 13:48 PST, Pratik Solanki
no flags Details | Formatted Diff | Diff
Patch (2.16 KB, patch)
2011-02-21 14:54 PST, Pratik Solanki
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pratik Solanki 2011-02-21 13:44:46 PST
When compiling CookieStorageCFNet.cpp on Mac, I get

PhaseScriptExecution "Check For Global Initializers"
ERROR: CookieStorageCFNet.o has one or more global initializers in it! (/Volumes/Data/psolanki/sources/external/WebKit.git/WebKitBuild/cfnetwork-mac/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/CookieStorageCFNet.o), near WebCore::currentCookieStorage() __ZN7WebCore20currentCookieStorageEv.eh
Comment 1 Pratik Solanki 2011-02-21 13:48:28 PST
Created attachment 83212 [details]
Patch

First stab at fixing this error. Is there a better way to do this?
Comment 2 Adam Barth 2011-02-21 14:11:53 PST
Comment on attachment 83212 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83212&action=review

> Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:43
> -static RetainPtr<CFHTTPCookieStorageRef> s_cookieStorage;
> +static CFHTTPCookieStorageRef s_cookieStorage;

Do we need to init s_cookieStorage to 0 ?

> Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:70
> +        s_cookieStorage = wkCreatePrivateHTTPCookieStorage();

Should we assert that s_cookieStorage is 0 here?
Comment 3 Pratik Solanki 2011-02-21 14:16:43 PST
Comment on attachment 83212 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83212&action=review

>> Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:43
>> +static CFHTTPCookieStorageRef s_cookieStorage;
> 
> Do we need to init s_cookieStorage to 0 ?

Statics are initialized to 0 so we should be fine here.

>> Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:70
>> +        s_cookieStorage = wkCreatePrivateHTTPCookieStorage();
> 
> Should we assert that s_cookieStorage is 0 here?

Good catch. adoptCF() calls CFRelease on its m_ptr so my patch would cause a leak. I think a conditional release before the if block should fix that.
Comment 4 Pratik Solanki 2011-02-21 14:54:56 PST
Created attachment 83219 [details]
Patch
Comment 5 Darin Adler 2011-02-21 15:03:19 PST
Comment on attachment 83219 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83219&action=review

> Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:61
> +    if (s_cookieStorage)
> +        CFRelease(s_cookieStorage);
> +
> +    CFRetain(cookieStorage);

The usual pattern is to put retain before release so things work properly if s_cookieStorage and cookieStorage happen to be the same object. You could do that here by just moving the CFRetain call up a couple lines.
Comment 6 Pratik Solanki 2011-02-21 16:15:13 PST
Commited r79261 - <http://trac.webkit.org/changeset/79261>