Ensure that Canvas2D on Skia/GL runs without Valgrind memcheck errors

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bjacob, Unassigned)

Tracking

Trunk
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

Goal of this bug: on any platform where Valgrind runs, get a full run of Canvas 2D tests on Skia/GL without memcheck errors.

Current status: with the latest rebase (bug 848491) and the pthreads patch (bug 874682), on Linux x86-64, 64bit build, I can get a full run of

  http://philip.html5.org/tests/canvas/suite/tests/index.all.html

in Valgrind, but when I quit this browser, I eventually get Valgrind bailing out and killing it before the end, with this message:

--22932:0:aspacem  Valgrind: FATAL: VG_N_SEGMENTS is too low.
--22932:0:aspacem    Increase it and rebuild.  Exiting now.
(Reporter)

Updated

5 years ago
Blocks: 869076
(Reporter)

Updated

5 years ago
Depends on: 848482
(Reporter)

Updated

5 years ago
Depends on: 875216
(Reporter)

Comment 1

5 years ago
Alright, I could carry by tweaking these magic values in Valgrind:

bjacob:/hack/valgrind$ svn diff
Index: coregrind/m_aspacemgr/aspacemgr-linux.c
===================================================================
--- coregrind/m_aspacemgr/aspacemgr-linux.c     (revision 13401)
+++ coregrind/m_aspacemgr/aspacemgr-linux.c     (working copy)
@@ -265,10 +265,10 @@
 /* ------ start of STATE for the address-space manager ------ */
 
 /* Max number of segments we can track. */
-#define VG_N_SEGMENTS 5000
+#define VG_N_SEGMENTS 50000
 
 /* Max number of segment file names we can track. */
-#define VG_N_SEGNAMES 1000
+#define VG_N_SEGNAMES 10000
 
 /* Max length of a segment file name. */
 #define VG_MAX_SEGNAMELEN 1000
(Reporter)

Updated

5 years ago
Depends on: 875218
(Reporter)

Updated

5 years ago
Depends on: 876859
(Reporter)

Updated

5 years ago
Depends on: 884057
(Reporter)

Comment 2

5 years ago
Created attachment 763948 [details]
remaining leaks according to memcheck on June 17 (graphics branch, cset debcc24281dc, after ownership-refactoring and other misc fixes)
(Reporter)

Updated

5 years ago
Depends on: 886518
(Reporter)

Comment 3

5 years ago
Created attachment 766897 [details]
remaining leaks after bug 886518 patch

What's left:

- profiler pseudostacks leak (the threads stuff) already fixed in mozilla-central --- we're only hitting this because the last mozilla-central->graphics merge was a long time ago

- an interesting pango-related leak:

==18492== 1,152 bytes in 18 blocks are definitely lost in loss record 7,058 of 7,255
==18492==    at 0x402C7C1: malloc (vg_replace_malloc.c:292)
==18492==    by 0xC7FFEBE: ??? (in /usr/lib/x86_64-linux-gnu/libfontconfig.so.1.4.4)
==18492==    by 0xC8003E9: ??? (in /usr/lib/x86_64-linux-gnu/libfontconfig.so.1.4.4)
==18492==    by 0xC805D58: ??? (in /usr/lib/x86_64-linux-gnu/libfontconfig.so.1.4.4)
==18492==    by 0xC80628F: FcFreeTypeCharSetAndSpacing (in /usr/lib/x86_64-linux-gnu/libfontconfig.so.1.4.4)
==18492==    by 0xC8077C9: FcFreeTypeQueryFace (in /usr/lib/x86_64-linux-gnu/libfontconfig.so.1.4.4)
==18492==    by 0x84B8ED5: gfxDownloadedFcFontEntry::InitPattern() (gfxPangoFonts.cpp:548)
==18492==    by 0x84BBEAC: gfxPangoFontGroup::NewFontEntry(gfxProxyFontEntry const&, unsigned char const*, unsigned int) (gfxPangoFonts.cpp:1790)
==18492==    by 0x84F0B08: gfxUserFontSet::LoadFont(gfxMixedFontFamily*, gfxProxyFontEntry*, unsigned char const*, unsigned int&) (gfxUserFontSet.cpp:646)
==18492==    by 0x84F1504: gfxUserFontSet::OnLoadComplete(gfxMixedFontFamily*, gfxProxyFontEntry*, unsigned char const*, unsigned int, tag_nsresult) (gfxUserFontSet.cpp:421)
==18492==    by 0x7382D92: nsFontFaceLoader::OnStreamComplete(nsIStreamLoader*, nsISupports*, tag_nsresult, unsigned int, unsigned char const*) (nsFontFaceLoader.cpp:217)
==18492==    by 0x6F9F47B: nsStreamLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) (nsStreamLoader.cpp:101)
==18492== 


- the SkGlyphCache is leaked, but that is intentional according to this comment:
http://hg.mozilla.org/mozilla-central/file/c87a950e1a09/gfx/skia/src/core/SkGlyphCache.cpp#l491

==18492== 648 bytes in 27 blocks are definitely lost in loss record 6,943 of 7,255
==18492==    at 0x402C7C1: malloc (vg_replace_malloc.c:292)
==18492==    by 0x85D1223: _moz_cairo_font_options_create (cairo-font-options.c:107)
==18492==    by 0x87ACBCA: SkScalerContext_CairoFT::SkScalerContext_CairoFT(SkTypeface*, SkDescriptor const*) (SkFontHost_cairo.cpp:188)
==18492==    by 0x87ACD1E: SkCairoFTTypeface::onCreateScalerContext(SkDescriptor const*) const (SkFontHost_cairo.cpp:94)
==18492==    by 0x887B38E: SkTypeface::createScalerContext(SkDescriptor const*) const (SkScalerContext.cpp:787)
==18492==    by 0x884BCE9: SkGlyphCache::SkGlyphCache(SkTypeface*, SkDescriptor const*) (SkGlyphCache.cpp:64)
==18492==    by 0x884BE2F: SkGlyphCache::VisitCache(SkTypeface*, SkDescriptor const*, bool (*)(SkGlyphCache const*, void*), void*) (SkGlyphCache.cpp:569)
==18492==    by 0x8860E81: DetachDescProc(SkTypeface*, SkDescriptor const*, void*) (SkPaint.cpp:419)
==18492==    by 0x8862E1C: SkPaint::descriptorProc(SkDeviceProperties const*, SkMatrix const*, void (*)(SkTypeface*, SkDescriptor const*, void*), void*, bool) const (SkPaint.cpp:1878)
==18492==    by 0x8862E7A: SkPaint::detachCache(SkDeviceProperties const*, SkMatrix const*) const (SkPaint.cpp:1884)
==18492==    by 0x88421D8: SkAutoGlyphCache::SkAutoGlyphCache(SkPaint const&, SkDeviceProperties const*, SkMatrix const*) (SkGlyphCache.h:283)
==18492==    by 0x8842902: SkDraw::drawPosText(char const*, unsigned long, float const*, float, int, SkPaint const&) const (SkDraw.cpp:1851)


- the rest seems to be noise, mostly GTK-related.
Attachment #763948 - Attachment is obsolete: true
(Reporter)

Comment 4

5 years ago
Created attachment 767384 [details]
remaining leaks after updated bug 886518 patch

Hm, the pango leak is not seen in this log. Checking if it makes sense that this small change would have fixed it.
Attachment #766897 - Attachment is obsolete: true
(Reporter)

Comment 5

5 years ago
Created attachment 767554 [details]
remaining leaks after updated bug 886518 patch, entire 2D canvas tests run

....here's a valgrind leak log for a run of the entire canvas test suite, confirming that the gfxPangoFonts leak doesn't reproduce anymore. With the SkGlyphCache leak being understood as intentional (see above) and the mozilla_sampler_register_thread being a known leak in the Gecko profiler, already fixed upstream, we can consider ourselves leak-free now!
Attachment #767384 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Updated

5 years ago
No longer depends on: 848491
You need to log in before you can comment on or make changes to this bug.