[Webkit-unassigned] [Bug 81659] [GTK] crash log reports support

[bugzilla-daemon at webkit.org]

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





--- Comment #7 from Martin Robinson <mrobinson at webkit.org>  2012-03-27 14:52:14 PST ---
(From update of attachment 134094)
View in context: https://bugs.webkit.org/attachment.cgi?id=134094&action=review

Looks good, but I'd rather someone more familiar with the crash dumping code do the final review. I have left some comments below.

> Tools/BuildSlaveSupport/gtk/README:-23
> -==============================================
> - Running a GTK+ build slave under daemontools
> -==============================================
> -
> -This directory contains several scripts which can be used to run a WebKitGTK+
> -build slave under daemontools [1]. This is convenient because daemontools
> -will automatically restart services when they die, and that means less human
> -intervention is needed.
> -
> -
> -Dependencies
> -============
> -
> -In order to use the provided service control files, you will need the
> -following:
> -
> -* The GNU Bash shell (the scripts contain some bash-isms)
> -
> -* The daemontools package (or one of its drop-in replacements, like runit
> -  or freedt; but only daemontools has been tested so far).
> -
> -* The crash dump monitor also uses "inotifywait" (part of inotify-tools [2])
> -

Did you mean to remove all of this README? Not all of it deals with crashmon.

> Tools/Scripts/new-run-webkit-tests:61
> +        jhbuild = os.path.join(script_dir, '..', 'gtk', 'run-with-jhbuild')
> +        cmd.insert(1, jhbuild)

This can be one line.

> Tools/Scripts/webkitpy/layout_tests/port/gtk.py:156
> +        name_str = name or '<unknown process name>'
> +        pid_str = str(pid or '<unknown>')

Okay, just a nit but you should use full words, unless the abbreviation would be more canonical. So IMO pid is good and str is bad.

In what cases is pid == None? If pid can be None and name can be None shouldn't you handle that case explicitly and do an early return?

> Tools/Scripts/webkitpy/layout_tests/port/gtk.py:164
> +        expected_crash_dump_filename = "core-pid_%s-_-process_%s" % (pid_str, name_str)
> +        if pid:
> +            match_filename = lambda name: name == expected_crash_dump_filename
> +        else:
> +            match_filename = lambda name: name.find(name_str) > -1

I think would be more clear as:

def match_filename(name):
    if pid:
        return name == expected_crash_dump_filename
    return name.find(name_str) > -1

If it's frequent that pid is None, wouldn't it be better to simply find the dump with the most recent timestamp?

> Tools/Scripts/webkitpy/layout_tests/port/gtk.py:166
> +        for path in reversed(sorted(dumps)):

Perhaps the sorting here ensures that the most recent dump is selected? I'm just guessing, so maybe it deserves a comment.

> Tools/Scripts/webkitpy/layout_tests/port/gtk.py:170
> +        stderr_lines = errors + (stderr or '<empty>').decode('utf8').splitlines()

What guarantee do we have that stderr is in UTF-8 format?

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