Bug 118785 – [GTK] Implement leak checking with valgrind

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Brian Holt]

https://bugs.webkit.org/show_bug.cgi?id=116317 is a meta bug for tracking memory leaks found in the Gtk port. It should remain open until all the leaks have been resolved.

This bug is to implement the leak checking.

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.

[Brian Holt]

Created attachment 206874 [details]
Patch

[Brian Holt]

Created attachment 208021 [details]
Launch DRT under valgrind

launch the DRT under valgrind to generate a xml file with details of leaks found. \n run with \n $ Tools/Scripts/run-webkit-tests --gtk --debug --leaks --wrapper=valgrind --run-singly LayoutTests/platform/gtk

[Brian Holt]

Given that the previous patch was large and daunting to review, I'm breaking this up into smaller patches.  The first one is to launch the DRT under Valgrind.  

I will submit another patch (in a new bug) to implement the parsing of the xml files that are generated by Valgrind.  This will contain Chromium code, but I will additionally upload diffs with the current Chromium code to assist reviewing.

[Mario Sanchez Prada]

Comment on attachment 208021 [details]
Launch DRT under valgrind

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

Thanks for the patch Brian. I'm informally reviewing it now, hope you will find my comments useful. 

Overall, the patch seems to be fine to me, although I see some things that perhaps might not be needed (e.g. the changes related to is_running) and some minor nits. See my comments below...

> Tools/ChangeLog:15
> +        * Scripts/webkitpy/port/base.py:
> +        (Port.check_for_leaks): Move is_running flag to base class.

I don't really understand why you need to move this here if you are actually not reading this value for gtk, and the same logic could be kept for the mac without having this parameter moved here

> Tools/Scripts/webkitpy/port/base.py:384
> -    def check_for_leaks(self, process_name, process_pid):
> +    def check_for_leaks(self, process_name, process_pid, is_running):

As mentioned above, it seems to me this change is not actually needed.

> Tools/Scripts/webkitpy/port/gtk.py:52
> +                raise ValueError('use --wrapper=\"valgrind\" for'
> +                                 ' memory leak detection on GTK')

No need to split these line in two

> Tools/Scripts/webkitpy/port/gtk.py:55
> +            self.set_option_default("batch_size", 1000)

Where does this 1000 number come from? Is it just personal experience or something else?

> Tools/Scripts/webkitpy/port/gtk.py:70
> +        multiplier = 10 if self.get_option("leaks") else 1

I think a brief comment here explaining why having a ten times bigger timeout is needed for the "leaks" case would be helpful

> Tools/Scripts/webkitpy/port/gtk.py:80
> +        # we want to be slow on cleanup as well
> +        # (for things like ASAN, Valgrind, etc.)

Put these two last lines in one. End with a period.

> Tools/Scripts/webkitpy/port/gtk.py:104
> +            xmlfile = "".join(('\"', self.results_directory(), "/drt-%p-",
> +                               uuid.uuid1().hex, "-leaks.xml", '\"'))

No need to split this in two lines. Also, I think you could use os.path.join() better here.

> Tools/Scripts/webkitpy/port/gtk.py:112
> +            # FIXME: instead we should be using "jhbuild-wrapper --gtk run"

instead -> Instead

> Tools/Scripts/webkitpy/port/gtk.py:115
> +            #environment['LD_LIBRARY_PATH'] = self.path_from_webkit_base(
> +            #    'WebKitBuild', 'Dependencies', 'Root', 'lib64')

Remove these two lines if not needed, otherwise uncomment them and put them in a single line

> Tools/Scripts/webkitpy/port/gtk.py:161
> +    def check_for_leaks(self, process_name, process_pid, is_running):
> +        # With Gtk we initiate memory checking by passing valgrind as a wrapper to the test.
> +        pass

As mentioned above, you're not actually reading the is_running value here, so it seems like you do not need it for gtk

> Tools/Scripts/webkitpy/port/gtk.py:166
> +        # In a subsequent patch the leak xml files will be parsed here.

Add a FIXME: prefix here

> Tools/Scripts/webkitpy/port/mac.py:153
> -    def check_for_leaks(self, process_name, process_pid):
> -        if not self.get_option('leaks'):
> +    def check_for_leaks(self, process_name, process_pid, is_running):
> +        # Only bother to check for leaks or stderr if the process is still running.
> +        if not self.get_option('leaks') or not is_running:

You can't avoid this change, I think, if you just leave the original code in server_process.py untouched, since you are not doing anything special after all in the newly supported port (GTK)

> Tools/Scripts/webkitpy/port/server_process.py:335
> -        # Only bother to check for leaks or stderr if the process is still running.
> -        if self.poll() is None:
> -            self._port.check_for_leaks(self.name(), self.pid())
> +        is_running = self.poll() is None
> +        self._port.check_for_leaks(self.name(), self.pid(), is_running)

I think you don't need to do this, and so that you can leave the original code here too.

> Tools/Scripts/webkitpy/port/server_process_unittest.py:50
> -    def check_for_leaks(self, process_name, process_pid):
> +    def check_for_leaks(self, process_name, process_pid, is_running):

You don't need this change either

[Mario Sanchez Prada]

Comment on attachment 208021 [details]
Launch DRT under valgrind

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

> Tools/Scripts/webkitpy/port/gtk.py:111
> +            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 " % (xmlfile)

Also, please keep only one pair --option=value per line, with the first one (--tool=memcheck) starting in the line following the assignment, to improve readability.

[Mario Sanchez Prada]

Comment on attachment 208021 [details]
Launch DRT under valgrind

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

> Tools/Scripts/webkitpy/port/gtk.py:111
> +            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 " % (xmlfile)

Also, please keep only one pair --option=value per line, with the first one (--tool=memcheck) starting in the line following the assignment, to improve readability.

[Brian Holt]

Created attachment 208129 [details]
Patch

Addressed review comments

[Brian Holt]

(In reply to comment #4)
> (From update of attachment 208021 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208021&action=review
> 
> Thanks for the patch Brian. I'm informally reviewing it now, hope you will find my comments useful. 
> 

Thanks for the review! Your comments made me check assumptions I had made along the (long) way of creating this patch and as result I've trimmed it down to a *much* smaller patch.

[Mario Sanchez Prada]

Comment on attachment 208129 [details]
Patch

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

Besides a small nit (which I'm not even sure whether others would agree with anyway), the patch now looks good to me, so I would say an official review would be in place now.

It's so beautiful to see it reduced to less than half of the original size! :)

> Tools/Scripts/webkitpy/port/gtk.py:92
> +            environment['G_SLICE'] = 'always-malloc'

I understand that forcing malloc() for every single memory allocation is some kind of prerequisite to use valgrind here?

If so, a comment explaining the reason would be handy here too, as it's not obvious why we need this.

> Tools/Scripts/webkitpy/port/gtk.py:96
> +            environment['VALGRIND_OPTS'] = "--tool=memcheck " \
> +                "--num-callers=40 " \

Nit. I would probably move this --tool parameter to the following line, so every option is aligned.

[Brian Holt]

Comment on attachment 208129 [details]
Patch

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

Thanks for the review!  Since the other changes are so heavily dependent on this getting in I'm going to hold off uploading new versions of them until this gets in (hopefully soon)!

>> Tools/Scripts/webkitpy/port/gtk.py:92
>> +            environment['G_SLICE'] = 'always-malloc'
> 
> I understand that forcing malloc() for every single memory allocation is some kind of prerequisite to use valgrind here?
> 
> If so, a comment explaining the reason would be handy here too, as it's not obvious why we need this.

Good point - and in finding a link (https://wiki.gnome.org/Valgrind) I've found that I should also set G_DEBUG=gc-friendly.

>> Tools/Scripts/webkitpy/port/gtk.py:96
>> +                "--num-callers=40 " \
> 
> Nit. I would probably move this --tool parameter to the following line, so every option is aligned.

Sorry, you mentioned that last review but I missed it.

[Brian Holt]

Created attachment 208133 [details]
Launch DRT under Valgrind v2

[Mario Sanchez Prada]

Comment on attachment 208133 [details]
Launch DRT under Valgrind v2

Thanks Brian for addressing all the comments. I've certainly nothing to add now, so... time for and official review? :-)

[Dirk Pranke]

I will try to take a look shortly :).

[Zan Dobersek]

Comment on attachment 208133 [details]
Launch DRT under Valgrind v2

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

> Tools/Scripts/webkitpy/port/gtk.py:71
> +        # Starting an application under Valgrind takes a lot longer than normal
> +        # so increase the timeout (empirically 10x is enough to avoid timeouts).
> +        multiplier = 10 if self.get_option("leaks") else 1
>          if self.get_option('configuration') == 'Debug':
> -            return 12 * 1000
> -        return 6 * 1000
> +            return multiplier * 12 * 1000
> +        return multiplier * 6 * 1000

This timeout value covers the maximum time each test is allowed to run, not the amount of time each DumpRenderTree/WebKitTestRunner instance takes to fire up and get ready for testing.

I assume that under Valgrind, you'd still want a prolonged timeout value (so applying the multiplier here is OK), but the comment is not technically correct.

[Dirk Pranke]

Comment on attachment 208133 [details]
Launch DRT under Valgrind v2

This looks fine. I'm basically rubber-stamping the valigrind command line part :).

[WebKit Commit Bot]

Comment on attachment 208133 [details]
Launch DRT under Valgrind v2

Clearing flags on attachment: 208133

Committed r153721: <http://trac.webkit.org/changeset/153721>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brian Holt]

(In reply to comment #17)
> All reviewed patches have been landed.  Closing bug.

Thanks everyone for the reviews, its much appreciated!