Bug 119446 - [GTK] Parse Valgrind xml leak files
: [GTK] Parse Valgrind xml leak files
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Tools / Tests
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Brian Holt
:
Depends on: 118785 126655
Blocks:
  Show dependency treegraph
 
Reported: 2013-08-02 08:24 PDT by Brian Holt
Modified: 2014-01-14 05:57 PST (History)
8 users (show)

See Also:


Attachments
Parse Valgrind XML files (50.01 KB, patch)
2013-08-02 09:12 PDT, Brian Holt
no flags Details | Formatted Diff | Diff
Diff of common.py with Chromium trunk (2.21 KB, patch)
2013-08-02 09:16 PDT, Brian Holt
no flags Details | Formatted Diff | Diff
Diff of gdb_helper.py with Chromium trunk (2.03 KB, patch)
2013-08-02 09:20 PDT, Brian Holt
no flags Details | Formatted Diff | Diff
Diff of memcheck_analyze.py with Chromium trunk (9.63 KB, patch)
2013-08-02 09:25 PDT, Brian Holt
no flags Details | Formatted Diff | Diff
WIP Patch 2 (54.07 KB, patch)
2013-10-30 03:24 PDT, Brian Holt
no flags Details | Formatted Diff | Diff
Patch (52.66 KB, patch)
2013-12-08 04:06 PST, Brian Holt
no flags Details | Formatted Diff | Diff
Patch (56.69 KB, patch)
2013-12-09 08:47 PST, Brian Holt
no flags Details | Formatted Diff | Diff
Patch (56.34 KB, patch)
2014-01-02 05:27 PST, Brian Holt
no flags Details | Formatted Diff | Diff
Patch (56.24 KB, patch)
2014-01-09 06:32 PST, Brian Holt
no flags Details | Formatted Diff | Diff
Updated to use Executive and MockExecutive in the unittest (61.34 KB, patch)
2014-01-10 06:07 PST, Brian Holt
no flags Details | Formatted Diff | Diff
Final version? (61.34 KB, patch)
2014-01-13 06:17 PST, Brian Holt
alex: review+
Details | Formatted Diff | Diff
Diff from previous r+'ed patch (12.66 KB, text/plain)
2014-01-13 09:54 PST, Brian Holt
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Holt 2013-08-02 08:24:26 PDT
Implement the parsing of the xml files that are generated by Valgrind.
Comment 1 Brian Holt 2013-08-02 09:12:38 PDT
Created attachment 208024 [details]
Parse Valgrind XML files
Comment 2 Brian Holt 2013-08-02 09:16:56 PDT
Created attachment 208025 [details]
Diff of common.py with Chromium trunk

This is a diff (without whitespace) of common.py which has been taken from Chromium.  The code has been reformatted to PEP8/pylint and the LICENSE file has been inserted at the top of the file.
Comment 3 Brian Holt 2013-08-02 09:20:38 PDT
Created attachment 208026 [details]
Diff of gdb_helper.py with Chromium trunk

As above, code has been reformatted to PEP8/pylint and the LICENSE file has been inserted at the top of the file.
Comment 4 Brian Holt 2013-08-02 09:25:45 PDT
Created attachment 208027 [details]
Diff of memcheck_analyze.py with Chromium trunk

As above the code has been reformatted to PEP8/pylint.  There are 2 non-cosmetic changes to this file:

1) a hunk to make suppressions platform dependent has been removed because it was found to break the parsing of suppressions.

2) Leaks_StillReachable, InvalidWrite and InvalidRead are explicitly excluded from the report (these can be added in at a later stage if deemed interesting).
Comment 5 Mario Sanchez Prada 2013-08-04 17:13:29 PDT
Comment on attachment 208024 [details]
Parse Valgrind XML files

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

Quick informal review. Mostly coding style issues and some small issues

> Tools/Scripts/webkitpy/port/common.py:37
> +import logging
> +import platform
> +import os
> +import signal
> +import subprocess
> +import sys
> +import time

Please use alphabetical order

> Tools/Scripts/webkitpy/port/common.py:39
> +

Extra line

> Tools/Scripts/webkitpy/port/common.py:43
> +
> +

Two empty lines here, while one would be enough between definitions. Please fix all across the file (it happens all the time in this patch)

> Tools/Scripts/webkitpy/port/common.py:55
> +    """ Runs a subprocess, until it finishes or |timeout| is exceeded and the

Extra space before "Runs"

> Tools/Scripts/webkitpy/port/common.py:266
> +    """ Prints out the list of used suppressions in a format common to all the

Extra space after the """

> Tools/Scripts/webkitpy/port/gdb_helper.py:40
> +

Extra line here too

> Tools/Scripts/webkitpy/port/gdb_helper.py:42
> +    ''' Parse the gdb output line, return a pair (file, line num) '''

You use ''' instead of """ in this file for documenting functions. Please use """ always (to be consistent), avoid the extra space at the beginning and the end, and finish the sentence with a period

> Tools/Scripts/webkitpy/port/gdb_helper.py:49
> +
> +

As in the other file, please use one empty line only to mark separation between defined functions

> Tools/Scripts/webkitpy/port/gdb_helper.py:51
> +    ''' For each address, return a pair (file, line num) '''

Use """ and period at the end, and avoid the extra spaces for consistency. Please fix further functions in this file too

> Tools/Scripts/webkitpy/port/gtk.py:133
> +        # FIXME: This will include too many leaks in subsequent runs until the results directory is cleared!

Why we don't just clear the directory automatically each time this logic is run, right at the start?

That way, you would have the results available after one execution as long as you haven't triggered a new one, giving you time enough to copy them to a safe location in case you want to keep them

> Tools/Scripts/webkitpy/port/gtk_leakdetector.py:35
> +

Extra line

> Tools/Scripts/webkitpy/port/gtk_leakdetector.py:47
> +        self._memcheck_leak_analyzer = MemcheckAnalyzer(port.webkit_base(),
> +                                                        use_gdb=False)

You can make this a single line

> Tools/Scripts/webkitpy/port/gtk_leakdetector.py:54
> +        return self._filesystem.glob(self._filesystem.join(directory,
> +                                                           "*-leaks.xml"))

Likewise

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:63
> +

Extra line

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:75
> +

Extra line. Please fix the rest like this one in this file

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:94
> +    prefixes_to_cut = ["build/src/", "valgrind/coregrind/",
> +                       "out/Release/../../"]

This can be made just one line, as in the original version of the file

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:140
> +            # Try using gdb

Missing period at the end

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:245
> +                self._backtraces.append([description,
> +                                         gatherFrames(node, source_dir)])

Make this one single line, as in the original version of the code

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:282
> +                buf += (frame[FUNCTION_NAME] or \
> +                        frame[INSTRUCTION_POINTER]) + "\n"

Make this one single line, as in the original version of the code

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:296
> +                    foo = TheAddressTable.GetFileLine(
> +                        frame[OBJECT_FILE],
> +                        frame[INSTRUCTION_POINTER])

Make this one line

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:302
> +                    output += (" (" + frame[SRC_FILE_DIR] +
> +                               "/" + frame[SRC_FILE_NAME] +
> +                               ":" + frame[SRC_LINE] + ")")

These three lines can be only one too

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:323
> +        # Widen suppression slightly to make portable between mac and linux
> +        # TODO(timurrrr): Oops, these transformations should happen
> +        # BEFORE calculating the hash!

Missing periods at the end of sentences

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:330
> +        # Split into lines so we can enforce length limits

Missing period

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:406
> +            if prev_line.strip() in ["</error>",
> +                                     "</errorcounts>",
> +                                     "</status>"]:

This can be one line too
Comment 6 Dirk Pranke 2013-08-05 13:00:45 PDT
Comment on attachment 208024 [details]
Parse Valgrind XML files

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

I didn't review everything in this patch yet (in particular, I haven't looked at memcheck_analyze.py much. However, this code is clearly not written to generally conform to the webkitpy conventions and (IMO) should be cleaned up to do so, first.

You also should write at least some unittests for this.

> Tools/Scripts/webkitpy/port/common.py:35
> +import subprocess

This code  duplicates a lot of the work in webkitpy.common.system and webkitpy.port.server_process . Please use that code instead and/or get rid of the duplicate logic here.

>> Tools/Scripts/webkitpy/port/common.py:43
>> +
> 
> Two empty lines here, while one would be enough between definitions. Please fix all across the file (it happens all the time in this patch)

Actually PEP-8 requires two blanks between top-level definitions.

> Tools/Scripts/webkitpy/port/common.py:219
> +         "_ZN4base8internal15RunnableAdapter*Run*"),

Most of this file is pretty generic, but this routine is clearly pretty specific to one particular caller or purpose. You should rename this to something more indicative. Also, somehow it seems unlikely that the base:: functions will be needed somewhere :).

> Tools/Scripts/webkitpy/port/gdb_helper.py:31
> +''' A bunch of helper functions for querying gdb.'''

Again, you should probably be using the routines in webkitpy.system instead.

> Tools/Scripts/webkitpy/port/gtk_leakdetector.py:27
> +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

I would probably just fold these classes into gtk.py rather than create a separate file.

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:666
> +def _main():

Normally people use main() for this rather than _main() . Also, in webkitpy land, we wouldn't have this at all and would either split this into a separate helper script in Tools/Scripts or a _unittest.py file.
Comment 7 Brian Holt 2013-08-06 02:27:56 PDT
Comment on attachment 208024 [details]
Parse Valgrind XML files

Thanks for the review and comments.  Obviously my strategy was to import (reviewed) Chromium code but I didn't realise that quite a bit of the code is already in WebKit.  I will webkitpy'ify this code and submit another patch (with unit tests).
Comment 8 Brian Holt 2013-10-30 03:24:03 PDT
Created attachment 215490 [details]
WIP Patch 2
Comment 9 Brian Holt 2013-10-30 03:37:42 PDT
(In reply to comment #8)
> Created an attachment (id=215490) [details]
> WIP Patch 2

As the title suggests, this is a WIP patch but the unit test for the leakdetector passes and if anyone wants to use this its good to go.
Comment 10 Brian Holt 2013-12-08 04:06:09 PST
Created attachment 218681 [details]
Patch
Comment 11 Alejandro G. Castro 2013-12-09 01:25:26 PST
Comment on attachment 218681 [details]
Patch

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

Patch is looking good, but I would like a pythong reviewer to check it. I like the result it is very useful. I'm also uploading a blocking patch to this one because I found some issue in the gtk.py leak detector. Also I would like to remove the two parameters --leaks and --wrapper=valgrind, at least for the default situation.

> Tools/ChangeLog:7
> +
> +        Need a short description (OOPS!).
> +        Need the bug URL (OOPS!).

You just can remove this.

> Tools/Scripts/webkitpy/port/gtk.py:-163
> -    # FIXME: We should find a way to share this implmentation with Gtk,
> -    # or teach run-launcher how to call run-safari and move this down to Port.

Why are you removing this comment from the show_results_html_file function?

> Tools/Scripts/webkitpy/port/leakdetector_valgrind_unittest.py:716
> +        files['/Users/mock/Library/Logs/layout-test-results/drt-28529-db92e4843be411e3bae1d43d7e01ba08-leaks.xml'] = mock_valgrind_output1
> +        files['/Users/mock/Library/Logs/layout-test-results/drt-28530-dd7213423be411e3aa7fd43d7e01ba08-leaks.xml'] = mock_valgrind_output2
> +        #files['/Users/mock/Library/Logs/layout-test-results/drt-28531-e8c7d7b83be411e390c9d43d7e01ba08-leaks.xml'] = mock_incomplete_valgrind_output
> +        #files['/Users/mock/Library/Logs/layout-test-results/drt-28532-ebc9a6c63be411e399d4d43d7e01ba08-leaks.xml'] = None
> +        #files['/Users/mock/Library/Logs/layout-test-results/drt-28532-fa6d0cd63be411e39c72d43d7e01ba08-leaks.xml'] = misformatted_mock_valgrind_output
> +        filesystem = MockFileSystem(files)

I guess this commented lines mean this is still unfinished? I would add at least a some empty output, broken output and proper output in different  unit tests.
Comment 12 Zan Dobersek 2013-12-09 02:50:47 PST
Comment on attachment 218681 [details]
Patch

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

> Tools/Scripts/webkitpy/port/leakdetector_valgrind.py:59
> +    return text
> +
> +# Constants that give real names to the abbreviations in valgrind XML output.

Nit: missing a newline here.

> Tools/Scripts/webkitpy/port/leakdetector_valgrind.py:80
> +def gather_frames(node, source_dir):
> +    frames = []
> +    for frame in node.getElementsByTagName("frame"):
> +        frame_dict = {
> +            INSTRUCTION_POINTER: get_text_of(frame, INSTRUCTION_POINTER),
> +            OBJECT_FILE: get_text_of(frame, OBJECT_FILE),
> +            FUNCTION_NAME: get_text_of(frame, FUNCTION_NAME),
> +            SRC_FILE_DIR: get_text_of(frame, SRC_FILE_DIR),
> +            SRC_FILE_NAME: get_text_of(frame, SRC_FILE_NAME),
> +            SRC_LINE: get_text_of(frame, SRC_LINE)}
> +
> +        frames += [frame_dict]
> +    return frames

You can put the frame_dict construction inside a lambda function and then simply do 'return [frame_dict(frame) for frame in node.getElementsByTagName("frame")]'.

> Tools/Scripts/webkitpy/port/leakdetector_valgrind.py:122
> +        output = ""
> +
> +        output += self._kind + "\n"

You can initialize output straight to self._kind + "\n".

> Tools/Scripts/webkitpy/port/leakdetector_valgrind.py:186
> +        output += "\n".join(supplines) + "\n"
> +
> +        return output

You can go straight to returning the concatenated output and the joined suppression lines.

> Tools/Scripts/webkitpy/port/leakdetector_valgrind.py:280
> +            os.remove(f)

You should use self._filesystem.remove(f) here.
The os module import can be removed after that.

> Tools/Scripts/webkitpy/port/leakdetector_valgrind_unittest.py:717
> +        leakdetector_valgrind = LeakDetectorValgrind(filesystem, '/Users/mock/Library/Logs/layout-test-results/')

Nit: You could use a simpler path for the mock results.

> Tools/Scripts/webkitpy/port/leakdetector_valgrind_unittest.py:724
> +        oc = OutputCapture()
> +        #output.set_log_level(logging.INFO)
> +        oc.capture_output()
> +        leakdetector_valgrind.parse_and_print_leaks_detail(files)
> +        _, _, logfile = oc.restore_output()
> +        self.assertEqual(logfile, make_mock_valgrind_results())

I think you can use OutputCapture().assert_outputs(...) here.
Comment 13 Brian Holt 2013-12-09 03:20:58 PST
Comment on attachment 218681 [details]
Patch

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

So overall it seems like a few small changes and the extra tests and this could get in.  Does the simplicification of --leaks and --wrapper="valgrind" really block this?  Can I do that in a follow-up patch?

>> Tools/Scripts/webkitpy/port/gtk.py:-163
>> -    # or teach run-launcher how to call run-safari and move this down to Port.
> 
> Why are you removing this comment from the show_results_html_file function?

Because it was copied from mac.py and doesn't make sense anymore:  why would we want to share this with Gtk when we are in gtk.py?  
Still it is unrelated to the memory leak detection so I can leave that to another patch if you prefer.

>> Tools/Scripts/webkitpy/port/leakdetector_valgrind_unittest.py:716
>> +        filesystem = MockFileSystem(files)
> 
> I guess this commented lines mean this is still unfinished? I would add at least a some empty output, broken output and proper output in different  unit tests.

Sure.

>> Tools/Scripts/webkitpy/port/leakdetector_valgrind_unittest.py:717
>> +        leakdetector_valgrind = LeakDetectorValgrind(filesystem, '/Users/mock/Library/Logs/layout-test-results/')
> 
> Nit: You could use a simpler path for the mock results.

Good idea.

>> Tools/Scripts/webkitpy/port/leakdetector_valgrind_unittest.py:724
>> +        self.assertEqual(logfile, make_mock_valgrind_results())
> 
> I think you can use OutputCapture().assert_outputs(...) here.

Ok, I'll look into it.
Comment 14 Brian Holt 2013-12-09 08:47:48 PST
Created attachment 218767 [details]
Patch
Comment 15 Zan Dobersek 2013-12-09 10:40:09 PST
Comment on attachment 218767 [details]
Patch

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

> Tools/Scripts/webkitpy/port/leakdetector_valgrind.py:64
> +    return text
> +
> +# Constants that give real names to the abbreviations in valgrind XML output.
> +
> +INSTRUCTION_POINTER = "ip"

I was thinking of the missing newline between the get_CDATA_of return statement and the comment.
No newline required between the comment and the INSTRUCTION_POINTER definition.

> Tools/Scripts/webkitpy/port/leakdetector_valgrind.py:74
> +def gather_frames(node, source_dir):
> +        frame_dict = lambda frame: {
> +            INSTRUCTION_POINTER: get_text_of(frame, INSTRUCTION_POINTER),

The indentation here is off.
Comment 16 Brian Holt 2013-12-09 10:42:00 PST
(In reply to comment #15)
> (From update of attachment 218767 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218767&action=review
> 
> > Tools/Scripts/webkitpy/port/leakdetector_valgrind.py:64
> > +    return text
> > +
> > +# Constants that give real names to the abbreviations in valgrind XML output.
> > +
> > +INSTRUCTION_POINTER = "ip"
> 
> I was thinking of the missing newline between the get_CDATA_of return statement and the comment.
> No newline required between the comment and the INSTRUCTION_POINTER definition.
> 
> > Tools/Scripts/webkitpy/port/leakdetector_valgrind.py:74
> > +def gather_frames(node, source_dir):
> > +        frame_dict = lambda frame: {
> > +            INSTRUCTION_POINTER: get_text_of(frame, INSTRUCTION_POINTER),
> 
> The indentation here is off.

Good catch on both of them: the pep8 style checker didn't get either.
Comment 17 Brian Holt 2013-12-09 10:42:50 PST
dpranke: would you mind taking a look over this patch?
Comment 18 Alejandro G. Castro 2013-12-10 07:45:24 PST
(In reply to comment #13)
> (From update of attachment 218681 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218681&action=review
> 
> So overall it seems like a few small changes and the extra tests and this could get in.  Does the simplicification of --leaks and --wrapper="valgrind" really block this?  Can I do that in a follow-up patch?
> 

Yeah, it is not added with this patch, that behaviour was added in a previous patch.
Comment 19 Brian Holt 2013-12-10 13:26:43 PST
https://bugs.webkit.org/show_bug.cgi?id=125540 created to invoke valgrind with the run-leaks script.
Comment 20 Alejandro G. Castro 2013-12-23 10:44:06 PST
Comment on attachment 218767 [details]
Patch

Brian can you add Zan improvements and upload a new patch?
Comment 21 Brian Holt 2014-01-02 05:27:11 PST
Created attachment 220221 [details]
Patch
Comment 22 Brian Holt 2014-01-03 11:42:27 PST
(In reply to comment #20)
> (From update of attachment 218767 [details])
> Brian can you add Zan improvements and upload a new patch?

Alex, did you see the new patch I uploaded with Zan's changes?  Is there anymore that you think we need or can this be committed?
Comment 23 Alejandro G. Castro 2014-01-08 09:58:46 PST
Comment on attachment 220221 [details]
Patch

LGTM, let's add it. Thanks for the patch.
Comment 24 WebKit Commit Bot 2014-01-08 10:26:14 PST
Comment on attachment 220221 [details]
Patch

Clearing flags on attachment: 220221

Committed r161512: <http://trac.webkit.org/changeset/161512>
Comment 25 WebKit Commit Bot 2014-01-08 10:26:18 PST
All reviewed patches have been landed.  Closing bug.
Comment 26 Alexey Proskuryakov 2014-01-08 12:30:53 PST
This patch broke webkitpy regression tests on Mac.
Comment 27 WebKit Commit Bot 2014-01-08 12:35:13 PST
Re-opened since this is blocked by bug 126655
Comment 28 Zan Dobersek 2014-01-08 13:39:54 PST
I wasn't able to fix all of the introduced problems, so I'll be rolling out the patch shortly.

Here's an example buildbot output that has logged both of the failures:
http://build.webkit.org/builders/Apple%20Mavericks%20Debug%20WK2%20%28Tests%29/builds/1429/steps/webkitpy-test/logs/stdio

I was able to fix one of the failures by enforcing LeakDetectorValgrind use only in the case of leak detection option being enabled:
http://trac.webkit.org/changeset/161517
(This will be rolled out as well.)

The other failure is stranger, the output isn't really helpful, and I have no idea why it would fail.
Comment 29 Alexey Proskuryakov 2014-01-08 19:53:03 PST
Looks like one of the tests still fails after the rollout :(
Comment 30 Alexey Proskuryakov 2014-01-08 19:55:41 PST
Sorry, I was confused - the rollout helped, it's an unrelated *API* failure that we have now.
Comment 31 Alejandro G. Castro 2014-01-09 03:15:23 PST
(In reply to comment #30)
> Sorry, I was confused - the rollout helped, it's an unrelated *API* failure that we have now.

Thanks cleaning the mess ap, I think zan has some proposals of how to fix it.
Comment 32 Brian Holt 2014-01-09 06:32:25 PST
Reopening to attach new patch.
Comment 33 Brian Holt 2014-01-09 06:32:31 PST
Created attachment 220722 [details]
Patch
Comment 34 Brian Holt 2014-01-09 06:38:41 PST
(In reply to comment #33)
> Created an attachment (id=220722) [details]
> Patch

As Zan recommended on IRC, I've changed from using subprocess.Popen to Executive().run_command.  We'll confirm that this work on mac before landing.
Comment 35 Brian Holt 2014-01-10 06:07:49 PST
Reopening to attach new patch.
Comment 36 Brian Holt 2014-01-10 06:07:55 PST
Created attachment 220837 [details]
Updated to use Executive and MockExecutive in the unittest
Comment 37 Zan Dobersek 2014-01-10 09:49:44 PST
Comment on attachment 220837 [details]
Updated to use Executive and MockExecutive in the unittest

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

Looks good, but left one more advice and another request to make the unit test picklable. More details below.

With the pickle thing fixed, the unit tests pass on Mac as well.

> Tools/Scripts/webkitpy/port/leakdetector_valgrind.py:143
> +            buf = ""
> +            for frame in backtrace[1]:
> +                buf += (frame[FUNCTION_NAME] or frame[INSTRUCTION_POINTER]) + "\n"
> +
> +            input = buf.encode('latin-1').split("\n")
> +            demangled_names = [self._executive.run_command(['c++filt', '-n', name]) for name in input if name]
> +
> +            i = 0
> +            for frame in backtrace[1]:
> +                output += ("  " + demangled_names[i])
> +                i = i + 1
> +
> +                if frame[SRC_FILE_DIR] != "":
> +                    output += (" (" + frame[SRC_FILE_DIR] + "/" + frame[SRC_FILE_NAME] +
> +                               ":" + frame[SRC_LINE] + ")")
> +                else:
> +                    output += " (" + frame[OBJECT_FILE] + ")"
> +                output += "\n"

Can this be turned into one iteration over backtrace[1]?

> Tools/Scripts/webkitpy/port/leakdetector_valgrind_unittest.py:820
> +        def mock_run_cppfilt_command(args):
> +            if args[0] == 'c++filt':
> +                return valgrind_output_cppfilt_map[args[2]]
> +            return ""

This should be moved into the top level so it is picklable (i.e. the test case can be serialized and transferred to a different process).
This otherwise throws a PicklingError exception when running the webkitpy tests in parallel.

https://stackoverflow.com/questions/8804830/python-multiprocessing-pickling-error
Comment 38 Brian Holt 2014-01-10 10:29:59 PST
Comment on attachment 220837 [details]
Updated to use Executive and MockExecutive in the unittest

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

>> Tools/Scripts/webkitpy/port/leakdetector_valgrind.py:143
>> +                output += "\n"
> 
> Can this be turned into one iteration over backtrace[1]?

Not sure, but I can try.

>> Tools/Scripts/webkitpy/port/leakdetector_valgrind_unittest.py:820
>> +            return ""
> 
> This should be moved into the top level so it is picklable (i.e. the test case can be serialized and transferred to a different process).
> This otherwise throws a PicklingError exception when running the webkitpy tests in parallel.
> 
> https://stackoverflow.com/questions/8804830/python-multiprocessing-pickling-error

Thanks for pointing that out.  Strangely I noticed that this test runs fine by itself but hangs when run as part of the suite.  Maybe this is the cause...
Comment 39 Brian Holt 2014-01-13 06:17:48 PST
Reopening to attach new patch.
Comment 40 Brian Holt 2014-01-13 06:17:57 PST
Created attachment 221033 [details]
Final version?
Comment 41 Brian Holt 2014-01-13 06:25:26 PST
(In reply to comment #40)
> Created an attachment (id=221033) [details]
> Final version?

Zan, would you mind running the unit tests on Mac?  They should work now that the c++filt output is mocked.
Comment 42 Zan Dobersek 2014-01-13 09:16:00 PST
(In reply to comment #41)
> (In reply to comment #40)
> > Created an attachment (id=221033) [details] [details]
> > Final version?
> 
> Zan, would you mind running the unit tests on Mac?  They should work now that the c++filt output is mocked.

All webkitpy tests on Mac pass.
Comment 43 Brian Holt 2014-01-13 09:54:42 PST
Created attachment 221057 [details]
Diff from previous r+'ed patch
Comment 44 Alejandro G. Castro 2014-01-13 10:23:00 PST
Comment on attachment 221033 [details]
Final version?

Thanks Zan for the help with the patch, let's try it again, recall to land it manually and wait for the bots to show you a happy face ;).
Comment 45 Brian Holt 2014-01-14 03:48:02 PST
Committed r161956: <http://trac.webkit.org/changeset/161956>
Comment 46 Brian Holt 2014-01-14 05:57:19 PST
All tests passed on Mac.  There are some new layout failures that are unrelated to this change.