Bug 118001

Summary:

[Qt] REGRESSION(r132916): Mac build broken by createCFString changes

Product:

WebKit

Reporter:

Tobias Netzel <tobias.netzel>

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED INVALID

Severity:

Normal

CC:

andersca, ap, benjamin, ossy

Priority:

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

Unspecified

Attachments:

Description	Flags
don't adopt CFStrings twice	none

Tobias Netzel

Reported 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.

Attachments
don't adopt CFStrings twice (3.24 KB, patch) 2013-06-25 13:35 PDT, Tobias Netzel	no flags	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Tobias Netzel

Comment 1 2013-06-25 13:35:35 PDT

Created attachment 205420 [details] don't adopt CFStrings twice

Alexey Proskuryakov

Comment 2 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.

Tobias Netzel

Comment 3 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.

Alexey Proskuryakov

Comment 4 2013-06-25 14:12:46 PDT

OK, so it's not "adopted twice", but a compilation failure?

Tobias Netzel

Comment 5 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());

Anders Carlsson

Comment 6 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.

Tobias Netzel

Comment 7 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.

Anders Carlsson

Comment 8 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?

Anders Carlsson

Comment 9 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.

Note You need to log in before you can comment on or make changes to this bug.