[Webkit-unassigned] [Bug 118785] [GTK] Implement leak checking with valgrind

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=118785





--- Comment #4 from Mario Sanchez Prada <mario at webkit.org>  2013-08-04 15:49:05 PST ---
(From update of attachment 208021)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.