Bug 118785

Created attachment 206874 [details] Patch

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

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.

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

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.

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.

Created attachment 208129 [details] Patch Addressed review comments

(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.

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.

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.

Created attachment 208133 [details] Launch DRT under Valgrind v2

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? :-)

I will try to take a look shortly :).

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.

Comment on attachment 208133 [details] Launch DRT under Valgrind v2 This looks fine. I'm basically rubber-stamping the valigrind command line part :).

Comment on attachment 208133 [details] Launch DRT under Valgrind v2 Clearing flags on attachment: 208133 Committed r153721: <http://trac.webkit.org/changeset/153721>

All reviewed patches have been landed. Closing bug.

(In reply to comment #17) > All reviewed patches have been landed. Closing bug. Thanks everyone for the reviews, its much appreciated!