Bug 119448

Created attachment 208029 [details] Patch

Comment on attachment 208029 [details] Patch 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

Created attachment 208195 [details] Suppress known leaks

(In reply to comment #2) > (From update of attachment 208029 [details]) > 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. Thanks for the review. In light of the comments especially about style I've made all comments for leaks start with Uppercase and end with a period. I've also chosen the offending function as the leak name with and optional "(Third party)" or "(Intentional)".

Comment on attachment 208195 [details] Suppress known leaks Cancelling review: DRT now times out Valgrind only when using suppressions. Will submit a new patch that addresses this as well.

Created attachment 208197 [details] Suppress known leaks

Comment on attachment 208197 [details] Suppress known leaks View in context: https://bugs.webkit.org/attachment.cgi?id=208197&action=review I'll let some gtk reviewer review the actual suppressions for this ... the python code looks a bit odd (not sure why you're using default_timeout_ms instead of driver_stop_timeout in either case). > Tools/Scripts/webkitpy/port/gtk.py:77 > + return super(GtkPort, self).default_timeout_ms() Should this be super(...).driver_stop_timeout() ?

Comment on attachment 208197 [details] Suppress known leaks View in context: https://bugs.webkit.org/attachment.cgi?id=208197&action=review >> Tools/Scripts/webkitpy/port/gtk.py:77 >> + return super(GtkPort, self).default_timeout_ms() > > Should this be super(...).driver_stop_timeout() ? You're right - it should be. > Tools/Scripts/webkitpy/port/gtk.py:103 > + suppressionsfile = self.path_from_webkit_base('Tools', 'Scripts', 'valgrind', 'suppressions.txt') Is the path for the new suppressions file ok?

Created attachment 208245 [details] Suppress known leaks v2

Comment on attachment 208245 [details] Suppress known leaks v2 Tools/Scripts/valgrind seems fine to me. Python change also looks good.

Any chance of a review by a Gtk reviewer?

Thanks Martin! Would you mind setting cq+?

Comment on attachment 208245 [details] Suppress known leaks v2 Clearing flags on attachment: 208245 Committed r154387: <http://trac.webkit.org/changeset/154387>

All reviewed patches have been landed. Closing bug.