25760 2009-05-13 14:09:00 -0700 Font leak in Chromium's Skia backend. 2009-05-15 00:02:43 -0700 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 1 evan eric agl brettw oldest_to_newest 121095 0 evan 2009-05-13 14:09:00 -0700 Valgrind found this: http://code.google.com/p/chromium/issues/detail?id=9475 121097 1 30285 evan 2009-05-13 14:13:14 -0700 Created attachment 30285 fix some stuff WebCore/ChangeLog | 11 +++++++++++ .../platform/graphics/chromium/FontCacheLinux.cpp | 5 ++--- 2 files changed, 13 insertions(+), 3 deletions(-) 121098 2 evan 2009-05-13 14:14:51 -0700 Please note that I don't really know what I'm doing, so review carefully. 121102 3 evan 2009-05-13 14:28:16 -0700 I apologize for not quite getting the review process right, but from some offline discussion it appears Brett is the right reviewer. Brett, agl wrote the code but he'd never seen these getCachedFont* bits. As our font man, what do you think? 121117 4 30285 brettw 2009-05-13 15:24:05 -0700 Comment on attachment 30285 fix some stuff I believe this is the way it's supposed to work, it's what we do on Windows. > - FontPlatformData* fpd = > - createFontPlatformData(font.fontDescription(), AtomicString((char*) family)); > - ret = new SimpleFontData(*fpd); > + FontPlatformData* fpd = getCachedFontPlatformData(font.fontDescription(), AtomicString((char*) family), false); > + ret = getCachedFontData(fpd); Is it WebKit style to require static_cast? Brett 121127 5 30285 fishd 2009-05-13 15:54:20 -0700 Comment on attachment 30285 fix some stuff > +2009-05-13 Evan Martin <evan@chromium.org> > + > + Reviewed by Darin Fisher <darin@chromium.org>. > + > + Fix a font-related leak in Chromium's Skia backend found by Valgrind. > + http://code.google.com/p/chromium/issues/detail?id=9475 Please add a reference to the bugs.webkit.org bug instead. In that bug you should/may refer to the chromium bug. yes, static_cast for the win! 121131 6 evan 2009-05-13 16:04:06 -0700 static_cast where? 121134 7 30294 evan 2009-05-13 16:13:47 -0700 Created attachment 30294 fix WebCore/ChangeLog | 11 +++++++++++ .../platform/graphics/chromium/FontCacheLinux.cpp | 5 ++--- 2 files changed, 13 insertions(+), 3 deletions(-) 121136 8 30294 eric 2009-05-13 16:25:01 -0700 Comment on attachment 30294 fix WebKit style guidelines: "Use full words, except in the rare case where an abbreviation would be more canonical and easier to understand." I would re-write this block: + FontPlatformData* fpd = getCachedFontPlatformData(font.fontDescription(), AtomicString(reinterpret_cast<char*>(family)), false); + ret = getCachedFontData(fpd); as: AtomicString fontFamily(reinterpret_cast<char*>(family)); ret = getCachedFontData(getCachedFontPlatformData(font.fontDescription(), fontFamily), false) or as platformData = Otherwise looks fine. I can fix it an land for you. 121140 9 eric 2009-05-13 16:30:01 -0700 review collision it would appear. Stupid bugzilla. 121148 10 evan 2009-05-13 16:42:28 -0700 Please feel free to munge at will. (In my defense, note that both the C-style cast and the "ret" were there before my change...) 121448 11 eric 2009-05-15 00:02:43 -0700 Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/platform/graphics/chromium/FontCacheLinux.cpp Committed r43756 30285 2009-05-13 14:13:14 -0700 2009-05-13 16:24:35 -0700 fix some stuff fix-some-stuff.patch text/plain 1593 evan MTYzYWZjNTA0YzdhNTEzNjczODA5MDY3OThlMTczNWVlNzNjZjcyNwpkaWZmIC0tZ2l0IGEvV2Vi Q29yZS9DaGFuZ2VMb2cgYi9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCA2MzFlOTAzLi5mNTMwNDhm IDEwMDY0NAotLS0gYS9XZWJDb3JlL0NoYW5nZUxvZworKysgYi9XZWJDb3JlL0NoYW5nZUxvZwpA QCAtMSwzICsxLDE0IEBACisyMDA5LTA1LTEzICBFdmFuIE1hcnRpbiAgPGV2YW5AY2hyb21pdW0u b3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5IERhcmluIEZpc2hlciA8ZGFyaW5AY2hyb21pdW0u b3JnPi4KKworICAgICAgICBGaXggYSBmb250LXJlbGF0ZWQgbGVhayBpbiBDaHJvbWl1bSdzIFNr aWEgYmFja2VuZCBmb3VuZCBieSBWYWxncmluZC4KKyAgICAgICAgaHR0cDovL2NvZGUuZ29vZ2xl LmNvbS9wL2Nocm9taXVtL2lzc3Vlcy9kZXRhaWw/aWQ9OTQ3NQorCisgICAgICAgICogcGxhdGZv cm0vZ3JhcGhpY3MvY2hyb21pdW0vRm9udENhY2hlTGludXguY3BwOgorICAgICAgICAoV2ViQ29y ZTo6Rm9udENhY2hlOjpnZXRGb250RGF0YUZvckNoYXJhY3RlcnMpOgorICAgICAgICBVc2UgY2Fj aGVzIGluc3RlYWQgb2YgIm5ldyIgb24gZXZlcnkgY2FsbC4KKwogMjAwOS0wNS0xMyAgRGF2aWQg SHlhdHQgIDxoeWF0dEBhcHBsZS5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgQmV0aCBEYWtp biBhbmQgRGFyaW4gQWRsZXIuCmRpZmYgLS1naXQgYS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNz L2Nocm9taXVtL0ZvbnRDYWNoZUxpbnV4LmNwcCBiL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3Mv Y2hyb21pdW0vRm9udENhY2hlTGludXguY3BwCmluZGV4IDQxODJkNDcuLmRiMGM4NWUgMTAwNjQ0 Ci0tLSBhL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vRm9udENhY2hlTGludXgu Y3BwCisrKyBiL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vRm9udENhY2hlTGlu dXguY3BwCkBAIC03OSw5ICs3OSw4IEBAIGNvbnN0IFNpbXBsZUZvbnREYXRhKiBGb250Q2FjaGU6 OmdldEZvbnREYXRhRm9yQ2hhcmFjdGVycyhjb25zdCBGb250JiBmb250LAogICAgIGlmIChtYXRj aCkgewogICAgICAgICBGY0NoYXI4KiBmYW1pbHk7CiAgICAgICAgIGlmIChGY1BhdHRlcm5HZXRT dHJpbmcobWF0Y2gsIEZDX0ZBTUlMWSwgMCwgJmZhbWlseSkgPT0gRmNSZXN1bHRNYXRjaCkgewot ICAgICAgICAgICAgRm9udFBsYXRmb3JtRGF0YSogZnBkID0KLSAgICAgICAgICAgICAgICBjcmVh dGVGb250UGxhdGZvcm1EYXRhKGZvbnQuZm9udERlc2NyaXB0aW9uKCksIEF0b21pY1N0cmluZygo Y2hhciopIGZhbWlseSkpOwotICAgICAgICAgICAgcmV0ID0gbmV3IFNpbXBsZUZvbnREYXRhKCpm cGQpOworICAgICAgICAgICAgRm9udFBsYXRmb3JtRGF0YSogZnBkID0gZ2V0Q2FjaGVkRm9udFBs YXRmb3JtRGF0YShmb250LmZvbnREZXNjcmlwdGlvbigpLCBBdG9taWNTdHJpbmcoKGNoYXIqKSBm YW1pbHkpLCBmYWxzZSk7CisgICAgICAgICAgICByZXQgPSBnZXRDYWNoZWRGb250RGF0YShmcGQp OwogICAgICAgICB9CiAgICAgICAgIEZjUGF0dGVybkRlc3Ryb3kobWF0Y2gpOwogICAgIH0K 30294 2009-05-13 16:13:47 -0700 2009-05-13 16:24:53 -0700 fix fix.patch text/plain 1581 evan ZTE2ZDQ3NzliMmQ0NTcwNzdmMGVmNzhiMjdlZmMwYzQxYTk3NjY1MwpkaWZmIC0tZ2l0IGEvV2Vi Q29yZS9DaGFuZ2VMb2cgYi9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCA2MzFlOTAzLi5mYTFjNjhk IDEwMDY0NAotLS0gYS9XZWJDb3JlL0NoYW5nZUxvZworKysgYi9XZWJDb3JlL0NoYW5nZUxvZwpA QCAtMSwzICsxLDE0IEBACisyMDA5LTA1LTEzICBFdmFuIE1hcnRpbiAgPGV2YW5AY2hyb21pdW0u b3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEZp eCBhIGZvbnQtcmVsYXRlZCBsZWFrIGluIENocm9taXVtJ3MgU2tpYSBiYWNrZW5kIGZvdW5kIGJ5 IFZhbGdyaW5kLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9MjU3NjAKKworICAgICAgICAqIHBsYXRmb3JtL2dyYXBoaWNzL2Nocm9taXVtL0ZvbnRDYWNo ZUxpbnV4LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkZvbnRDYWNoZTo6Z2V0Rm9udERhdGFGb3JD aGFyYWN0ZXJzKToKKyAgICAgICAgVXNlIGNhY2hlcyBpbnN0ZWFkIG9mICJuZXciIG9uIGV2ZXJ5 IGNhbGwuCisKIDIwMDktMDUtMTMgIERhdmlkIEh5YXR0ICA8aHlhdHRAYXBwbGUuY29tPgogCiAg ICAgICAgIFJldmlld2VkIGJ5IEJldGggRGFraW4gYW5kIERhcmluIEFkbGVyLgpkaWZmIC0tZ2l0 IGEvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9Gb250Q2FjaGVMaW51eC5jcHAg Yi9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2Nocm9taXVtL0ZvbnRDYWNoZUxpbnV4LmNwcApp bmRleCA0MTgyZDQ3Li4yZWE0NThmIDEwMDY0NAotLS0gYS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBo aWNzL2Nocm9taXVtL0ZvbnRDYWNoZUxpbnV4LmNwcAorKysgYi9XZWJDb3JlL3BsYXRmb3JtL2dy YXBoaWNzL2Nocm9taXVtL0ZvbnRDYWNoZUxpbnV4LmNwcApAQCAtNzksOSArNzksOCBAQCBjb25z dCBTaW1wbGVGb250RGF0YSogRm9udENhY2hlOjpnZXRGb250RGF0YUZvckNoYXJhY3RlcnMoY29u c3QgRm9udCYgZm9udCwKICAgICBpZiAobWF0Y2gpIHsKICAgICAgICAgRmNDaGFyOCogZmFtaWx5 OwogICAgICAgICBpZiAoRmNQYXR0ZXJuR2V0U3RyaW5nKG1hdGNoLCBGQ19GQU1JTFksIDAsICZm YW1pbHkpID09IEZjUmVzdWx0TWF0Y2gpIHsKLSAgICAgICAgICAgIEZvbnRQbGF0Zm9ybURhdGEq IGZwZCA9Ci0gICAgICAgICAgICAgICAgY3JlYXRlRm9udFBsYXRmb3JtRGF0YShmb250LmZvbnRE ZXNjcmlwdGlvbigpLCBBdG9taWNTdHJpbmcoKGNoYXIqKSBmYW1pbHkpKTsKLSAgICAgICAgICAg IHJldCA9IG5ldyBTaW1wbGVGb250RGF0YSgqZnBkKTsKKyAgICAgICAgICAgIEZvbnRQbGF0Zm9y bURhdGEqIGZwZCA9IGdldENhY2hlZEZvbnRQbGF0Zm9ybURhdGEoZm9udC5mb250RGVzY3JpcHRp b24oKSwgQXRvbWljU3RyaW5nKHJlaW50ZXJwcmV0X2Nhc3Q8Y2hhcio+KGZhbWlseSkpLCBmYWxz ZSk7CisgICAgICAgICAgICByZXQgPSBnZXRDYWNoZWRGb250RGF0YShmcGQpOwogICAgICAgICB9 CiAgICAgICAgIEZjUGF0dGVybkRlc3Ryb3kobWF0Y2gpOwogICAgIH0K