67815 2011-09-08 16:09:55 -0700 [Chromium] Fix leak of Skia stream with custom CSS fonts 2011-09-09 11:30:08 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 simonjam simonjam abarth reed tony webkit.review.bot oldest_to_newest 464468 0 simonjam 2011-09-08 16:09:55 -0700 [Chromium] Fix leak of Skia stream with custom CSS fonts 464470 1 106809 simonjam 2011-09-08 16:11:06 -0700 Created attachment 106809 Patch 464478 2 simonjam 2011-09-08 16:19:36 -0700 FYI, the underlying SKIA function is here: http://google.com/codesearch#OAMlx_jo-ck/src/skia/ext/SkFontHost_fontconfig.cpp&l=166 It clearly doesn't take ownership of the stream, so we have to delete it. 464489 3 tony 2011-09-08 16:38:00 -0700 Do we use SkFontHost_fontconfig on mac? Also, it looks like in the original version upstream, the stream is owned by the SkTypeface: http://codesearch.google.com/#OAMlx_jo-ck/src/third_party/skia/src/ports/SkFontHost_linux.cpp&exact_package=chromium&q=file:.*skfonthost.*linux.*&type=cs&l=579 Maybe we need to fix src/skia/ext/SkFontHost_fontconfig.cpp to delete it? 464492 4 106809 abarth 2011-09-08 16:41:44 -0700 Comment on attachment 106809 Patch Oops. Didn't see Tony's comment. 464501 5 simonjam 2011-09-08 16:58:30 -0700 (In reply to comment #3) > Do we use SkFontHost_fontconfig on mac? I didn't look. The original bug said Mac had the same leak. I assumed it was implemented the same way. Turns out the two Mac implementations don't do anything with the stream, so that's why they leak it. > Also, it looks like in the original version upstream, the stream is owned by the SkTypeface: > http://codesearch.google.com/#OAMlx_jo-ck/src/third_party/skia/src/ports/SkFontHost_linux.cpp&exact_package=chromium&q=file:.*skfonthost.*linux.*&type=cs&l=579 > > Maybe we need to fix src/skia/ext/SkFontHost_fontconfig.cpp to delete it? Wow, this is really inconsistent. It looks like the _linux one uses ref counting. We can do the same thing here and then not have to worry about how each platform deals with it. New patch coming. 464502 6 106814 simonjam 2011-09-08 16:59:24 -0700 Created attachment 106814 Patch 464505 7 106814 abarth 2011-09-08 17:07:03 -0700 Comment on attachment 106814 Patch Please don't call unref directly. Maybe you should use a RefPtr and adoptRef? 464506 8 106814 abarth 2011-09-08 17:07:50 -0700 Comment on attachment 106814 Patch If RemoteFontStream is RefCounted, its constructor should be private and it should expose a ::create method that calls adoptRef. 464511 9 simonjam 2011-09-08 17:12:22 -0700 (In reply to comment #7) > (From update of attachment 106814 [details]) > Please don't call unref directly. Maybe you should use a RefPtr and adoptRef? This is a SkRefCnt object. I'm honestly not familiar at all with Skia. Is there a RefPtr equivalent for them? 464516 10 abarth 2011-09-08 17:17:00 -0700 > This is a SkRefCnt object. I'm honestly not familiar at all with Skia. Is there a RefPtr equivalent for them? I'm not sure about Skia, but we've created specialized RefPtr-like classes to handle these cases in the past. For example, we have one that understands how to deal with CoreGraphics ref-counted pointers. 464517 11 abarth 2011-09-08 17:18:41 -0700 http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/wtf/RetainPtr.h 464518 12 abarth 2011-09-08 17:19:03 -0700 We probably need something similar to that for Skia if Skia doesn't have one already for us to use. 464539 13 simonjam 2011-09-08 17:54:18 -0700 (In reply to comment #12) > We probably need something similar to that for Skia if Skia doesn't have one already for us to use. They have one, but it's broken. Both SkRefCnt's constructor and SkRefPtr's constructor call ref() on the object, so you immediately have a ref count of 2 and must manually unref() once before SkRefPtr works as expected. See this in action here: http://skia.googlecode.com/svn-history/r1812/trunk/tests/PDFPrimitivesTest.cpp ... SkRefPtr<SkPDFInt> int42 = new SkPDFInt(42); int42->unref(); // SkRefPtr and new both took a reference. ... That's probably why we don't use SkRefPtr at all in WebKit. Instead, it looks like people just call unref() directly (there are 6 other references in this directory). I vote for just following them and moving on. We can file a separate bug to fix all of the callers of Skia's unref(). 464547 14 abarth 2011-09-08 18:04:59 -0700 That API sounds amazingly error prone. Sounds like they need to add the concept of PassRefPtr or adoptRef. Perhaps we should create a subclass of SkRefPtr that add an adoptRef method? If you feel like this is too big a yak to shave, you should feel free to put it off to a future patch. 464566 15 simonjam 2011-09-08 18:39:27 -0700 (In reply to comment #14) > If you feel like this is too big a yak to shave, you should feel free to put it off to a future patch. Honestly, I think the right thing to do would be to fix SkRefPtr to behave like every other RefPtr-like class in the world. I already found one place in Chrome where we use SkRefPtr "incorrectly" and leak something. Once SkRefPtr is fixed, then we can just use it as normal in WebKit. For now, I'd like to just land this patch, because it's on the Chrome CodeYellow list. 464575 16 simonjam 2011-09-08 18:50:40 -0700 (In reply to comment #15) > Honestly, I think the right thing to do would be to fix SkRefPtr to behave like every other RefPtr-like class in the world. I already found one place in Chrome where we use SkRefPtr "incorrectly" and leak something. Once SkRefPtr is fixed, then we can just use it as normal in WebKit. FYI, I submitted a bug against Skia about this. I'll fix it if nobody else does. 464585 17 106814 abarth 2011-09-08 19:04:22 -0700 Comment on attachment 106814 Patch Sounds like a good plan. 464672 18 106814 webkit.review.bot 2011-09-09 00:13:08 -0700 Comment on attachment 106814 Patch Clearing flags on attachment: 106814 Committed r94838: <http://trac.webkit.org/changeset/94838> 464673 19 webkit.review.bot 2011-09-09 00:13:13 -0700 All reviewed patches have been landed. Closing bug. 464744 20 reed 2011-09-09 05:18:58 -0700 Just saw this thread this morning... Another fix that is consistent with other code in skia could be: SkAutoTUnref<SkStream> stream = new RemoteFontStream(buffer); SkTypeface* typeface = SkTypeface::CreateFromStream(stream.get()); 464938 21 abarth 2011-09-09 11:30:08 -0700 Thanks Mike. Maybe we should use that pattern everywhere so that folks will have good examples to emulate. 106809 2011-09-08 16:11:06 -0700 2011-09-08 16:59:21 -0700 Patch bug-67815-20110908161105.patch text/plain 2739 simonjam U3VidmVyc2lvbiBSZXZpc2lvbjogOTQ3NDgKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBlMTA1MmQwMjI3ZDE2YjU2 NTU5MmQ1ZDc1MzhiMjljNWNhYjM1MDlkLi45MTMyMmZhN2JlMjk5ODQzZDllMTI3YjQ5MWRjN2Rk YTM4MzhjZDcwIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTEtMDktMDggIEphbWVz IFNpbW9uc2VuICA8c2ltb25qYW1AY2hyb21pdW0ub3JnPgorCisgICAgICAgIFtDaHJvbWl1bV0g Rml4IGxlYWsgb2YgU2tpYSBzdHJlYW0gd2l0aCBjdXN0b20gQ1NTIGZvbnRzCisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02NzgxNQorCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRlc3Q6IGZhc3QvY3NzL2ZvbnQt ZmFjZS1vcGVudHlwZS5odG1sIHVuZGVyIHZhbGdyaW5kCisKKyAgICAgICAgKiBwbGF0Zm9ybS9n cmFwaGljcy9tYWMvRm9udEN1c3RvbVBsYXRmb3JtRGF0YS5jcHA6CisgICAgICAgIChXZWJDb3Jl OjpjcmVhdGVGb250Q3VzdG9tUGxhdGZvcm1EYXRhKToKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFw aGljcy9za2lhL0ZvbnRDdXN0b21QbGF0Zm9ybURhdGEuY3BwOgorICAgICAgICAoV2ViQ29yZTo6 Y3JlYXRlRm9udEN1c3RvbVBsYXRmb3JtRGF0YSk6CisKIDIwMTEtMDktMDcgIFNoZXJpZmYgQm90 ICA8d2Via2l0LnJldmlldy5ib3RAZ21haWwuY29tPgogCiAgICAgICAgIFVucmV2aWV3ZWQsIHJv bGxpbmcgb3V0IHI5NDY3NCBhbmQgcjk0Njg5LgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUv cGxhdGZvcm0vZ3JhcGhpY3MvbWFjL0ZvbnRDdXN0b21QbGF0Zm9ybURhdGEuY3BwIGIvU291cmNl L1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvbWFjL0ZvbnRDdXN0b21QbGF0Zm9ybURhdGEuY3Bw CmluZGV4IDQ3ZDFmMjE1YWEyNjE5YjVlYTA2ZmUwNjFlNGYxZTFmMzg3YmFmYWMuLjYyNWRiN2Rl YzBkNTViMjI2YzVmNTkyZGViMDUxMGU0OGM3ZWNlZWYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJD b3JlL3BsYXRmb3JtL2dyYXBoaWNzL21hYy9Gb250Q3VzdG9tUGxhdGZvcm1EYXRhLmNwcAorKysg Yi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9tYWMvRm9udEN1c3RvbVBsYXRmb3Jt RGF0YS5jcHAKQEAgLTE2MSw4ICsxNjEsOCBAQCBGb250Q3VzdG9tUGxhdGZvcm1EYXRhKiBjcmVh dGVGb250Q3VzdG9tUGxhdGZvcm1EYXRhKFNoYXJlZEJ1ZmZlciogYnVmZmVyKQogCiAgICAgRm9u dEN1c3RvbVBsYXRmb3JtRGF0YSogZm9udEN1c3RvbVBsYXRmb3JtRGF0YSA9IG5ldyBGb250Q3Vz dG9tUGxhdGZvcm1EYXRhKGNvbnRhaW5lclJlZiwgY2dGb250UmVmLnJlbGVhc2VSZWYoKSk7CiAj aWYgVVNFKFNLSUFfT05fTUFDX0NIUk9NSVVNKQotICAgIFJlbW90ZUZvbnRTdHJlYW0qIHN0cmVh bSA9IG5ldyBSZW1vdGVGb250U3RyZWFtKGJ1ZmZlcik7Ci0gICAgZm9udEN1c3RvbVBsYXRmb3Jt RGF0YS0+bV90eXBlZmFjZSA9IFNrVHlwZWZhY2U6OkNyZWF0ZUZyb21TdHJlYW0oc3RyZWFtKTsK KyAgICBSZW1vdGVGb250U3RyZWFtIHN0cmVhbShidWZmZXIpOworICAgIGZvbnRDdXN0b21QbGF0 Zm9ybURhdGEtPm1fdHlwZWZhY2UgPSBTa1R5cGVmYWNlOjpDcmVhdGVGcm9tU3RyZWFtKCZzdHJl YW0pOwogI2VuZGlmCiAgICAgcmV0dXJuIGZvbnRDdXN0b21QbGF0Zm9ybURhdGE7CiB9CmRpZmYg LS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9za2lhL0ZvbnRDdXN0b21Q bGF0Zm9ybURhdGEuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3Mvc2tpYS9G b250Q3VzdG9tUGxhdGZvcm1EYXRhLmNwcAppbmRleCBiNDIzMGQyOTgwZGY1YzZiZmZkZDQyMjkx NzExZjkwZGI3Nzc0NjEwLi4wNjBhOGYyMGU1M2FlNWQyMjNkMWU0N2Q5Y2U0YWQzMWE0NDY2MGNi IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9za2lhL0ZvbnRD dXN0b21QbGF0Zm9ybURhdGEuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBo aWNzL3NraWEvRm9udEN1c3RvbVBsYXRmb3JtRGF0YS5jcHAKQEAgLTE4OCw4ICsxODgsOCBAQCBG b250Q3VzdG9tUGxhdGZvcm1EYXRhKiBjcmVhdGVGb250Q3VzdG9tUGxhdGZvcm1EYXRhKFNoYXJl ZEJ1ZmZlciogYnVmZmVyKQogICAgICAgICByZXR1cm4gMDsKICAgICByZXR1cm4gbmV3IEZvbnRD dXN0b21QbGF0Zm9ybURhdGEoZm9udFJlZmVyZW5jZSwgZm9udE5hbWUpOwogI2VsaWYgT1MoVU5J WCkgfHwgUExBVEZPUk0oQlJFV01QKQotICAgIFJlbW90ZUZvbnRTdHJlYW0qIHN0cmVhbSA9IG5l dyBSZW1vdGVGb250U3RyZWFtKGJ1ZmZlcik7Ci0gICAgU2tUeXBlZmFjZSogdHlwZWZhY2UgPSBT a1R5cGVmYWNlOjpDcmVhdGVGcm9tU3RyZWFtKHN0cmVhbSk7CisgICAgUmVtb3RlRm9udFN0cmVh bSBzdHJlYW0oYnVmZmVyKTsKKyAgICBTa1R5cGVmYWNlKiB0eXBlZmFjZSA9IFNrVHlwZWZhY2U6 OkNyZWF0ZUZyb21TdHJlYW0oJnN0cmVhbSk7CiAgICAgaWYgKCF0eXBlZmFjZSkKICAgICAgICAg cmV0dXJuIDA7CiAgICAgcmV0dXJuIG5ldyBGb250Q3VzdG9tUGxhdGZvcm1EYXRhKHR5cGVmYWNl KTsK 106814 2011-09-08 16:59:24 -0700 2011-09-09 00:13:08 -0700 Patch bug-67815-20110908165923.patch text/plain 2353 simonjam U3VidmVyc2lvbiBSZXZpc2lvbjogOTQ3NDgKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBlMTA1MmQwMjI3ZDE2YjU2 NTU5MmQ1ZDc1MzhiMjljNWNhYjM1MDlkLi45MTMyMmZhN2JlMjk5ODQzZDllMTI3YjQ5MWRjN2Rk YTM4MzhjZDcwIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTEtMDktMDggIEphbWVz IFNpbW9uc2VuICA8c2ltb25qYW1AY2hyb21pdW0ub3JnPgorCisgICAgICAgIFtDaHJvbWl1bV0g Rml4IGxlYWsgb2YgU2tpYSBzdHJlYW0gd2l0aCBjdXN0b20gQ1NTIGZvbnRzCisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02NzgxNQorCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRlc3Q6IGZhc3QvY3NzL2ZvbnQt ZmFjZS1vcGVudHlwZS5odG1sIHVuZGVyIHZhbGdyaW5kCisKKyAgICAgICAgKiBwbGF0Zm9ybS9n cmFwaGljcy9tYWMvRm9udEN1c3RvbVBsYXRmb3JtRGF0YS5jcHA6CisgICAgICAgIChXZWJDb3Jl OjpjcmVhdGVGb250Q3VzdG9tUGxhdGZvcm1EYXRhKToKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFw aGljcy9za2lhL0ZvbnRDdXN0b21QbGF0Zm9ybURhdGEuY3BwOgorICAgICAgICAoV2ViQ29yZTo6 Y3JlYXRlRm9udEN1c3RvbVBsYXRmb3JtRGF0YSk6CisKIDIwMTEtMDktMDcgIFNoZXJpZmYgQm90 ICA8d2Via2l0LnJldmlldy5ib3RAZ21haWwuY29tPgogCiAgICAgICAgIFVucmV2aWV3ZWQsIHJv bGxpbmcgb3V0IHI5NDY3NCBhbmQgcjk0Njg5LgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUv cGxhdGZvcm0vZ3JhcGhpY3MvbWFjL0ZvbnRDdXN0b21QbGF0Zm9ybURhdGEuY3BwIGIvU291cmNl L1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvbWFjL0ZvbnRDdXN0b21QbGF0Zm9ybURhdGEuY3Bw CmluZGV4IDQ3ZDFmMjE1YWEyNjE5YjVlYTA2ZmUwNjFlNGYxZTFmMzg3YmFmYWMuLjljOWViOGNm ZTM4YzQxZTkxYmYzMmRkMTU2NmRlMGZiOTAyZWE4NTYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJD b3JlL3BsYXRmb3JtL2dyYXBoaWNzL21hYy9Gb250Q3VzdG9tUGxhdGZvcm1EYXRhLmNwcAorKysg Yi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9tYWMvRm9udEN1c3RvbVBsYXRmb3Jt RGF0YS5jcHAKQEAgLTE2Myw2ICsxNjMsNyBAQCBGb250Q3VzdG9tUGxhdGZvcm1EYXRhKiBjcmVh dGVGb250Q3VzdG9tUGxhdGZvcm1EYXRhKFNoYXJlZEJ1ZmZlciogYnVmZmVyKQogI2lmIFVTRShT S0lBX09OX01BQ19DSFJPTUlVTSkKICAgICBSZW1vdGVGb250U3RyZWFtKiBzdHJlYW0gPSBuZXcg UmVtb3RlRm9udFN0cmVhbShidWZmZXIpOwogICAgIGZvbnRDdXN0b21QbGF0Zm9ybURhdGEtPm1f dHlwZWZhY2UgPSBTa1R5cGVmYWNlOjpDcmVhdGVGcm9tU3RyZWFtKHN0cmVhbSk7CisgICAgc3Ry ZWFtLT51bnJlZigpOwogI2VuZGlmCiAgICAgcmV0dXJuIGZvbnRDdXN0b21QbGF0Zm9ybURhdGE7 CiB9CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9za2lhL0Zv bnRDdXN0b21QbGF0Zm9ybURhdGEuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhp Y3Mvc2tpYS9Gb250Q3VzdG9tUGxhdGZvcm1EYXRhLmNwcAppbmRleCBiNDIzMGQyOTgwZGY1YzZi ZmZkZDQyMjkxNzExZjkwZGI3Nzc0NjEwLi5hODA4NDA5MTY5YmVkMDYyNDg0MDRiZWM0ZWRkMDNi Mjk1ODc5NWJjIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9z a2lhL0ZvbnRDdXN0b21QbGF0Zm9ybURhdGEuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRm b3JtL2dyYXBoaWNzL3NraWEvRm9udEN1c3RvbVBsYXRmb3JtRGF0YS5jcHAKQEAgLTE5MCw2ICsx OTAsNyBAQCBGb250Q3VzdG9tUGxhdGZvcm1EYXRhKiBjcmVhdGVGb250Q3VzdG9tUGxhdGZvcm1E YXRhKFNoYXJlZEJ1ZmZlciogYnVmZmVyKQogI2VsaWYgT1MoVU5JWCkgfHwgUExBVEZPUk0oQlJF V01QKQogICAgIFJlbW90ZUZvbnRTdHJlYW0qIHN0cmVhbSA9IG5ldyBSZW1vdGVGb250U3RyZWFt KGJ1ZmZlcik7CiAgICAgU2tUeXBlZmFjZSogdHlwZWZhY2UgPSBTa1R5cGVmYWNlOjpDcmVhdGVG cm9tU3RyZWFtKHN0cmVhbSk7CisgICAgc3RyZWFtLT51bnJlZigpOwogICAgIGlmICghdHlwZWZh Y2UpCiAgICAgICAgIHJldHVybiAwOwogICAgIHJldHVybiBuZXcgRm9udEN1c3RvbVBsYXRmb3Jt RGF0YSh0eXBlZmFjZSk7Cg==