67906 2011-09-11 14:13:34 -0700 Use of static RefPtr in FontPlatformData causes global destructor ordering problems 2011-09-20 15:38:49 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) PC Windows 7 RESOLVED FIXED P2 Normal --- 0 marshall webkit-unassigned abarth webkit.review.bot oldest_to_newest 465408 0 marshall 2011-09-11 14:13:34 -0700 From WebKit/Source/WebCore/platform/graphics/chromium/FontPlatformDataChromiumWin.cpp: FontPlatformData::RefCountedHFONT* FontPlatformData::hashTableDeletedFontValue() { static RefPtr<RefCountedHFONT> deletedValue = RefCountedHFONT::create(reinterpret_cast<HFONT>(-1)); return deletedValue.get(); } The problem with using static RefPtr is that, on Windows, the destructor of |deletedValue| will not be called until doexit() is executed on the main application thread. In single-process applications (like Chromium Embedded Framework) the main application thread may not be the same as the WebKit thread. This problem was exposed by WebKit revision 92254 which adds asserts to RefCounted to make sure ref/deref happens on the right thread. According to the WebKit experts the correct solution is to intentionally leak the static RefCountedHFONT object using leakRef(). 465410 1 107007 marshall 2011-09-11 14:18:54 -0700 Created attachment 107007 Proposed fix for bug 67906. Please review the attached bug_67906.patch. 465411 2 107008 marshall 2011-09-11 14:24:31 -0700 Created attachment 107008 Proposed fix for bug 67906. Please review the attached patch. 465466 3 107008 abarth 2011-09-11 20:38:48 -0700 Comment on attachment 107008 Proposed fix for bug 67906. This looks fine, but we should use DECLARE_STATIC_LOCAL though (and I would merge those too lines together). 465578 4 107045 marshall 2011-09-12 06:23:37 -0700 Created attachment 107045 Proposed Fix #3 Changed to use DEFINE_STATIC_LOCAL. 465583 5 marshall 2011-09-12 06:28:58 -0700 (In reply to comment #4) > Created an attachment (id=107045) [details] > Proposed Fix #3 > > Changed to use DEFINE_STATIC_LOCAL. Note: The above patch doesn't use leakRef() in combination with DEFINE_STATIC_LOCAL. This is because DEFINE_STATIC_LOCAL is already leaking a RefPtr that is holding a reference to the object, so I don't believe it's necessary to additionally call leakRef(). 470306 6 marshall 2011-09-20 13:50:25 -0700 (In reply to comment #3) > (From update of attachment 107008 [details]) > This looks fine, but we should use DECLARE_STATIC_LOCAL though (and I would merge those too lines together). Hi Adam, can you review the updated patch? Thanks. 470314 7 107045 abarth 2011-09-20 14:02:32 -0700 Comment on attachment 107045 Proposed Fix #3 Looks great. Sorry for dropping the ball. 470409 8 107045 webkit.review.bot 2011-09-20 15:38:43 -0700 Comment on attachment 107045 Proposed Fix #3 Clearing flags on attachment: 107045 Committed r95576: <http://trac.webkit.org/changeset/95576> 470411 9 webkit.review.bot 2011-09-20 15:38:49 -0700 All reviewed patches have been landed. Closing bug. 107007 2011-09-11 14:18:54 -0700 2011-09-11 14:24:31 -0700 Proposed fix for bug 67906. bug_67906.patch text/plain 1656 marshall SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nDQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09DQotLS0gV2ViQ29yZS9DaGFuZ2VM b2cJKHJldmlzaW9uIDk0ODU2KQ0KKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkp DQpAQCAtMSwzICsxLDEzIEBADQorMjAxMS0wOS0xMSAgTWFyc2hhbGwgR3JlZW5ibGF0dCAgPG1h cnNoYWxsQGNocm9taXVtLm9yZz4KKworICAgICAgICBVc2UgbGVha1JlZigpIHRvIGludGVudGlv bmFsbHkgbGVhayB0aGUgc3RhdGljIFJlZkNvdW50ZWRIRk9OVCBvYmplY3QgcmV0dXJuZWQgZnJv bQorICAgICAgICBGb250UGxhdGZvcm1EYXRhOjpoYXNoVGFibGVEZWxldGVkRm9udFZhbHVlKCkg dG8gYXZvaWQgZ2xvYmFsIGRlc3RydWN0b3Igb3JkZXJpbmcgcHJvYmxlbXMuCisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02NzkwNgorCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogcGxhdGZvcm0vZ3JhcGhpY3Mv Y2hyb21pdW0vRm9udFBsYXRmb3JtRGF0YUNocm9taXVtV2luLmNwcDoKKwogMjAxMS0wOS0wOSAg UGF2ZWwgUG9kaXZpbG92ICA8cG9kaXZpbG92QGNocm9taXVtLm9yZz4KIAogICAgICAgICBXZWIg SW5zcGVjdG9yOiBpbnRyb2R1Y2UgSmF2YVNjcmlwdFNvdXJjZUZyYW1lIGNsYXNzLgpJbmRleDog V2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9Gb250UGxhdGZvcm1EYXRhQ2hyb21p dW1XaW4uY3BwDQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09DQotLS0gV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9jaHJv bWl1bS9Gb250UGxhdGZvcm1EYXRhQ2hyb21pdW1XaW4uY3BwCShyZXZpc2lvbiA5NDg1NikNCisr KyBXZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2Nocm9taXVtL0ZvbnRQbGF0Zm9ybURhdGFDaHJv bWl1bVdpbi5jcHAJKHdvcmtpbmcgY29weSkNCkBAIC0zOCw2ICszOCw3IEBADQogCiAjaW5jbHVk ZSAiUGxhdGZvcm1TdXBwb3J0LmgiCiAjaW5jbHVkZSAiU2tpYUZvbnRXaW4uaCIKKyNpbmNsdWRl ICJTdGRMaWJFeHRyYXMuaCIKIAogbmFtZXNwYWNlIFdlYkNvcmUgewogCkBAIC0xMTYsOSArMTE3 LDkgQEANCiAKIEZvbnRQbGF0Zm9ybURhdGE6OlJlZkNvdW50ZWRIRk9OVCogRm9udFBsYXRmb3Jt RGF0YTo6aGFzaFRhYmxlRGVsZXRlZEZvbnRWYWx1ZSgpCiB7Ci0gICAgc3RhdGljIFJlZlB0cjxS ZWZDb3VudGVkSEZPTlQ+IGRlbGV0ZWRWYWx1ZSA9Ci0gICAgICAgIFJlZkNvdW50ZWRIRk9OVDo6 Y3JlYXRlKHJlaW50ZXJwcmV0X2Nhc3Q8SEZPTlQ+KC0xKSk7Ci0gICAgcmV0dXJuIGRlbGV0ZWRW YWx1ZS5nZXQoKTsKKyAgICBzdGF0aWMgUmVmQ291bnRlZEhGT05UKiBkZWxldGVkVmFsdWUgPQor ICAgICAgICBSZWZDb3VudGVkSEZPTlQ6OmNyZWF0ZShyZWludGVycHJldF9jYXN0PEhGT05UPigt MSkpLmxlYWtSZWYoKTsKKyAgICByZXR1cm4gZGVsZXRlZFZhbHVlOwogfQogCiBTQ1JJUFRfRk9O VFBST1BFUlRJRVMqIEZvbnRQbGF0Zm9ybURhdGE6OnNjcmlwdEZvbnRQcm9wZXJ0aWVzKCkgY29u c3QK 107008 2011-09-11 14:24:31 -0700 2011-09-12 06:23:37 -0700 Proposed fix for bug 67906. bug_67906b.patch text/plain 1527 marshall SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nDQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09DQotLS0gV2ViQ29yZS9DaGFuZ2VM b2cJKHJldmlzaW9uIDk0ODU2KQ0KKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkp DQpAQCAtMSwzICsxLDEzIEBADQorMjAxMS0wOS0xMSAgTWFyc2hhbGwgR3JlZW5ibGF0dCAgPG1h cnNoYWxsQGNocm9taXVtLm9yZz4KKworICAgICAgICBVc2UgbGVha1JlZigpIHRvIGludGVudGlv bmFsbHkgbGVhayB0aGUgc3RhdGljIFJlZkNvdW50ZWRIRk9OVCBvYmplY3QgcmV0dXJuZWQgZnJv bQorICAgICAgICBGb250UGxhdGZvcm1EYXRhOjpoYXNoVGFibGVEZWxldGVkRm9udFZhbHVlKCkg dG8gYXZvaWQgZ2xvYmFsIGRlc3RydWN0b3Igb3JkZXJpbmcgcHJvYmxlbXMuCisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02NzkwNgorCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogcGxhdGZvcm0vZ3JhcGhpY3Mv Y2hyb21pdW0vRm9udFBsYXRmb3JtRGF0YUNocm9taXVtV2luLmNwcDoKKwogMjAxMS0wOS0wOSAg UGF2ZWwgUG9kaXZpbG92ICA8cG9kaXZpbG92QGNocm9taXVtLm9yZz4KIAogICAgICAgICBXZWIg SW5zcGVjdG9yOiBpbnRyb2R1Y2UgSmF2YVNjcmlwdFNvdXJjZUZyYW1lIGNsYXNzLgpJbmRleDog V2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9Gb250UGxhdGZvcm1EYXRhQ2hyb21p dW1XaW4uY3BwDQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09DQotLS0gV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9jaHJv bWl1bS9Gb250UGxhdGZvcm1EYXRhQ2hyb21pdW1XaW4uY3BwCShyZXZpc2lvbiA5NDg1NikNCisr KyBXZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2Nocm9taXVtL0ZvbnRQbGF0Zm9ybURhdGFDaHJv bWl1bVdpbi5jcHAJKHdvcmtpbmcgY29weSkNCkBAIC0xMTYsOSArMTE2LDkgQEANCiAKIEZvbnRQ bGF0Zm9ybURhdGE6OlJlZkNvdW50ZWRIRk9OVCogRm9udFBsYXRmb3JtRGF0YTo6aGFzaFRhYmxl RGVsZXRlZEZvbnRWYWx1ZSgpCiB7Ci0gICAgc3RhdGljIFJlZlB0cjxSZWZDb3VudGVkSEZPTlQ+ IGRlbGV0ZWRWYWx1ZSA9Ci0gICAgICAgIFJlZkNvdW50ZWRIRk9OVDo6Y3JlYXRlKHJlaW50ZXJw cmV0X2Nhc3Q8SEZPTlQ+KC0xKSk7Ci0gICAgcmV0dXJuIGRlbGV0ZWRWYWx1ZS5nZXQoKTsKKyAg ICBzdGF0aWMgUmVmQ291bnRlZEhGT05UKiBkZWxldGVkVmFsdWUgPQorICAgICAgICBSZWZDb3Vu dGVkSEZPTlQ6OmNyZWF0ZShyZWludGVycHJldF9jYXN0PEhGT05UPigtMSkpLmxlYWtSZWYoKTsK KyAgICByZXR1cm4gZGVsZXRlZFZhbHVlOwogfQogCiBTQ1JJUFRfRk9OVFBST1BFUlRJRVMqIEZv bnRQbGF0Zm9ybURhdGE6OnNjcmlwdEZvbnRQcm9wZXJ0aWVzKCkgY29uc3QK 107045 2011-09-12 06:23:37 -0700 2011-09-20 15:38:43 -0700 Proposed Fix #3 bug_67906c.patch text/plain 1598 marshall SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nDQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09DQotLS0gV2ViQ29yZS9DaGFuZ2VM b2cJKHJldmlzaW9uIDk0ODU2KQ0KKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkp DQpAQCAtMSwzICsxLDEzIEBADQorMjAxMS0wOS0xMSAgTWFyc2hhbGwgR3JlZW5ibGF0dCAgPG1h cnNoYWxsQGNocm9taXVtLm9yZz4KKworICAgICAgICBVc2UgREVGSU5FX1NUQVRJQ19MT0NBTCB0 byBpbnRlbnRpb25hbGx5IGxlYWsgdGhlIHN0YXRpYyBSZWZDb3VudGVkSEZPTlQgb2JqZWN0IHJl dHVybmVkIGZyb20KKyAgICAgICAgRm9udFBsYXRmb3JtRGF0YTo6aGFzaFRhYmxlRGVsZXRlZEZv bnRWYWx1ZSgpIHRvIGF2b2lkIGdsb2JhbCBkZXN0cnVjdG9yIG9yZGVyaW5nIHByb2JsZW1zLgor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9Njc5MDYKKwor ICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHBsYXRmb3Jt L2dyYXBoaWNzL2Nocm9taXVtL0ZvbnRQbGF0Zm9ybURhdGFDaHJvbWl1bVdpbi5jcHA6CisKIDIw MTEtMDktMDkgIFBhdmVsIFBvZGl2aWxvdiAgPHBvZGl2aWxvdkBjaHJvbWl1bS5vcmc+CiAKICAg ICAgICAgV2ViIEluc3BlY3RvcjogaW50cm9kdWNlIEphdmFTY3JpcHRTb3VyY2VGcmFtZSBjbGFz cy4KSW5kZXg6IFdlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vRm9udFBsYXRmb3Jt RGF0YUNocm9taXVtV2luLmNwcA0KPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQ0KLS0tIFdlYkNvcmUvcGxhdGZvcm0vZ3Jh cGhpY3MvY2hyb21pdW0vRm9udFBsYXRmb3JtRGF0YUNocm9taXVtV2luLmNwcAkocmV2aXNpb24g OTQ4NTYpDQorKysgV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9Gb250UGxhdGZv cm1EYXRhQ2hyb21pdW1XaW4uY3BwCSh3b3JraW5nIGNvcHkpDQpAQCAtMzgsNiArMzgsNyBAQA0K IAogI2luY2x1ZGUgIlBsYXRmb3JtU3VwcG9ydC5oIgogI2luY2x1ZGUgIlNraWFGb250V2luLmgi CisjaW5jbHVkZSAiU3RkTGliRXh0cmFzLmgiCiAKIG5hbWVzcGFjZSBXZWJDb3JlIHsKIApAQCAt MTE2LDggKzExNyw4IEBADQogCiBGb250UGxhdGZvcm1EYXRhOjpSZWZDb3VudGVkSEZPTlQqIEZv bnRQbGF0Zm9ybURhdGE6Omhhc2hUYWJsZURlbGV0ZWRGb250VmFsdWUoKQogewotICAgIHN0YXRp YyBSZWZQdHI8UmVmQ291bnRlZEhGT05UPiBkZWxldGVkVmFsdWUgPQotICAgICAgICBSZWZDb3Vu dGVkSEZPTlQ6OmNyZWF0ZShyZWludGVycHJldF9jYXN0PEhGT05UPigtMSkpOworICAgIERFRklO RV9TVEFUSUNfTE9DQUwoUmVmUHRyPFJlZkNvdW50ZWRIRk9OVD4sIGRlbGV0ZWRWYWx1ZSwKKyAg ICAgICAgICAgICAgICAgICAgICAgIChSZWZDb3VudGVkSEZPTlQ6OmNyZWF0ZShyZWludGVycHJl dF9jYXN0PEhGT05UPigtMSkpKSk7CiAgICAgcmV0dXJuIGRlbGV0ZWRWYWx1ZS5nZXQoKTsKIH0K IAo=