118001 2013-06-25 13:13:02 -0700 [Qt] REGRESSION(r132916): Mac build broken by createCFString changes 2013-09-13 08:12:33 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Mac Unspecified RESOLVED INVALID P2 Normal --- 0 tobias.netzel webkit-unassigned andersca ap benjamin ossy oldest_to_newest 903416 0 tobias.netzel 2013-06-25 13:13:02 -0700 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. 903418 1 205420 tobias.netzel 2013-06-25 13:35:35 -0700 Created attachment 205420 don't adopt CFStrings twice 903422 2 ap 2013-06-25 13:55:54 -0700 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. 903424 3 tobias.netzel 2013-06-25 14:09:27 -0700 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. 903425 4 ap 2013-06-25 14:12:46 -0700 OK, so it's not "adopted twice", but a compilation failure? 903430 5 tobias.netzel 2013-06-25 14:23:38 -0700 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()); 903431 6 andersca 2013-06-25 14:26:28 -0700 (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. 903433 7 tobias.netzel 2013-06-25 14:40:52 -0700 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. 910675 8 andersca 2013-07-22 07:55:00 -0700 This patch looks good, but if the code is unused we should just delete it. Is it still used? 928846 9 andersca 2013-09-13 08:12:33 -0700 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. 205420 2013-06-25 13:35:35 -0700 2013-09-13 08:11:50 -0700 don't adopt CFStrings twice createCFString.patch text/plain 3315 tobias.netzel SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE1MTk3MCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE5IEBACisyMDEzLTA2LTI1ICBUb2JpYXMg TmV0emVsICA8dG9iaWFzLm5ldHplbEBnbWFpbC5jb20+CisKKyAgICAgICAgUkVHUkVTU0lPTjog cjEzMjkxNiBDRlN0cmluZ3MgYXJlIGFkb3B0ZWQgdHdpY2UKKyAgICAgICAgaHR0cHM6Ly9idWdz LndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTExODAwMQorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIHIxMzI5MTYgd2FzIG1pc3Npbmcgc29tZSBwbGFj ZXMgd2hlcmUgYWRvcHRDRigpIGlzIGNhbGxlZCBvbiB0aGUgcmVzdWx0IGZyb20gY3JlYXRlQ0ZT dHJpbmcoKSwKKyAgICAgICAgd2hpY2ggYWxyZWFkeSBpcyBhIFJldGFpblB0ciB0byBhIENGU3Ry aW5nLgorCisgICAgICAgICogcGxhdGZvcm0vdGV4dC9tYWMvSHlwaGVuYXRpb25NYWMubW06Cisg ICAgICAgIChXZWJDb3JlOjo6OmNyZWF0ZVZhbHVlRm9yS2V5KToKKyAgICAgICAgKiBwbHVnaW5z L21hYy9QbHVnaW5QYWNrYWdlTWFjLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlBsdWdpblBhY2th Z2U6OmZldGNoSW5mbyk6CisgICAgICAgIChXZWJDb3JlOjpQbHVnaW5QYWNrYWdlOjpsb2FkKToK KwogMjAxMy0wNi0yNSAgQWxleCBDaHJpc3RlbnNlbiAgPGFjaHJpc3RlbnNlbkBhcHBsZS5jb20+ CiAKICAgICAgICAgQWRkZWQgUExBVEZPUk0oV0lOKSB0byBtYW55IHBsYWNlcyB3aGVyZSBpdCB3 YXMgbWlzc2luZyBmb3IgY29tcGlsaW5nIFdlYkdMIGZvciBXaW5kb3dzLgpJbmRleDogU291cmNl L1dlYkNvcmUvcGxhdGZvcm0vdGV4dC9tYWMvSHlwaGVuYXRpb25NYWMubW0KPT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQot LS0gU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vdGV4dC9tYWMvSHlwaGVuYXRpb25NYWMubW0JKHJl dmlzaW9uIDE1MTcxOCkKKysrIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL3RleHQvbWFjL0h5cGhl bmF0aW9uTWFjLm1tCSh3b3JraW5nIGNvcHkpCkBAIC00NCw3ICs0NCw3IEBAIGJvb2wgQXRvbWlj U3RyaW5nS2V5ZWRNUlVDYWNoZTxib29sPjo6Y3IKIHRlbXBsYXRlPD4KIGJvb2wgQXRvbWljU3Ry aW5nS2V5ZWRNUlVDYWNoZTxib29sPjo6Y3JlYXRlVmFsdWVGb3JLZXkoY29uc3QgQXRvbWljU3Ry aW5nJiBsb2NhbGVJZGVudGlmaWVyKQogewotICAgIFJldGFpblB0cjxDRlN0cmluZ1JlZj4gY2ZM b2NhbGVJZGVudGlmaWVyID0gYWRvcHRDRihsb2NhbGVJZGVudGlmaWVyLmNyZWF0ZUNGU3RyaW5n KCkpOworICAgIFJldGFpblB0cjxDRlN0cmluZ1JlZj4gY2ZMb2NhbGVJZGVudGlmaWVyID0gbG9j YWxlSWRlbnRpZmllci5zdHJpbmcoKS5jcmVhdGVDRlN0cmluZygpOwogICAgIFJldGFpblB0cjxD RkRpY3Rpb25hcnlSZWY+IGNvbXBvbmVudHMgPSBhZG9wdENGKENGTG9jYWxlQ3JlYXRlQ29tcG9u ZW50c0Zyb21Mb2NhbGVJZGVudGlmaWVyKGtDRkFsbG9jYXRvckRlZmF1bHQsIGNmTG9jYWxlSWRl bnRpZmllci5nZXQoKSkpOwogICAgIENGU3RyaW5nUmVmIGxhbmd1YWdlID0gcmVpbnRlcnByZXRf Y2FzdDxDRlN0cmluZ1JlZj4oQ0ZEaWN0aW9uYXJ5R2V0VmFsdWUoY29tcG9uZW50cy5nZXQoKSwg a0NGTG9jYWxlTGFuZ3VhZ2VDb2RlKSk7CiAgICAgc3RhdGljIENGU3RyaW5nUmVmIGVuZ2xpc2hM YW5ndWFnZSA9IENGU1RSKCJlbiIpOwpJbmRleDogU291cmNlL1dlYkNvcmUvcGx1Z2lucy9tYWMv UGx1Z2luUGFja2FnZU1hYy5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvcGx1Z2lu cy9tYWMvUGx1Z2luUGFja2FnZU1hYy5jcHAJKHJldmlzaW9uIDE1MTcxOCkKKysrIFNvdXJjZS9X ZWJDb3JlL3BsdWdpbnMvbWFjL1BsdWdpblBhY2thZ2VNYWMuY3BwCSh3b3JraW5nIGNvcHkpCkBA IC0xMzgsNyArMTM4LDcgQEAgYm9vbCBQbHVnaW5QYWNrYWdlOjpmZXRjaEluZm8oKQogICAgIGlm IChtaW1lVHlwZXNGaWxlTmFtZSAmJiBDRkdldFR5cGVJRChtaW1lVHlwZXNGaWxlTmFtZS5nZXQo KSkgPT0gQ0ZTdHJpbmdHZXRUeXBlSUQoKSkgewogCiAgICAgICAgIFdURjo6UmV0YWluUHRyPENG U3RyaW5nUmVmPiBmaWxlTmFtZSA9IChDRlN0cmluZ1JlZiltaW1lVHlwZXNGaWxlTmFtZS5nZXQo KTsKLSAgICAgICAgV1RGOjpSZXRhaW5QdHI8Q0ZTdHJpbmdSZWY+IGhvbWVEaXIgPSBhZG9wdENG KGhvbWVEaXJlY3RvcnlQYXRoKCkuY3JlYXRlQ0ZTdHJpbmcoKSk7CisgICAgICAgIFdURjo6UmV0 YWluUHRyPENGU3RyaW5nUmVmPiBob21lRGlyID0gaG9tZURpcmVjdG9yeVBhdGgoKS5jcmVhdGVD RlN0cmluZygpOwogICAgICAgICBXVEY6OlJldGFpblB0cjxDRlN0cmluZ1JlZj4gcGF0aCA9IGFk b3B0Q0YoQ0ZTdHJpbmdDcmVhdGVXaXRoRm9ybWF0KDAsIDAsIENGU1RSKCIlQC9MaWJyYXJ5L1By ZWZlcmVuY2VzLyVAIiksIGhvbWVEaXIuZ2V0KCksIGZpbGVOYW1lLmdldCgpKSk7CiAKICAgICAg ICAgV1RGOjpSZXRhaW5QdHI8Q0ZEaWN0aW9uYXJ5UmVmPiBwbGlzdCA9IHJlYWRQTGlzdEZpbGUo cGF0aC5nZXQoKSwgLypjcmVhdGVGaWxlKi8gZmFsc2UsIG1fbW9kdWxlKTsKQEAgLTI1NSw3ICsy NTUsNyBAQCBib29sIFBsdWdpblBhY2thZ2U6OmxvYWQoKQogICAgICAgICByZXR1cm4gdHJ1ZTsK ICAgICB9CiAKLSAgICBXVEY6OlJldGFpblB0cjxDRlN0cmluZ1JlZj4gcGF0aCA9IGFkb3B0Q0Yo bV9wYXRoLmNyZWF0ZUNGU3RyaW5nKCkpOworICAgIFdURjo6UmV0YWluUHRyPENGU3RyaW5nUmVm PiBwYXRoID0gbV9wYXRoLmNyZWF0ZUNGU3RyaW5nKCk7CiAgICAgV1RGOjpSZXRhaW5QdHI8Q0ZV UkxSZWY+IHVybCA9IGFkb3B0Q0YoQ0ZVUkxDcmVhdGVXaXRoRmlsZVN5c3RlbVBhdGgoa0NGQWxs b2NhdG9yRGVmYXVsdCwgcGF0aC5nZXQoKSwKICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGtDRlVSTFBPU0lYUGF0 aFN0eWxlLCBmYWxzZSkpOwogICAgIG1fbW9kdWxlID0gQ0ZCdW5kbGVDcmVhdGUoTlVMTCwgdXJs LmdldCgpKTsK