Bug 160198 – Crash Log Clobbering on Unbound Crashes

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

Fix from r27472618

[Jonathan Bedard]

Created attachment 284599 [details]
Patch

[Jonathan Bedard]

Note that this is a temporary fix which needs to be applied to prevent clobbering of crash logs on bots.  Long term, there is a refactor needed in much of the port-specific code in the testing scripts.

[Aakash Jain]

Comment on attachment 284599 [details]
Patch

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

mostly looks good to me.

> Tools/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=160198

You might consider including the radar number here. e.g.: <rdar://problem/27472618>

> Tools/ChangeLog:11
> +        (TestResultWriter.output_filename): Only remove file extension if it is clearly a file extension

Nit: end the line with a '.'

> Tools/Scripts/webkitpy/common/system/crashlogs.py:34
>  class CrashLogs(object):

should have two blank lines before class. I wonder why style check did not catch this.

> LayoutTests/ChangeLog:5
> +

Ditto.

> LayoutTests/ChangeLog:8
> +        * fast/harness/results.html: Only remove file extension if it is clearly a file extension

Ditto.

> LayoutTests/fast/harness/results.html:275
> +    if (splitExtension(test)[1].length > 5)

This logic doesn't match with corresponding python code. splitExtension() returns the exact number of characters in extension, while fs.splitext() in python returns one extra character (counting '.').  For e.g. for "test.html" splitExtension(test)[1].length returns 4, and len(fs.splitext(output_filename)[1]) returns 5.

[Jonathan Bedard]

Created attachment 284614 [details]
Patch

[Aakash Jain]

Comment on attachment 284614 [details]
Patch

looks good to me. I probably can't r+, so Alexey or someone has to approve it.

[WebKit Commit Bot]

Comment on attachment 284614 [details]
Patch

Rejecting attachment 284614 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[Alexey Proskuryakov]

Comment on attachment 284614 [details]
Patch

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

Looks good to me.

> Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:93
> +        if len(fs.splitext(output_filename)[1]) - 1 > 5:
> +            return output_filename + modifier

Shouldn't we check the extension for test name, not the full path?

Could you please add a comment explaining why this is needed and why it's temporary? Something like:

# self._test_name can contain either a test name, or <...> (e.g. test-name.txt or com.apple.WebKit.WebContent.Development.123).
# FIXME: Refactor to avoid using this variable inconsistently.

[Alexey Proskuryakov]

Comment on attachment 284614 [details]
Patch

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

> LayoutTests/fast/harness/results.html:276
> +    if (splitExtension(test)[1].length > 5)
> +        return test;

Also both places needs to be cross-referenced in comments, so that we don't forget to update them at the same time.

[Jonathan Bedard]

Created attachment 284655 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 284655 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:94
> +        # FIXME: Refactor to avoid variable inconsistency.

I'm not sure if "variable inconsistency" explains the issue.

I'd say something like "FIXME: Refactor to avoid using self._test_name for both test names and process names."

[Jonathan Bedard]

Created attachment 284810 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 284810 [details]
Patch

Clearing flags on attachment: 284810

Committed r203902: <http://trac.webkit.org/changeset/203902>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.