Bug 180828 – Improve leaks detector output

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

Fixing two issues:
1. run-leaks omits many lines from leaks tool output, making it incompatible with other tools. Notably, symbolication cannot be performed.
2. run-leaks output goes to "run-webkit-tests --debug-rwt-logging" output. This makes it much longer than needed, sometimes even overloading buildbot. We don't need full output in test log, as separate files are created for each of these.

[Alexey Proskuryakov]

Created attachment 329382 [details]
proposed patch

[Alexey Proskuryakov]

Comment on attachment 329382 [details]
proposed patch

Need to make tests pass, and also need to delete files that show 0 leaks.

[Alexey Proskuryakov]

Created attachment 329392 [details]
proposed patch

[Joseph Pecoraro]

Comment on attachment 329382 [details]
proposed patch

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

> Tools/Scripts/run-leaks:154
> +        if ($line =~ /^Process [0-9]+: [0-9]+ leaks?/) {
> +            ($leakCount) = ($line =~ /[[:blank:]]+([0-9]+)[[:blank:]]+leaks?/);
> +            if (!defined($leakCount)) {
> +                reportError("Could not parse leak count reported by leaks tool.");
> +                return;
> +            }
> +        }

Nit: In these regular expressions `[0-9]+` can be `\d+`, and `[[:blank:]]+` can be `\s+` which I think is more normal. Up to you.

Either way you also don't need to run the regex twice, in multiple differing formats, and I don't think the !defined($leakCount) branch can ever be reached because of the check on line 148.

This can be simplified to:

    if ($line =~ /^Process \d+: (\d+) leaks?/) {
        $leakCount = $1;
        next;
    }

> Tools/Scripts/run-leaks:164
>              if (!defined($address)) {

There is code in this section that checks the type:

    175            my ($type) = ($line =~ /'([^']+)'/); #'
    176            if (!defined($type)) {
    177                $type = ""; # The leaks tool sometimes omits the type.
    178            }

This looks wrong to me. I have never seen types quoted in single quotes in leaks output. Some example lines (from a bunch of leaks logs) are:

    Leak: 0x60400003a9e0  size=32  zone: DefaultMallocZone_0x102a98000
    Leak: 0x60800003a8e0  size=32  zone: DefaultMallocZone_0x102a98000   NSArray  ObjC  CoreFoundation
    Leak: 0x7ffd93d02a80  size=48  zone: DefaultMallocZone_0x102971000   CFSet  ObjC  CoreFoundation
    Leak: 0x7ffd93d02ab0  size=32  zone: DefaultMallocZone_0x102971000   CFDictionary (Value Storage)  C  CoreFoundation
    Leak: 0x7ffd94827880  size=80  zone: DefaultMallocZone_0x102971000   CFData  ObjC  CoreFoundation
    Leak: 0x7ffd9482b0d0  size=32  zone: DefaultMallocZone_0x102971000   NSMutableArray (Storage)  C  CoreFoundation
    Leak: 0x7ffd94832990  size=240  zone: DefaultMallocZone_0x102971000
    Leak: 0x6040000a47a0  size=96  zone: DefaultMallocZone_0x102a98000   AEDescImpl  C++  AE
    Leak: 0x7fae1f339540  size=64  zone: DefaultMallocZone_0x10ee3a000   __NSMallocBlock__  ObjC  CoreFoundation  AppKit  __68-[NSScrollingBehaviorLegacy _latchMomentumScrollEventsToScrollView:]_block_invoke  0x7fff32740fc6
    Leak: 0x7f87fff06890  size=48  zone: WebKit Using System Malloc_0x1115f9000   WebCore::StyleRuleCSSStyleDeclaration  C++  WebCore

Perhaps this should be updated to:

    # The type is the first value after the zone that is surrounding by double spaces.
    my ($type) = ($line =~ /zone: .*?\s*  (.*?)  /);
    if (!defined($type)) {
        $type = ""; # The leaks tool sometimes omits the type.
    }

Which in my quick tests on the above lines produces what I would expect:

    TYPE: 
    TYPE: NSArray
    TYPE: CFSet
    TYPE: CFDictionary (Value Storage)
    TYPE: CFData
    TYPE: NSMutableArray (Storage)
    TYPE: 
    TYPE: AEDescImpl
    TYPE: __NSMallocBlock__
    TYPE: WebCore::StyleRuleCSSStyleDeclaration

> Tools/Scripts/webkitpy/port/leakdetector.py:120
>              # Oddly enough, run-leaks (or the underlying leaks tool) does not seem to always output utf-8,

I'm guessing this is when dumping string contents.

[Joseph Pecoraro]

> This can be simplified to:
> 
>     if ($line =~ /^Process \d+: (\d+) leaks?/) {
>         $leakCount = $1;
>         next;
>     }

Err, without the `next` so that the line gets included in output!

[Joseph Pecoraro]

Comment on attachment 329392 [details]
proposed patch

r=me but all my earlier comments still apply. And could be tested as well (especially Type, since I think that is totally wrong and will never work right now).

[Joseph Pecoraro]

>     # The type is the first value after the zone that is surrounding by
> double spaces.
>     my ($type) = ($line =~ /zone: .*?\s*  (.*?)  /);
>     if (!defined($type)) {
>         $type = ""; # The leaks tool sometimes omits the type.
>     }

Technically it looks like it is always preceded by 3 spaces. So the regex could be

    /zone: .*?\s*  (.*?)  /
    /zone: .*?   (.*?)  /

... either of these, with an appropriate comment.

[Alexey Proskuryakov]

Isn't \d unnecessarily permissive in Perl? I think that it includes everything in the Unicode class.

I didn't want to touch the pre-existing regexes, but I guess we can change those now as well.

[Joseph Pecoraro]

(In reply to Alexey Proskuryakov from comment #8)
> Isn't \d unnecessarily permissive in Perl? I think that it includes
> everything in the Unicode class.

Hmm, I don't believe so. Colloquially `\d` is "numbers". Even if it was overly permissive that normally isn't a problem if you know the input you're parsing is simple, so using `\d` tends to be the norm. (And also its supported pretty much everywhere).

Based on the chart in here, the posix [:digit:] should match Perl's shorthand \d:
https://www.regular-expressions.info/posixbrackets.html

> I didn't want to touch the pre-existing regexes, but I guess we can change
> those now as well.

I only brought it up in these cases because:

  • Leak Count input was parsed twice, with different regexes, unnecessarily
  • Leak Type input was parsed incorrectly and looks like it would never work

For example the cases with [:xdigit:] seem worth keeping as is.

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/r225937>. Thank you for the review!

I only added a fixme for the type problem, as it's orthogonal to what this patch solves, and doesn't affect what the tools produce (as we don't skip any types currently).

I'm not sure if I agree with the proposed regex that starts with "zone" - it feels like three spaces are actually the marker. But I would need to dig into more examples to understand what the forward compatibility story for this format is, if any.

[Radar WebKit Bug Importer]

<rdar://problem/36060251>