Bug 155000 - webkitpy should verify timestamp from CrashLogs while collecting all crash logs
Summary: webkitpy should verify timestamp from CrashLogs while collecting all crash logs
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aakash Jain
Depends on:
Reported: 2016-03-03 18:49 PST by Aakash Jain
Modified: 2016-03-07 13:40 PST (History)
6 users (show)

See Also:

Proposed patch (2.29 KB, patch)
2016-03-03 18:57 PST, Aakash Jain
ap: review-
ap: commit-queue-
Details | Formatted Diff | Diff
Updated Patch (3.88 KB, patch)
2016-03-05 20:44 PST, Aakash Jain
ap: review+
Details | Formatted Diff | Diff
Updated patch (3.82 KB, patch)
2016-03-07 12:13 PST, Aakash Jain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aakash Jain 2016-03-03 18:49:53 PST
Sometimes "Other crashes" section in the layout-tests results contains crashes that actually happened hours before. It might be OS regression. We should verify timestamps in crash logs for robustness.
see <rdar://problem/24860219>
Comment 1 Aakash Jain 2016-03-03 18:57:32 PST
Created attachment 272815 [details]
Proposed patch
Comment 2 Alexey Proskuryakov 2016-03-05 15:18:49 PST
Comment on attachment 272815 [details]
Proposed patch

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

> Tools/ChangeLog:10
> +        (CrashLogs._is_new_crash_log): Check if crash logs are newer than required.

I would say: "Make sure that crash log timestamp is within expected time range, because file modification time is not always accurate".

> Tools/Scripts/webkitpy/common/system/crashlogs.py:165
> +    def _is_new_crash_log(self, log_contents, newer_than=None):

Do we ever want to call this without a newer_than argument? It's confusing to have it optional when the function makes no sense without it.

Just seeing the function name, someone could imagine that this is about crash log format (new vs. old). Maybe something like "_is_timestamp_newer_than" would be better?

Another idea: instead of _is_new_crash_log, add a function that returns internal timestamp (possibly as datetime), and then the caller can compare it to "newer_than". There is no need to combine extracting the timestamp with a policy decision of what it should be.
Comment 3 Aakash Jain 2016-03-05 20:44:36 PST
Created attachment 273123 [details]
Updated Patch

Updated patch with review comments incorporated.
Comment 4 Alexey Proskuryakov 2016-03-07 12:03:52 PST
Comment on attachment 273123 [details]
Updated Patch

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

> Tools/Scripts/webkitpy/common/system/crashlogs.py:170
> +    def get_timestamp_from_logs(self, log_contents):

Should this be "get_timestamp_from_logs" (singular)?
Comment 5 Aakash Jain 2016-03-07 12:13:31 PST
Created attachment 273201 [details]
Updated patch
Comment 6 WebKit Commit Bot 2016-03-07 12:45:50 PST
Comment on attachment 273201 [details]
Updated patch

Rejecting attachment 273201 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 273201, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Tools/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/937825
Comment 7 WebKit Commit Bot 2016-03-07 13:40:27 PST
Comment on attachment 273201 [details]
Updated patch

Clearing flags on attachment: 273201

Committed r197704: <http://trac.webkit.org/changeset/197704>
Comment 8 WebKit Commit Bot 2016-03-07 13:40:30 PST
All reviewed patches have been landed.  Closing bug.