Bug 116317 - [GTK] Metabug: Memory leak detection using valgrind
Summary: [GTK] Metabug: Memory leak detection using valgrind
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 118382 118385 118528 118567 118248 118297 118307 118362 118386 118412 118473 118497 118505 118596 118675 118785 125441
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-17 08:19 PDT by Brian Holt
Modified: 2017-03-11 10:53 PST (History)
22 users (show)

See Also:


Attachments
Patch (223.19 KB, patch)
2013-05-20 07:45 PDT, Brian Holt
no flags Details | Formatted Diff | Diff
Patch (141.44 KB, patch)
2013-06-06 02:29 PDT, Brian Holt
no flags Details | Formatted Diff | Diff
WIP patch (118.38 KB, patch)
2013-07-04 05:58 PDT, Brian Holt
no flags Details | Formatted Diff | Diff
Patch (78.15 KB, patch)
2013-07-11 09:30 PDT, Brian Holt
no flags Details | Formatted Diff | Diff
Patch (78.05 KB, patch)
2013-07-12 03:18 PDT, Brian Holt
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Holt 2013-05-17 08:19:27 PDT
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.
Comment 1 Brian Holt 2013-05-20 07:45:50 PDT
Created attachment 202274 [details]
Patch
Comment 2 Ryosuke Niwa 2013-05-20 11:29:14 PDT
You shouldn’t be copying the suppressions from Chromium. It’s got all sorts of things that are irrelevant for DRT.
Comment 3 Ryosuke Niwa 2013-05-20 11:30:56 PDT
I also don’t think we should be copying code from Chromium. It’s got all sorts of useless stuff in it.
Comment 4 Brian Holt 2013-05-21 06:10:23 PDT
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?
Comment 5 Brian Holt 2013-06-06 02:29:03 PDT
Created attachment 203916 [details]
Patch
Comment 6 WebKit Commit Bot 2013-06-06 02:31:00 PDT
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.
Comment 7 Ryosuke Niwa 2013-06-06 02:34:36 PDT
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.
Comment 8 Brian Holt 2013-06-06 02:40:59 PDT
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.
Comment 9 Sergio Villar Senin 2013-07-04 03:46:54 PDT
Looks great, god job. Note that you have several suppressions that refer to the V8 engine, they're no longer valid.
Comment 10 Mario Sanchez Prada 2013-07-04 04:06:21 PDT
(In reply to comment #9)
> Looks great, god job. 
               ^^^
Good to know I work with a god!

/me could not resist
Comment 11 Brian Holt 2013-07-04 05:58:14 PDT
Created attachment 206083 [details]
WIP patch
Comment 12 Brian Holt 2013-07-11 09:30:00 PDT
Created attachment 206472 [details]
Patch
Comment 13 WebKit Commit Bot 2013-07-11 09:32:21 PDT
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.
Comment 14 Brian Holt 2013-07-11 09:33:30 PDT
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 15 Brian Holt 2013-07-11 09:36:00 PDT
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.
Comment 16 Martin Robinson 2013-07-11 09:40:56 PDT
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.
Comment 17 Brian Holt 2013-07-11 09:53:23 PDT
(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.
Comment 18 Brian Holt 2013-07-12 03:18:41 PDT
Created attachment 206519 [details]
Patch
Comment 19 WebKit Commit Bot 2013-07-12 03:21:36 PDT
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.
Comment 20 Brian Holt 2013-07-12 03:37:38 PDT
(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 21 Brian Holt 2013-07-17 03:00:21 PDT
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.