Bug 118001 - [Qt] REGRESSION(r132916): Mac build broken by createCFString changes
Summary: [Qt] REGRESSION(r132916): Mac build broken by createCFString changes
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-25 13:13 PDT by Tobias Netzel
Modified: 2013-09-13 08:12 PDT (History)
4 users (show)

See Also:


Attachments
don't adopt CFStrings twice (3.24 KB, patch)
2013-06-25 13:35 PDT, Tobias Netzel
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tobias Netzel 2013-06-25 13:13:02 PDT
r132916 changed createCFString() to return a RetainPtr to a CFString instead of a CFString itself.
In that patch there are missing a few cases where adoptCF() is still called on the return value of createCFString(), leading to crashes here where that String is then collected by the ObjC garbage collector, although it still needs to be accessed.
Comment 1 Tobias Netzel 2013-06-25 13:35:35 PDT
Created attachment 205420 [details]
don't adopt CFStrings twice
Comment 2 Alexey Proskuryakov 2013-06-25 13:55:54 PDT
I don't understand how this code can compile. Does any platform actually build it?

HyphenationMac.mm is 10.6 only, and I don't think that we support building WebKit with 10.6. This file should be just deleted.

PluginPackageMac.cpp is referenced from WebCore.pri, but then again, I'm not sure how it compiles.
Comment 3 Tobias Netzel 2013-06-25 14:09:27 PDT
Bug 102057 is referring PluginPackageMac.cpp.

Originally I had found the bug in recently removed Carbon specific code in PluginViewMac.mm and the other occurences I found by running grep on the entire source tree.
Comment 4 Alexey Proskuryakov 2013-06-25 14:12:46 PDT
OK, so it's not "adopted twice", but a compilation failure?
Comment 5 Tobias Netzel 2013-06-25 14:23:38 PDT
First adoption in Source/WebCore/platform/text/cf/StringImplCF.cpp:
RetainPtr<CFStringRef> StringImpl::createCFString()
{
    ...
    return adoptCF(string);
}


Second ones here:
    WTF::RetainPtr<CFStringRef> homeDir = adoptCF(homeDirectoryPath().createCFString());
and here:
    WTF::RetainPtr<CFStringRef> path = adoptCF(m_path.createCFString());
Comment 6 Anders Carlsson 2013-06-25 14:26:28 PDT
(In reply to comment #5)
> First adoption in Source/WebCore/platform/text/cf/StringImplCF.cpp:
> RetainPtr<CFStringRef> StringImpl::createCFString()
> {
>     ...
>     return adoptCF(string);
> }
> 
> 
> Second ones here:
>     WTF::RetainPtr<CFStringRef> homeDir = adoptCF(homeDirectoryPath().createCFString());
> and here:
>     WTF::RetainPtr<CFStringRef> path = adoptCF(m_path.createCFString());

adoptCF does not take a RetainPtr, so the code above would not compile.
Comment 7 Tobias Netzel 2013-06-25 14:40:52 PDT
Indeed I previously had changed the concerning lines to adopt the result of createCFString().get() instead of just createCFString() in order to get the code compiled (but that led to the described crashes).

I guess that means I've found rotten code that has to be eliminated.
Comment 8 Anders Carlsson 2013-07-22 07:55:00 PDT
This patch looks good, but if the code is unused we should just delete it. Is it still used?
Comment 9 Anders Carlsson 2013-09-13 08:12:33 PDT
HyphenationMac.mm has been removed and we haven't seen any build errors from PluginPackageMac so it's clear that code isn't built (we should consider removing it).

I'm going to close this bug.