Bug 47572 – [Chromium] Leak in WebCore::createFontCustomPlatformData

• Component: WebKit Misc.
• Status: RESOLVED INVALID

[Hironori Bono]

(Copied from <http://crbug.com/31800>.)

From: http://build.chromium.org/buildbot/memory/builders/Webkit%20Linux%20(valgrind%20layout)/builds/12963/steps/memory%20test:%20layout/logs/stdio

Leak_DefinitelyLost
52 (16 direct, 36 indirect) bytes in 1 blocks are definitely lost in loss record 8,849 of 11,050
  operator new(unsigned int) (sr/local/google/valgrind-for-chromium-client/valgrind/scripts/valgrind-memcheck/coregrind/m_replacemalloc/vg_replace_malloc.c:276)
  WebCore::createFontCustomPlatformData(WebCore::SharedBuffer*) (third_party/WebKit/WebCore/platform/graphics/skia/FontCustomPlatformData.cpp:193)
  WebCore::CachedFont::ensureCustomFontData() (third_party/WebKit/WebCore/loader/CachedFont.cpp:112)
  WebCore::CSSFontFaceSource::getFontData(WebCore::FontDescription const&, bool, bool, WebCore::CSSFontSelector*) (third_party/WebKit/WebCore/css/CSSFontFaceSource.cpp:162)
  WebCore::CSSFontFace::getFontData(WebCore::FontDescription const&, bool, bool) (third_party/WebKit/WebCore/css/CSSFontFace.cpp:112)
  WebCore::CSSSegmentedFontFace::getFontData(WebCore::FontDescription const&) (third_party/WebKit/WebCore/css/CSSSegmentedFontFace.cpp:106)
  WebCore::CSSFontSelector::getFontData(WebCore::FontDescription const&, WTF::AtomicString const&) (third_party/WebKit/WebCore/css/CSSFontSelector.cpp:531)
  WebCore::FontCache::getFontData(WebCore::Font const&, int&, WebCore::FontSelector*) (third_party/WebKit/WebCore/platform/graphics/FontCache.cpp:384)
  WebCore::FontFallbackList::fontDataAt(WebCore::Font const*, unsigned int) const (third_party/WebKit/WebCore/platform/graphics/FontFallbackList.cpp:105)
  WebCore::FontFallbackList::primaryFontData(WebCore::Font const*) const (third_party/WebKit/WebCore/platform/graphics/FontFallbackList.h:66)
  WebCore::FontFallbackList::determinePitch(WebCore::Font const*) const (third_party/WebKit/WebCore/platform/graphics/FontFallbackList.cpp:76)
  WebCore::FontFallbackList::isFixedPitch(WebCore::Font const*) const (third_party/WebKit/WebCore/platform/graphics/FontFallbackList.h:47)
  WebCore::Font::isFixedPitch() const (third_party/WebKit/WebCore/platform/graphics/Font.h:251)
  WebCore::RenderText::computePreferredLogicalWidths(int, WTF::HashSet<WebCore::SimpleFontData const*, WTF::PtrHash<WebCore::SimpleFontData const*>, WTF::HashTraits<WebCore::SimpleFontData const*> >&, WebCore::GlyphOverflow&) (third_party/WebKit/WebCore/rendering/RenderText.cpp:540)
  WebCore::RenderText::computePreferredLogicalWidths(int) (third_party/WebKit/WebCore/rendering/RenderText.cpp:687)
  WebCore::RenderText::trimmedPrefWidths(int, int&, bool&, int&, bool&, bool&, bool&, int&, int&, int&, int&, bool&) (third_party/WebKit/WebCore/rendering/RenderText.cpp:585)
  WebCore::RenderBlock::computeInlinePreferredLogicalWidths() (third_party/WebKit/WebCore/rendering/RenderBlock.cpp:4912)
  WebCore::RenderBlock::computePreferredLogicalWidths() (third_party/WebKit/WebCore/rendering/RenderBlock.cpp:4594)
  WebCore::RenderBox::maxPreferredLogicalWidth() const (third_party/WebKit/WebCore/rendering/RenderBox.cpp:603)
  WebCore::RenderBox::computePositionedLogicalWidthUsing(WebCore::Length, WebCore::RenderBoxModelObject const*, WebCore::TextDirection, int, int, WebCore::Length, WebCore::Length, WebCore::Length, WebCore::Length, int&, int&, int&, int&) (third_party/WebKit/WebCore/rendering/RenderBox.cpp:2300)
  WebCore::RenderBox::computePositionedLogicalWidth() (third_party/WebKit/WebCore/rendering/RenderBox.cpp:2109)
  WebCore::RenderBox::computeLogicalWidth() (third_party/WebKit/WebCore/rendering/RenderBox.cpp:1428)
  WebCore::RenderBlock::layoutBlock(bool, int) (third_party/WebKit/WebCore/rendering/RenderBlock.cpp:1128)
  WebCore::RenderBlock::layout() (third_party/WebKit/WebCore/rendering/RenderBlock.cpp:1105)
  WebCore::RenderObject::layoutIfNeeded() (third_party/WebKit/WebCore/rendering/RenderObject.h:497)
  WebCore::RenderBlock::layoutPositionedObjects(bool) (third_party/WebKit/WebCore/rendering/RenderBlock.cpp:2048)
  WebCore::RenderBlock::layoutBlock(bool, int) (third_party/WebKit/WebCore/rendering/RenderBlock.cpp:1254)
  WebCore::RenderBlock::layout() (third_party/WebKit/WebCore/rendering/RenderBlock.cpp:1105)
  WebCore::RenderView::layout() (third_party/WebKit/WebCore/rendering/RenderView.cpp:122)
  WebCore::FrameView::layout(bool) (third_party/WebKit/WebCore/page/FrameView.cpp:763)
Suppression (error hash=#FFFFFFFFC6F32A08#):
{
   <insert_a_suppression_name_here>
   Memcheck:Leak
   fun:_Znw*
   fun:_ZN7WebCore28createFontCustomPlatformDataEPNS_12SharedBufferE
   fun:_ZN7WebCore10CachedFont20ensureCustomFontDataEv
   fun:_ZN7WebCore17CSSFontFaceSource11getFontDataERKNS_15FontDescriptionEbbPNS_15CSSFontSelectorE
   fun:_ZN7WebCore11CSSFontFace11getFontDataERKNS_15FontDescriptionEbb
   fun:_ZN7WebCore20CSSSegmentedFontFace11getFontDataERKNS_15FontDescriptionE
   fun:_ZN7WebCore15CSSFontSelector11getFontDataERKNS_15FontDescriptionERKN3WTF12AtomicStringE
   fun:_ZN7WebCore9FontCache11getFontDataERKNS_4FontERiPNS_12FontSelectorE
   fun:_ZNK7WebCore16FontFallbackList10fontDataAtEPKNS_4FontEj
   fun:_ZNK7WebCore16FontFallbackList15primaryFontDataEPKNS_4FontE
   fun:_ZNK7WebCore16FontFallbackList14determinePitchEPKNS_4FontE
   fun:_ZNK7WebCore16FontFallbackList12isFixedPitchEPKNS_4FontE
   fun:_ZNK7WebCore4Font12isFixedPitchEv
   fun:_ZN7WebCore10RenderText29computePreferredLogicalWidthsEiRN3WTF7HashSetIPKNS_14SimpleFontDataENS1_7PtrHashIS5_EENS1_10HashTraitsIS5_EEEERNS_13GlyphOverflowE
   fun:_ZN7WebCore10RenderText29computePreferredLogicalWidthsEi
   fun:_ZN7WebCore10RenderText17trimmedPrefWidthsEiRiRbS1_S2_S2_S2_S1_S1_S1_S1_S2_
   fun:_ZN7WebCore11RenderBlock35computeInlinePreferredLogicalWidthsEv
   fun:_ZN7WebCore11RenderBlock29computePreferredLogicalWidthsEv
   fun:_ZNK7WebCore9RenderBox24maxPreferredLogicalWidthEv
   fun:_ZN7WebCore9RenderBox34computePositionedLogicalWidthUsingENS_6LengthEPKNS_20RenderBoxModelObjectENS_13TextDirectionEiiS1_S1_S1_S1_RiS6_S6_S6_
   fun:_ZN7WebCore9RenderBox29computePositionedLogicalWidthEv
   fun:_ZN7WebCore9RenderBox19computeLogicalWidthEv
}

It seems this leak still happens sometimes on our valgrind bots, but not always. To investigate this leak today, it seems we forgot deleting the cached resources as written in Issue 23081 <http://crbug.com/23081>. (Mac WebKit releases them in [WebView _close:].) In fact, running the debug version of out test_shell with '--layout-tests LayoutTests/fast/css/font-face-opentype.html' options reports a leak of a CachedResource (i.e. CachedFont) object.

  % ~/Chrome/src/xcodebuild/Debug/TestShell.app/Contents/MacOS/TestShell --layout-tests LayoutTests/fast/css/font-face-opentype.html

  #URL:file:///Users/hbono/Chrome/src/third_party/WebKit/LayoutTests/fast/css/font-face-opentype.html
   (suppressed)
  #EOF
  LEAK: 1 CachedResource
  LEAK: 4 WebCoreNode

Regards,

[Hironori Bono]

Created attachment 70584 [details]
A quick fix

Greetings,

This is a quick fix for this issue. Even though this fix cals WebCore::cache()->setDisabled(true) in WebKit::shutdown(), ot may be a better idea to call this function in WebViewImpl::close() as Safari calls it?

Regards,

Hironori Bono

[Darin Fisher (:fishd, Google)]

Comment on attachment 70584 [details]
A quick fix

I would prefer to have Chrome call WebCache::clear() when in valgrind mode.
I don't think the release product should need to clear the WebCore cache
at renderer shutdown time.  It is costly to walk the cache and bring all of
those objects into memory just to free them.  This will make it slower to
close tabs, etc.

[Hironori Bono]

Created attachment 70835 [details]
A quick fix 2

(In reply to comment #2)
> (From update of attachment 70584 [details])
> I would prefer to have Chrome call WebCache::clear() when in valgrind mode.
> I don't think the release product should need to clear the WebCore cache
> at renderer shutdown time.  It is costly to walk the cache and bring all of
> those objects into memory just to free them.  This will make it slower to
> close tabs, etc.

Thank you for your comment. I have added a flag that controls whether we should clean the resources so we can clean them only when we run Chrome or TestShell on valgrind. (I'm going to send a Chromium change that actually sets this flag when running on valgrind.) Is it possible to look the updated one?

Regards,

Hironori Bono

[Darin Fisher (:fishd, Google)]

Comment on attachment 70835 [details]
A quick fix 2

Sorry, I meant that I think this should be a Chromium-only patch.
Just change Chromium to call WebKit::WebCache::clear() before
calling WebKit::shutdown(), provided we are in valgrind mode.

No WebKit change required :-)

[Hironori Bono]

(In reply to comment #4)
> (From update of attachment 70835 [details])
> Sorry, I meant that I think this should be a Chromium-only patch.
> Just change Chromium to call WebKit::WebCache::clear() before
> calling WebKit::shutdown(), provided we are in valgrind mode.
> 
> No WebKit change required :-)

Sorry, I did not notice WebKit API of chromium has WebCache.h. Yeah, we don't need to change WebKit.

Regards,

Hironori Bono

[Hironori Bono]

Greetings

I would like to close this bug because it does not need WebKit changes. Sorry for my false alarm.

Regards,

Hironori Bono