Bug 67815 – [Chromium] Fix leak of Skia stream with custom CSS fonts

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Simonsen]

[Chromium] Fix leak of Skia stream with custom CSS fonts

[James Simonsen]

Created attachment 106809 [details]
Patch

[James Simonsen]

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.

[Tony Chang]

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?

[Adam Barth]

Comment on attachment 106809 [details]
Patch

Oops.  Didn't see Tony's comment.

[James Simonsen]

(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.

[James Simonsen]

Created attachment 106814 [details]
Patch

[Adam Barth]

Comment on attachment 106814 [details]
Patch

Please don't call unref directly.  Maybe you should use a RefPtr and adoptRef?

[Adam Barth]

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.

[James Simonsen]

(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?

[Adam Barth]

> 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.

[Adam Barth]

http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/wtf/RetainPtr.h

[Adam Barth]

We probably need something similar to that for Skia if Skia doesn't have one already for us to use.

[James Simonsen]

(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().

[Adam Barth]

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.

[James Simonsen]

(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.

[James Simonsen]

(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.

[Adam Barth]

Comment on attachment 106814 [details]
Patch

Sounds like a good plan.

[WebKit Review Bot]

Comment on attachment 106814 [details]
Patch

Clearing flags on attachment: 106814

Committed r94838: <http://trac.webkit.org/changeset/94838>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mike Reed]

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());

[Adam Barth]

Thanks Mike.  Maybe we should use that pattern everywhere so that folks will have good examples to emulate.