[Chromium] Fix leak of Skia stream with custom CSS fonts
Created attachment 106809 [details] Patch
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.
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?
Comment on attachment 106809 [details] Patch Oops. Didn't see Tony's comment.
(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.
Created attachment 106814 [details] Patch
Comment on attachment 106814 [details] Patch Please don't call unref directly. Maybe you should use a RefPtr and adoptRef?
Comment on attachment 106814 [details] Patch If RemoteFontStream is RefCounted, its constructor should be private and it should expose a ::create method that calls adoptRef.
(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?
> 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.
http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/wtf/RetainPtr.h
We probably need something similar to that for Skia if Skia doesn't have one already for us to use.
(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().
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.
(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.
(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.
Comment on attachment 106814 [details] Patch Sounds like a good plan.
Comment on attachment 106814 [details] Patch Clearing flags on attachment: 106814 Committed r94838: <http://trac.webkit.org/changeset/94838>
All reviewed patches have been landed. Closing bug.
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());
Thanks Mike. Maybe we should use that pattern everywhere so that folks will have good examples to emulate.