Bug 54905 – Remove global initializer in CookieStorageCFNet.cpp

• Component: Platform
• Status: RESOLVED FIXED

[Pratik Solanki]

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

[Pratik Solanki]

Created attachment 83212 [details]
Patch

First stab at fixing this error. Is there a better way to do this?

[Adam Barth]

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?

[Pratik Solanki]

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.

[Pratik Solanki]

Created attachment 83219 [details]
Patch

[Darin Adler]

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.

[Pratik Solanki]

Commited r79261 - <http://trac.webkit.org/changeset/79261>