[Webkit-unassigned] [Bug 119448] [Gtk] Suppress irrelevant or known leaks for Valgrind

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Aug 4 16:36:46 PDT 2013


--- Comment #2 from Mario Sanchez Prada <mario at webkit.org>  2013-08-04 16:36:28 PST ---
(From update of attachment 208029)
View in context: https://bugs.webkit.org/attachment.cgi?id=208029&action=review

Thanks for the patch, Brian. Informal review here:

First of all, the patch looks good in general, although there are some problems such as the fact that the patch does not seem to be consistent with the fact that this bug depends on bug 118785 (you're adding some code here that is already proposed as part of the latest patch for that other bug, instead of proposing just the incremental diff compared to that one), or that there are some issues related to coding style that would need fixing before landing.

See my comments below...

> Tools/Scripts/webkitpy/port/gtk.py:31
> +import uuid

This line is already included as part of the latest patch for bug 118785. Please remove it from here.

> Tools/Scripts/webkitpy/port/gtk.py:94
> +        if self.get_option("leaks"):
> +            environment['G_SLICE'] = 'always-malloc'
> +            xmlfile = "".join(('\"', self.results_directory(), "/drt-%p-",
> +                               uuid.uuid1().hex, "-leaks.xml", '\"'))
> +            suppressionsfile = self.path_from_webkit_base(
> +                'Tools', 'valgrind', 'memcheck', 'suppressions.txt')
> +            environment['VALGRIND_OPTS'] = "--tool=memcheck " \
> +            "--num-callers=40 --demangle=no --trace-children=no " \
> +            "--smc-check=all-non-file --leak-check=yes " \
> +            "--leak-resolution=high --show-possibly-lost=no " \
> +            "--show-reachable=no --leak-check=full " \
> +            " --undef-value-errors=no " \
> +             "--gen-suppressions=all --xml=yes --xml-file=%s " \

As in the previous case commented above, these lines are already included as part of the latest patch for bug 118785. Please remove it from here and leave only the part that is actually added (the --suppressions=%s part)

> Tools/Scripts/webkitpy/port/gtk.py:99
> +            # FIXME: instead we should be using "jhbuild-wrapper --gtk run"
> +            # to ensure we get the correct library path.
> +            #environment['LD_LIBRARY_PATH'] = self.path_from_webkit_base(
> +            # 'WebKitBuild', 'Dependencies', 'Root', 'lib64')

These lines are already part of the other patch too. Please remove them

> Tools/Scripts/webkitpy/port/gtk.py:101
> +

Extra line.

> Tools/valgrind/memcheck/suppressions.txt:1
> +# There are four kinds of suppressions in this file.

Please fix this sentence: You mention there are four kinds of suppressions but I could only find three.

> Tools/valgrind/memcheck/suppressions.txt:2
> +# 1. third party stuff we have no control over

Bad sentence: Start with capitalized word ("Third") and finish with a period.

> Tools/valgrind/memcheck/suppressions.txt:5
> +# 2. intentional unit test errors, or stuff that is somehow a false positive
> +# in our own code, or stuff that is so trivial it's not worth fixing


> Tools/valgrind/memcheck/suppressions.txt:8
> +# These should all be in webkit's bug tracking system (but a few aren't yet).

webkit -> WebKit

> Tools/valgrind/memcheck/suppressions.txt:13
> +# 1. third party stuff we have no control over

Bad sentence: Start with capitalized word ("Third") and finish with a period.

> Tools/valgrind/memcheck/suppressions.txt:15
> +   #gtk developers don't like cleaning up one-time leaks.  See http://mail.gnome.org/archives/gtk-devel-list/2004-April/msg00230.html

Missing space at the beginning, missing period at the end and bad capitalization: "#gtk developers" -> "# GTK developers"

Also, you have two spaces between the period and "See". Please replace those spaces with a line break instead

> Tools/valgrind/memcheck/suppressions.txt:29
> +   FcConfigAppFontAddFile_leak

This underscore here is not needed (and inconsistent with what you are doing in other places (e.g. the previous two suppressions). Also, I would probably call this "Fontconfig leak 1" for consistency with the rest of the file (see my following comments below)

> Tools/valgrind/memcheck/suppressions.txt:36
> +   # See also http://www.gnome.org/~johan/gtk.suppression
> +   # (which has a smattering of similar pango suppressions)

Missing period at the end of the line

> Tools/valgrind/memcheck/suppressions.txt:37
> +   pango_font_leak_todo

Underscores not needed, and name not consistent with others. What about "Fontconfig leak 2"?

> Tools/valgrind/memcheck/suppressions.txt:45
> +   pango_font_leak_todo_2

"Fontconfig leak 3"?

> Tools/valgrind/memcheck/suppressions.txt:55
> +   pango_font_leak_todo_3

"Fontconfig leak 4"?

Same applies to many other names in this file. Please try to get a rule for naming suppressions and stick to it, for the sake of consistency (and remember you do not need to use underscores here)

> Tools/valgrind/memcheck/suppressions.txt:63
> +   pango_font_leak_todo_4

"Fontcontig leak 5"? (last nit of this kind that I mention, please fix the others)

> Tools/valgrind/memcheck/suppressions.txt:88
> +   # Similar to fontconfig_bug_8428 below. Reported in
> +   # https://bugs.freedesktop.org/show_bug.cgi?id=8215

Missing period at the end.

> Tools/valgrind/memcheck/suppressions.txt:98
> +   # Fontconfig leak, seen in shard 16 of 20 of ui_tests
> +   # See https://bugs.freedesktop.org/show_bug.cgi?id=8428
> +   # and http://www.gnome.org/~johan/gtk.suppression


> Tools/valgrind/memcheck/suppressions.txt:116
> +   fontconfig_bug   (Third Party)

Please notice you have extra unneeded spaces in this line while fixing its name

> Tools/valgrind/memcheck/suppressions.txt:176
> +   dlopen leak on error. See http://sourceware.org/bugzilla/show_bug.cgi?id=12878.

Be consistent: Please either you put the "See http://..." string in the previous line as a comment preceeded by "#", or you just avoid using comments at all and you always bundle the "Seeh http://..." string along with the name of the rules. I personally think the first option is better.

> Tools/valgrind/memcheck/suppressions.txt:203
> +   # array of weak references freed but not processed?

Bad capitalization at the beginning. Missing period at the end of the line.

> Tools/valgrind/memcheck/suppressions.txt:211
> +   http://sources.redhat.com/bugzilla/show_bug.cgi?id=5171

You never used an URL before as the name of the suppression. As I mentioned before, please be consistent.

> Tools/valgrind/memcheck/suppressions.txt:283
> +    Gtk leaks an X11 window bug_96402 (Third party)

Extra space before "Gtk". Also, it is GTK or GTK+, but not Gtk.

> Tools/valgrind/memcheck/suppressions.txt:299
> +   Gtk leaks a cairo context (large leak)

Gtk -> GTK

> Tools/valgrind/memcheck/suppressions.txt:309
> +   leaks in bash

leaks -> Leaks

> Tools/valgrind/memcheck/suppressions.txt:343
> +# XRandRInfo object seems to be leaking inside XRRFindDisplay.  This happens the
> +# first time it is called, no matter who the caller is.  We have observed this
> +# problem with both XRRSelectInput and XRRQueryExtension.

Extra blank spaces that are not needed after the two first periods.

> Tools/valgrind/memcheck/suppressions.txt:351
> +   Flash Player Leak

Flash Player Leak -> Flash player leak

> Tools/valgrind/memcheck/suppressions.txt:426
> +   cairo pixman image allocate

Part of this sentence is a function name, so I would put something like "Cairo _pixman_image_allocate leak"

> Tools/valgrind/memcheck/suppressions.txt:514
> +   AtkProperty is cached but not released (see https://bugs.webkit.org/show_bug.cgi?id=118567#c2)

Again, please be consistent with naming and URLs

> Tools/valgrind/memcheck/suppressions.txt:526
> +   AtkRelationSet is never freed because the target AtkObject is cached https://bugs.webkit.org/show_bug.cgi?id=118567


> Tools/valgrind/memcheck/suppressions.txt:545
> +# 2. intentional unit test errors, or stuff that is somehow a false positive
> +# in our own code, or stuff that is so trivial it's not worth fixing

Bad sentence: Start with capitalized word ("Third") and finish with a period.

> Tools/valgrind/memcheck/suppressions.txt:646
> +   Leak webkitAccessibleNew https://bugs.webkit.org/show_bug.cgi?id=118382

Same comment about consistency on names and URLs

> Tools/valgrind/memcheck/suppressions.txt:655
> +   Leak atk title https://bugs.webkit.org/show_bug.cgi?id=118385


> Tools/valgrind/memcheck/suppressions.txt:665
> +   Leak PluginObject in exceptional circumstances https://bugs.webkit.org/show_bug.cgi?id=118528


> Tools/valgrind/memcheck/suppressions.txt:675
> +   TextCodecICU::registerCodecs is leaking https://bugs.webkit.org/show_bug.cgi?id=118505


Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

More information about the webkit-unassigned mailing list