Bug 116317 – [GTK] Metabug: Memory leak detection using valgrind

• Component: Tools / Tests
• Status: UNCONFIRMED

[Brian Holt]

Memory leak detection is a recurrent topic on webkit-dev but at present there exists only support for the Mac leaks tool.  

This meta-bug is to track work required to 
1) launch the DRT under valgrind to generate a xml file with details of leaks found
2) to parse this file and display leak results
3) update the valgrind suppressions file(s)
4) raise bugs for known memory leaks/violations.

A work-in-progress patch will be uploaded shortly.

[Brian Holt]

Created attachment 202274 [details]
Patch

[Ryosuke Niwa]

You shouldn’t be copying the suppressions from Chromium. It’s got all sorts of things that are irrelevant for DRT.

[Ryosuke Niwa]

I also don’t think we should be copying code from Chromium. It’s got all sorts of useless stuff in it.

[Brian Holt]

As I mentioned in the email to webkit-dev, Chromium has a fairly mature process for memory leak and error detection. The simplest path to follow was to reuse Chromium code as a starting point.  As you've suggested I'll start to strip away unnecessary code to simplify the patch.

Any other suggestions, comments, feedback?

[Brian Holt]

Created attachment 203916 [details]
Patch

[WebKit Commit Bot]

Attachment 203916 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py', u'Tools/Scripts/webkitpy/port/base.py', u'Tools/Scripts/webkitpy/port/common.py', u'Tools/Scripts/webkitpy/port/gdb_helper.py', u'Tools/Scripts/webkitpy/port/gtk.py', u'Tools/Scripts/webkitpy/port/gtk_leakdetector.py', u'Tools/Scripts/webkitpy/port/mac.py', u'Tools/Scripts/webkitpy/port/memcheck_analyze.py', u'Tools/Scripts/webkitpy/port/server_process.py', u'Tools/Scripts/webkitpy/port/server_process_unittest.py', u'Tools/valgrind/memcheck/suppressions.txt']" exit_code: 1
Tools/Scripts/webkitpy/port/gtk.py:174:  [GtkPort.print_leaks_summary] Instance of 'GtkLeakDetector' has no 'parse_leaks_output' member  [pylint/E1101] [5]
Tools/Scripts/webkitpy/port/common.py:131:  [RunSubprocess] Instance of 'Popen' has no 'pid' member (but some types could not be inferred)  [pylint/E1103] [5]
Tools/Scripts/webkitpy/port/common.py:134:  [RunSubprocess] Instance of 'Popen' has no 'pid' member (but some types could not be inferred)  [pylint/E1103] [5]
Tools/Scripts/webkitpy/port/common.py:135:  [RunSubprocess] Instance of 'Popen' has no 'pid' member (but some types could not be inferred)  [pylint/E1103] [5]
Tools/Scripts/webkitpy/port/gtk_leakdetector.py:78:  [GtkLeakDetector.check_for_leaks] Undefined variable 'leaks_output'  [pylint/E0602] [5]
Tools/Scripts/webkitpy/port/memcheck_analyze.py:126:  whitespace before '}'  [pep8/E202] [5]
Total errors found: 6 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Be sure to copy all copyright notices, etc... and be sure to follow the license terms of Chromium.  I also don't think you should be adding your copyright notice if you're simply coping fils and stripping code.

[Brian Holt]

I've gone through the suppressions file very carefully, removing all Chromium specific bits.

After running 
$ Tools/Scripts/run-webkit-tests --gtk --leak --wrapper="valgrind" --run-singly LayoutTests/
there are no leaks/errors that are not in the suppressions file.

I'll post the results of memcheck_analyse shortly.

[Sergio Villar Senin]

Looks great, god job. Note that you have several suppressions that refer to the V8 engine, they're no longer valid.

[Mario Sanchez Prada]

(In reply to comment #9)
> Looks great, god job. 
               ^^^
Good to know I work with a god!

/me could not resist

[Brian Holt]

Created attachment 206083 [details]
WIP patch

[Brian Holt]

Created attachment 206472 [details]
Patch

[WebKit Commit Bot]

Attachment 206472 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py', u'Tools/Scripts/webkitpy/port/base.py', u'Tools/Scripts/webkitpy/port/common.py', u'Tools/Scripts/webkitpy/port/gdb_helper.py', u'Tools/Scripts/webkitpy/port/gtk.py', u'Tools/Scripts/webkitpy/port/gtk_leakdetector.py', u'Tools/Scripts/webkitpy/port/mac.py', u'Tools/Scripts/webkitpy/port/memcheck_analyze.py', u'Tools/Scripts/webkitpy/port/server_process.py', u'Tools/Scripts/webkitpy/port/server_process_unittest.py', u'Tools/valgrind/memcheck/suppressions.txt']" exit_code: 1
Tools/Scripts/webkitpy/port/common.py:129:  [RunSubprocess] Instance of 'Popen' has no 'pid' member (but some types could not be inferred)  [pylint/E1103] [5]
Tools/Scripts/webkitpy/port/common.py:132:  [RunSubprocess] Instance of 'Popen' has no 'pid' member (but some types could not be inferred)  [pylint/E1103] [5]
Tools/Scripts/webkitpy/port/common.py:133:  [RunSubprocess] Instance of 'Popen' has no 'pid' member (but some types could not be inferred)  [pylint/E1103] [5]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brian Holt]

This patch has proven itself as quite useful so far for finding and resolving leaks.  I think that this patch is probably quite close so any feedback would be very welcome.

[Brian Holt]

Comment on attachment 206472 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=206472&action=review

This seems to be an issue with the style checker because Popen definitely has a member called pid.

>> Tools/Scripts/webkitpy/port/common.py:129
>> +            subprocess.call(["taskkill", "/T", "/F", "/PID", str(p.pid)])
> 
> [RunSubprocess] Instance of 'Popen' has no 'pid' member (but some types could not be inferred)  [pylint/E1103] [5]

From http://docs.python.org/2/library/subprocess.html:
Popen.pid
    The process ID of the child process.
    Note that if you set the shell argument to True, this is the process ID of the spawned shell.

[Martin Robinson]

Nice. This is a big patch, so I haven't had time to review it. It's probably best when adding a lot of Python code to follow the style of the existing code though -- which is PEP8 as far as I know. Also, we typically don't abbreviate variable names and comments are complete sentences with periods at the end.

[Brian Holt]

(In reply to comment #16)
> Nice. This is a big patch, so I haven't had time to review it. It's probably best when adding a lot of Python code to follow the style of the existing code though -- which is PEP8 as far as I know. Also, we typically don't abbreviate variable names and comments are complete sentences with periods at the end.

I've PEP8'ed the code so it passes the style check with the exception of the erroneous message about Popen.pid.  

I'll go back through all the code to check for variable names and comments, hopefully I can upload a new patch tomorrow.

[Brian Holt]

Created attachment 206519 [details]
Patch

[WebKit Commit Bot]

Attachment 206519 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py', u'Tools/Scripts/webkitpy/port/base.py', u'Tools/Scripts/webkitpy/port/common.py', u'Tools/Scripts/webkitpy/port/gdb_helper.py', u'Tools/Scripts/webkitpy/port/gtk.py', u'Tools/Scripts/webkitpy/port/gtk_leakdetector.py', u'Tools/Scripts/webkitpy/port/mac.py', u'Tools/Scripts/webkitpy/port/memcheck_analyze.py', u'Tools/Scripts/webkitpy/port/server_process.py', u'Tools/Scripts/webkitpy/port/server_process_unittest.py', u'Tools/valgrind/memcheck/suppressions.txt']" exit_code: 1
Tools/Scripts/webkitpy/port/common.py:129:  [RunSubprocess] Instance of 'Popen' has no 'pid' member (but some types could not be inferred)  [pylint/E1103] [5]
Tools/Scripts/webkitpy/port/common.py:132:  [RunSubprocess] Instance of 'Popen' has no 'pid' member (but some types could not be inferred)  [pylint/E1103] [5]
Tools/Scripts/webkitpy/port/common.py:133:  [RunSubprocess] Instance of 'Popen' has no 'pid' member (but some types could not be inferred)  [pylint/E1103] [5]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brian Holt]

(In reply to comment #19)
> Attachment 206519 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py', u'Tools/Scripts/webkitpy/port/base.py', u'Tools/Scripts/webkitpy/port/common.py', u'Tools/Scripts/webkitpy/port/gdb_helper.py', u'Tools/Scripts/webkitpy/port/gtk.py', u'Tools/Scripts/webkitpy/port/gtk_leakdetector.py', u'Tools/Scripts/webkitpy/port/mac.py', u'Tools/Scripts/webkitpy/port/memcheck_analyze.py', u'Tools/Scripts/webkitpy/port/server_process.py', u'Tools/Scripts/webkitpy/port/server_process_unittest.py', u'Tools/valgrind/memcheck/suppressions.txt']" exit_code: 1
> Tools/Scripts/webkitpy/port/common.py:129:  [RunSubprocess] Instance of 'Popen' has no 'pid' member (but some types could not be inferred)  [pylint/E1103] [5]
> Tools/Scripts/webkitpy/port/common.py:132:  [RunSubprocess] Instance of 'Popen' has no 'pid' member (but some types could not be inferred)  [pylint/E1103] [5]
> Tools/Scripts/webkitpy/port/common.py:133:  [RunSubprocess] Instance of 'Popen' has no 'pid' member (but some types could not be inferred)  [pylint/E1103] [5]
> Total errors found: 3 in 12 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

https://bugs.webkit.org/show_bug.cgi?id=118592 filed to fix the false positives.

[Brian Holt]

Comment on attachment 206519 [details]
Patch

This bug should remain a metabug for tracking open leaks, so moving the leak tool itself to https://bugs.webkit.org/show_bug.cgi?id=118785.