[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
https://bugs.webkit.org/show_bug.cgi?id=119448
--- 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
Likewise.
> 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
Likewise
> 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
Ditto.
> 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
Likewise
> Tools/valgrind/memcheck/suppressions.txt:665
> + Leak PluginObject in exceptional circumstances https://bugs.webkit.org/show_bug.cgi?id=118528
Likewise
> Tools/valgrind/memcheck/suppressions.txt:675
> + TextCodecICU::registerCodecs is leaking https://bugs.webkit.org/show_bug.cgi?id=118505
Likewise
--
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