Bug 100419 – String::createCFString should return a RetainPtr

• Component: New Bugs
• Status: RESOLVED FIXED

[Anders Carlsson]

String::createCFString should return a RetainPtr

[Anders Carlsson]

Created attachment 170736 [details]
Patch

[Build Bot]

Comment on attachment 170736 [details]
Patch

Attachment 170736 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14552746

[Build Bot]

Comment on attachment 170736 [details]
Patch

Attachment 170736 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14593382

[Anders Carlsson]

Created attachment 170764 [details]
Patch

[Build Bot]

Comment on attachment 170764 [details]
Patch

Attachment 170764 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14564763

[Darin Adler]

Comment on attachment 170764 [details]
Patch

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

Fantastic patch. One big mistake though. You’ll see it.

> Source/WTF/ChangeLog:16
> +        (AtomicString):

Please remove bogus lines like this even if the script generates them.

> Source/WTF/ChangeLog:18
> +        (WTF):

Ditto.

> Source/WTF/ChangeLog:19
> +        (StringImpl):

Ditto.

> Source/WTF/ChangeLog:21
> +        (String):

Ditto.

> Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:94
> +    if (RetainPtr<CFStringRef> cfURL = resource->url().string().createCFString())

Does this end up using the move constructor?

> Source/WebCore/platform/RuntimeApplicationChecks.cpp:49
> +    RetainPtr<CFStringRef> bundleIdentifierToCompare(AdoptCF, bundleIdentifierString.createCFString().get());

Oops!!!!!!! No!

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:380
> +    return RetainPtr<CFStringRef>(AdoptCF, UTTypeCreatePreferredIdentifierForTag(kUTTagClassMIMEType, mimeType.createCFString().get(), 0));

Can this call the adoptCF function instead? I noticed you did that a lot.

> Source/WebCore/platform/graphics/cg/ImageSourceCGMac.mm:37
> +    RetainPtr<CFStringRef> mime = adoptCF(UTTypeCopyPreferredTagWithClass(uti.createCFString().get(), kUTTagClassMIMEType));
>      return mime.get();

Can this not use a local variable at all?

> Source/WebCore/platform/graphics/cg/ImageSourceCGMac.mm:43
> +    RetainPtr<CFStringRef> extension = adoptCF(UTTypeCopyPreferredTagWithClass(uti.createCFString().get(), kUTTagClassFilenameExtension));
>      return extension.get();

Can this not use a local variable at all?

> Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm:285
> +                    runFontData = fontCache()->getCachedFontData(m_font.fontDescription(), String(fontName.get()), false, FontCache::DoNotRetain).get();

Not sure I understand why this change was needed.

> Source/WebCore/platform/mac/ClipboardMac.mm:102
> +    RetainPtr<CFStringRef> utiType(AdoptCF, UTTypeCreatePreferredIdentifierForTag(kUTTagClassMIMEType, mimeType.createCFString().get(), NULL));

Can this use the adoptCF function?

> Source/WebCore/platform/mac/SSLKeyGeneratorMac.cpp:66
> +    RetainPtr<CFStringRef> result(AdoptCF, wkSignedPublicKeyAndChallengeString(keySize, challengeString.createCFString().get(), keyDescription.get()));
>  
>      return result.get();

If you used the adoptCF function here then you would not need the “result” local variable any more.

> Source/WebCore/platform/network/cf/DNSCFNet.cpp:93
> +    RetainPtr<CFHostRef> host(AdoptCF, CFHostCreateWithName(0, hostname.createCFString().get()));

Maybe the adoptCF function here?

> Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp:210
> +            m_httpHeaderFields.set(String((CFStringRef)keys[i]), String((CFStringRef)values[i]));

Why is this now necessary?

> Source/WebCore/platform/win/SearchPopupMenuWin.cpp:55
>      String key = "com.apple.WebKit.searchField:" + name;
> -    return RetainPtr<CFStringRef>(AdoptCF, key.createCFString());
> +    return key.createCFString();

Probably don’t need a local variable any more here.

> Source/WebKit/cf/WebCoreSupport/WebInspectorClientCF.cpp:94
> -    RetainPtr<CFPropertyListRef> objectToStore;
> -    objectToStore.adoptCF(setting.createCFString());
> +    RetainPtr<CFPropertyListRef> objectToStore = setting.createCFString();
>      ASSERT(objectToStore);
>  
> -    RetainPtr<CFStringRef> preferencesKey(AdoptCF, createKeyForPreferences(key));
> +    RetainPtr<CFStringRef> preferencesKey = createKeyForPreferences(key);
>      CFPreferencesSetAppValue(preferencesKey.get(), objectToStore.get(), kCFPreferencesCurrentApplication);

Don’t really need the local variables here any more.

> Source/WebKit2/Platform/mac/ModuleMac.mm:33
> +    RetainPtr<CFURLRef> bundleURL(AdoptCF, CFURLCreateWithFileSystemPath(kCFAllocatorDefault, m_path.createCFString().get(), kCFURLPOSIXPathStyle, FALSE));

Could use the adoptCF function here.

> Source/WebKit2/Shared/Plugins/Netscape/mac/NetscapePluginModuleMac.mm:355
> +    RetainPtr<CFURLRef> bundleURL(AdoptCF, CFURLCreateWithFileSystemPath(kCFAllocatorDefault, pluginPath.createCFString().get(), kCFURLPOSIXPathStyle, false));

Could use the adoptCF function here.

> Source/WebKit2/Shared/Plugins/Netscape/mac/NetscapePluginModuleMac.mm:397
> +    RetainPtr<CFURLRef> bundleURL(AdoptCF, CFURLCreateWithFileSystemPath(kCFAllocatorDefault, pluginPath.createCFString().get(), kCFURLPOSIXPathStyle, false));

Could use the adoptCF function here.

[Anders Carlsson]

Created attachment 170779 [details]
Patch

[Anders Carlsson]

(In reply to comment #6)
> (From update of attachment 170764 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170764&action=review
> 
> Does this end up using the move constructor?

Yes.

> 
> > Source/WebCore/platform/RuntimeApplicationChecks.cpp:49
> > +    RetainPtr<CFStringRef> bundleIdentifierToCompare(AdoptCF, bundleIdentifierString.createCFString().get());
> 
> Oops!!!!!!! No!

Oops!!!!!!! Fixed!

> > Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm:285
> > +                    runFontData = fontCache()->getCachedFontData(m_font.fontDescription(), String(fontName.get()), false, FontCache::DoNotRetain).get();
> 
> Not sure I understand why this change was needed.

It's because I removed the AtomicString constructor that takes a CFStringRef. The rationale is in the ChangeLog.


> > Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp:210
> > +            m_httpHeaderFields.set(String((CFStringRef)keys[i]), String((CFStringRef)values[i]));
> 
> Why is this now necessary?

See my answer above.

[Build Bot]

Comment on attachment 170779 [details]
Patch

Attachment 170779 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14546863

[Anders Carlsson]

Created attachment 170798 [details]
Patch

[Benjamin Poulain]

Comment on attachment 170798 [details]
Patch

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

> Source/WTF/ChangeLog:13
> +        WTFString.h creates a naming conflict between JSC::Debugger and the Debugger function and there's currently
> +        no AtomicStringCF.cpp where we could safely include RetainPtr.h. Furthermore, these functions were only used
> +        twice throughout WebCore so just use the StringImpl and String functions instead.

Which functions use the AtomicString constructor? I had special plan for those two constructors, I may even have a radar for it.
I did not know there were not used a lot.

[Anders Carlsson]

Created attachment 170802 [details]
Patch

[Build Bot]

Comment on attachment 170802 [details]
Patch

Attachment 170802 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14555897

[Anders Carlsson]

Created attachment 171076 [details]
Patch

[Build Bot]

Comment on attachment 171076 [details]
Patch

Attachment 171076 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14607356

[Anders Carlsson]

Created attachment 171099 [details]
Patch

[Build Bot]

Comment on attachment 171099 [details]
Patch

Attachment 171099 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14611561

[Anders Carlsson]

Created attachment 171110 [details]
Patch

[Build Bot]

Comment on attachment 171110 [details]
Patch

Attachment 171110 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14616624

[Anders Carlsson]

Created attachment 171114 [details]
Patch

[Build Bot]

Comment on attachment 171114 [details]
Patch

Attachment 171114 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14611651

[Anders Carlsson]

Created attachment 171122 [details]
Patch

[Build Bot]

Comment on attachment 171122 [details]
Patch

Attachment 171122 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14617649

[Anders Carlsson]

Created attachment 171137 [details]
Patch

[Darin Adler]

Comment on attachment 171137 [details]
Patch

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

> Source/WTF/wtf/text/AtomicString.h:-141
> -    AtomicString(CFStringRef s) :  m_string(add(String(s).impl())) { }

Was anyone using this? If so, we should make sure they have an efficient version. This version is not efficient, because it allocates a StringImpl that is likely deallocated a moment later once it’s found in the atomic string table.

> Source/WTF/wtf/text/AtomicString.h:-142
> -    CFStringRef createCFString() const { return m_string.createCFString(); }

Removing this seems fine.

> Source/WTF/wtf/text/AtomicString.h:141
>      AtomicString(NSString* s) : m_string(add(String(s).impl())) { }

If we’re actually using this function, we should change it to not allocate a StringImpl that is likely deallocated a moment later once it’s found in the atomic string table.

> Source/WebCore/platform/cf/FileSystemCF.cpp:66
> +    return RetainPtr<CFURLRef>(AdoptCF, CFURLCreateWithFileSystemPath(0, path.createCFString().get(), pathStyle, FALSE));

adoptCF function here?

> Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm:285
> +                    runFontData = fontCache()->getCachedFontData(m_font.fontDescription(), String(fontName.get()), false, FontCache::DoNotRetain).get();

Why is the explicit String conversion needed here, again? Is this a place where we are creating an AtomicString? If so, then it does seem too bad to me that we are allocating and then destroying a StringImp for the font name each time.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:1004
> +    RetainPtr<CFStringRef> cfIdentifier = String(base + ".PrivateBrowsing").createCFString();

I don’t think the String is needed around (base + ".PrivateBrowsing").

> Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp:210
> +            m_httpHeaderFields.set(String((CFStringRef)keys[i]), String((CFStringRef)values[i]));

Same question about explicit String conversion as above. Is this for making an AtomicString? And if so, isn’t it a bit wasteful to make and destroy a StringImpl every time?

> Source/WebCore/platform/network/cf/ResourceResponseCFNet.cpp:106
> +                    m_httpHeaderFields.set(String(commonHeaderFields[i]), String(value));

Ditto.

> Source/WebCore/platform/network/cf/ResourceResponseCFNet.cpp:124
> +                m_httpHeaderFields.set(String((CFStringRef)keys[i]), String((CFStringRef)values[i]));

Ditto.

> Source/WebCore/platform/network/cf/SocketStreamHandleCFNet.cpp:390
> +    return String("WebKit socket stream, " + handle->m_url.string()).createCFString().leakRef();

I don’t think the String() is needed around the ("WebKit socket stream, " + handle->m_url.string()) expression here.

> Source/WebCore/platform/win/SearchPopupMenuWin.cpp:55
>      String key = "com.apple.WebKit.searchField:" + name;
> -    return RetainPtr<CFStringRef>(AdoptCF, key.createCFString());
> +    return key.createCFString();

Could be done all on one line.

> Source/WebKit2/UIProcess/cf/WebPageProxyCF.cpp:182
> +    return String("com.apple.WebKit.searchField:" + name).createCFString();

I don’t think the String is needed before ("com.apple.WebKit.searchField:" + name).

[Anders Carlsson]

(In reply to comment #25)
> (From update of attachment 171137 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171137&action=review
> 
> > Source/WTF/wtf/text/AtomicString.h:-141
> > -    AtomicString(CFStringRef s) :  m_string(add(String(s).impl())) { }
> 
> Was anyone using this? If so, we should make sure they have an efficient version. This version is not efficient, because it allocates a StringImpl that is likely deallocated a moment later once it’s found in the atomic string table.

Yes. I made it efficient in #100701, so I won't be removing it.

I> > Source/WTF/wtf/text/AtomicString.h:141
> >      AtomicString(NSString* s) : m_string(add(String(s).impl())) { }
> 
> If we’re actually using this function, we should change it to not allocate a StringImpl that is likely deallocated a moment later once it’s found in the atomic string table.

Ditto.

> Why is the explicit String conversion needed here, again? Is this a place where we are creating an AtomicString? If so, then it does seem too bad to me that we are allocating and then destroying a StringImp for the font name each time.
> 

Ditto.

> > Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:1004
> > +    RetainPtr<CFStringRef> cfIdentifier = String(base + ".PrivateBrowsing").createCFString();
> 
> I don’t think the String is needed around (base + ".PrivateBrowsing").

It is - otherwise I get:

error: no member named 'createCFString' in 'WTF::StringAppend<const char *, WTF::String>'

> > Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp:210
> > +            m_httpHeaderFields.set(String((CFStringRef)keys[i]), String((CFStringRef)values[i]));
> 
> Same question about explicit String conversion as above. Is this for making an AtomicString? And if so, isn’t it a bit wasteful to make and destroy a StringImpl every time?

I removed this now since AtomicString(CFStringRef) is efficient.

> 
> > Source/WebCore/platform/network/cf/ResourceResponseCFNet.cpp:106
> > +                    m_httpHeaderFields.set(String(commonHeaderFields[i]), String(value));
> 
> Ditto.
> 

Ditto.

> > Source/WebCore/platform/network/cf/ResourceResponseCFNet.cpp:124
> > +                m_httpHeaderFields.set(String((CFStringRef)keys[i]), String((CFStringRef)values[i]));
> 
> Ditto.

Ditto.

> 
> > Source/WebCore/platform/network/cf/SocketStreamHandleCFNet.cpp:390
> > +    return String("WebKit socket stream, " + handle->m_url.string()).createCFString().leakRef();
> 
> I don’t think the String() is needed around the ("WebKit socket stream, " + handle->m_url.string()) expression here.

Again, getting:

error: no member named 'createCFString' in 'WTF::StringAppend<const char *, WTF::String>'

> 
> > Source/WebKit2/UIProcess/cf/WebPageProxyCF.cpp:182
> > +    return String("com.apple.WebKit.searchField:" + name).createCFString();
> 
> I don’t think the String is needed before ("com.apple.WebKit.searchField:" + name).

Ditto.

[Anders Carlsson]

Created attachment 171344 [details]
Patch

[Anders Carlsson]

Created attachment 171347 [details]
Patch

[Anders Carlsson]

Created attachment 171348 [details]
Patch

[Anders Carlsson]

Created attachment 171349 [details]
Patch

[Anders Carlsson]

Created attachment 171350 [details]
Patch

[Anders Carlsson]

Committed r132916: <http://trac.webkit.org/changeset/132916>

[Tobias Netzel]

Created attachment 202820 [details]
don't adopt CFStringRefs twice

I'm currently running into problems with CFStrings that have apparently been adopted twice.
Please check if the proposed patch is correct, and if that's the case I'll prepare a new bug with a committable patch.

[Benjamin Poulain]

> I'm currently running into problems with CFStrings that have apparently been adopted twice.
> Please check if the proposed patch is correct, and if that's the case I'll prepare a new bug with a committable patch.

Looks reasonable.
Please file a separate bug and attach a WebKit patch to it.