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.
Created attachment 202274 [details] Patch
You shouldn’t be copying the suppressions from Chromium. It’s got all sorts of things that are irrelevant for DRT.
I also don’t think we should be copying code from Chromium. It’s got all sorts of useless stuff in it.
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?
Created attachment 203916 [details] Patch
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.
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.
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.
Looks great, god job. Note that you have several suppressions that refer to the V8 engine, they're no longer valid.
(In reply to comment #9) > Looks great, god job. ^^^ Good to know I work with a god! /me could not resist
Created attachment 206083 [details] WIP patch
Created attachment 206472 [details] Patch
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.
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.
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.
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.
(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.
Created attachment 206519 [details] Patch
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.
(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.
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.