Bug 63468 - webkitpy should understand crash logs
Summary: webkitpy should understand crash logs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on: 63619
Blocks: 55907
  Show dependency treegraph
 
Reported: 2011-06-27 10:43 PDT by Adam Barth
Modified: 2011-06-29 04:10 PDT (History)
2 users (show)

See Also:


Attachments
Patch (11.66 KB, patch)
2011-06-27 10:45 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (11.79 KB, patch)
2011-06-27 11:16 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (6.35 KB, patch)
2011-06-27 11:29 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (11.81 KB, patch)
2011-06-27 11:36 PDT, Adam Barth
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2011-06-27 10:43:26 PDT
webkitpy should understand crash logs
Comment 1 Adam Barth 2011-06-27 10:45:48 PDT
Created attachment 98748 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-06-27 10:56:13 PDT
Comment on attachment 98748 [details]
Patch

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

> Tools/Scripts/webkitpy/common/system/crashlog.py:44
> +        if sys.platform == "darwin":
> +            self._capture_log_darwin(process_name)

Its funny how initing one of these does the search.  I think I would have called this thing CrashLogs and had a most_recent_log(process_name=) function instead.

> Tools/Scripts/webkitpy/common/system/crashlog.py:58
> +        log_directory = self._filesystem.expanduser("~")
> +        log_directory = os.path.join(log_directory, "Library", "Logs")
> +        if self._filesystem.exists(os.path.join(log_directory, "DiagnosticReports")):
> +            log_directory = os.path.join(log_directory, "DiagnosticReports")
> +        else:
> +            log_directory = os.path.join(log_directory, "CrashReporter")

I would have made this a separate function.

> Tools/Scripts/webkitpy/common/system/executive.py:301
> +        ps_process = self.popen(['ps', '-eo', 'pid,comm'], stdout=self.PIPE, stderr=self.PIPE)

Really? Python doesn't offer us a better way than running ps?  I guess not: http://ubuntuforums.org/showthread.php?t=463850

> Tools/Scripts/webkitpy/common/system/executive.py:311
> +        return running_pids

Do we need to sort these?  Might add a comment which explains why we don't.  Does this handle pid-wrapping? do we care?

> Tools/Scripts/webkitpy/common/system/executive.py:323
> +        while self.check_running_pid(pid):
> +            time.sleep(0.25)

Can't we just waitpid?  I guess on unix it has to be a child process?  I wonder if the same restriction applies to windows?
Comment 3 Adam Barth 2011-06-27 11:15:51 PDT
(In reply to comment #2)
> (From update of attachment 98748 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98748&action=review
> 
> > Tools/Scripts/webkitpy/common/system/crashlog.py:44
> > +        if sys.platform == "darwin":
> > +            self._capture_log_darwin(process_name)
> 
> Its funny how initing one of these does the search.  I think I would have called this thing CrashLogs and had a most_recent_log(process_name=) function instead.

I'm not sure I fully understand.  You're saying we should use a free function instead of a constructor?  What would the CrashLog class have in it?

> > Tools/Scripts/webkitpy/common/system/crashlog.py:58
> > +        log_directory = self._filesystem.expanduser("~")
> > +        log_directory = os.path.join(log_directory, "Library", "Logs")
> > +        if self._filesystem.exists(os.path.join(log_directory, "DiagnosticReports")):
> > +            log_directory = os.path.join(log_directory, "DiagnosticReports")
> > +        else:
> > +            log_directory = os.path.join(log_directory, "CrashReporter")
> 
> I would have made this a separate function.

Done.

> > Tools/Scripts/webkitpy/common/system/executive.py:301
> > +        ps_process = self.popen(['ps', '-eo', 'pid,comm'], stdout=self.PIPE, stderr=self.PIPE)
> 
> Really? Python doesn't offer us a better way than running ps?  I guess not: http://ubuntuforums.org/showthread.php?t=463850

This is what the internets recommended.

> > Tools/Scripts/webkitpy/common/system/executive.py:311
> > +        return running_pids
> 
> Do we need to sort these?  Might add a comment which explains why we don't.  Does this handle pid-wrapping? do we care?

I've sorted it.  I'm sure it matters that much.  I suspect that we'll have some race conditions because of parallelism, but we'll have to deal with them when they occur.

> > Tools/Scripts/webkitpy/common/system/executive.py:323
> > +        while self.check_running_pid(pid):
> > +            time.sleep(0.25)
> 
> Can't we just waitpid?  I guess on unix it has to be a child process?  I wonder if the same restriction applies to windows?

Under the covers, check_running_pid does what the perl code used to do and what the Internet recommends.  The perl code specifically has a comment about not using waitpid for this purpose.
Comment 4 Adam Barth 2011-06-27 11:16:39 PDT
Created attachment 98751 [details]
Patch
Comment 5 Adam Barth 2011-06-27 11:29:48 PDT
Created attachment 98757 [details]
Patch
Comment 6 Eric Seidel (no email) 2011-06-27 11:35:27 PDT
Comment on attachment 98757 [details]
Patch

Missing your new file. ;(
Comment 7 Adam Barth 2011-06-27 11:36:47 PDT
Created attachment 98759 [details]
Patch
Comment 8 Eric Seidel (no email) 2011-06-27 11:41:19 PDT
Comment on attachment 98759 [details]
Patch

LGTM.
Comment 9 Adam Barth 2011-06-27 11:47:49 PDT
Committed r89843: <http://trac.webkit.org/changeset/89843>