1441287 - [mozcrash] Add support for unicode paths on Windows

Status: RESOLVED FIXED

(Testing :: Mozbase, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Mozcrash fails on Windows to process minidump files for profiles which have unicode characters included:


 0:52.73 mozcrash INFO Copy/paste: c:/mozilla/win32-minidump_stackwalk.exe c:\us
ers\mozauto\appdata\local\temp\tmpy5bgbl.$¢€🍪\minidumps\353cdea5-2763-49e4
-8269-de83718b5769.dmp c:\users\mozauto\appdata\local\temp\tmp9uoaon
Traceback (most recent call last):
  File "f:\code\gecko\testing/mozbase/mozrunner\mozrunner\base\runner.py", line
228, in check_for_crashes
    test=test_name)
  File "f:\code\gecko\testing/mozbase/mozcrash\mozcrash\mozcrash.py", line 128,
in log_crashes
    stackwalk_binary=stackwalk_binary):
  File "f:\code\gecko\testing/mozbase/mozcrash\mozcrash\mozcrash.py", line 214,
in __iter__
    rv = self._process_dump_file(path, extra)
  File "f:\code\gecko\testing/mozbase/mozcrash\mozcrash\mozcrash.py", line 258,
in _process_dump_file
    stderr=subprocess.PIPE
  File "c:\mozilla-build\python\Lib\subprocess.py", line 710, in __init__
    errread, errwrite)
  File "c:\mozilla-build\python\Lib\subprocess.py", line 958, in _execute_child
    startupinfo)
UnicodeEncodeError: 'ascii' codec can't encode characters in position 87-90: ord
inal not in range(128)

This occurs because not all list entries of command are Unicode.

Comment 1

[(not currently active) Ted Mielczarek]

I *think* if we converted each element of `command` to unicode this would work:
https://dxr.mozilla.org/mozilla-central/rev/7208b6a7b11c3ed8c87a7f17c9c30a8f9583e791/testing/mozbase/mozcrash/mozcrash/mozcrash.py#248

...but I'm fuzzy on how to properly deal with values that come from the environment in Python, like `stackwalk_binary` there.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Using a list comprehension like [arg.encode("UTF-8") for arg in command] it works just fine. I will check more in detail tomorrow what else will be affected, and get some more unit tests added.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1441287 - [mozcrash] Refactor unit tests.

Review commit: https://reviewboard.mozilla.org/r/229250/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229250/

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1441287 - [mozcrash] check_for_crashes should always return count of crashes.

Right now if no minidump file is present in the minidump folder,
the check_for_crashes method returns False. Whereby in all other
cases the number of crashes is returned.

To be consistent this method should always return a number, and
in case of no minidumps it should be 0.

Review commit: https://reviewboard.mozilla.org/r/229252/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229252/

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1441287 - [mozcrash] Convert unit tests to pytest.

Review commit: https://reviewboard.mozilla.org/r/229254/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229254/

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1441287 - [mozcrash] Add support for unicode paths.

Review commit: https://reviewboard.mozilla.org/r/229256/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229256/

Comment 7

[Code Review Bot [:reviewbot]]

Comment on attachment 8960515 [details]
Bug 1441287 - [mozcrash] Convert unit tests to pytest.

https://reviewboard.mozilla.org/r/229254/#review235050


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/mozbase/mozcrash/tests/conftest.py:15
(Diff revision 1)
> +
> [e-mail](scope='session')
> +def stackwalk(tmpdir_factory):
> +    stackwalk = tmpdir_factory.mktemp('stackwalk_binary').join('stackwalk')
> +    stackwalk.write('fake binary')
> +    stackwalk.chmod(0744)

Error: Invalid token [py3: is-parseable]

Comment 8

[Code Review Bot [:reviewbot]]

Comment on attachment 8960516 [details]
Bug 1441287 - [mozcrash] Add support for unicode paths.

https://reviewboard.mozilla.org/r/229256/#review235052


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/mozbase/mozcrash/tests/test_basic.py:73
(Diff revision 1)
>  
> +def tst_stackwalk_unicode(check_for_crashes, minidump_files, tmpdir, capsys):
> +    """Test that check_for_crashes can handle unicode in dump_directory."""
> +    stackwalk = tmpdir.mkdir(u"

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8960513 [details]
Bug 1441287 - [mozcrash] Refactor unit tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229250/diff/1-2/

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8960514 [details]
Bug 1441287 - [mozcrash] check_for_crashes should always return count of crashes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229252/diff/1-2/

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8960515 [details]
Bug 1441287 - [mozcrash] Convert unit tests to pytest.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229254/diff/1-2/

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8960516 [details]
Bug 1441287 - [mozcrash] Add support for unicode paths.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229256/diff/1-2/

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8960515 [details]
Bug 1441287 - [mozcrash] Convert unit tests to pytest.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229254/diff/2-3/

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8960516 [details]
Bug 1441287 - [mozcrash] Add support for unicode paths.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229256/diff/2-3/

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8960516 [details]
Bug 1441287 - [mozcrash] Add support for unicode paths.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229256/diff/3-4/

Comment 16

[Andrew Halberstadt [:ahal]]

Comment on attachment 8960513 [details]
Bug 1441287 - [mozcrash] Refactor unit tests.

https://reviewboard.mozilla.org/r/229250/#review235486

Comment 17

[Andrew Halberstadt [:ahal]]

Comment on attachment 8960514 [details]
Bug 1441287 - [mozcrash] check_for_crashes should always return count of crashes.

https://reviewboard.mozilla.org/r/229252/#review235488

Comment 18

[Andrew Halberstadt [:ahal]]

Comment on attachment 8960515 [details]
Bug 1441287 - [mozcrash] Convert unit tests to pytest.

https://reviewboard.mozilla.org/r/229254/#review235492

Thanks, this is a good cleanup!

Comment 19

[Andrew Halberstadt [:ahal]]

Comment on attachment 8960516 [details]
Bug 1441287 - [mozcrash] Add support for unicode paths.

https://reviewboard.mozilla.org/r/229256/#review235498

This makes me a little nervous, but looks good to me. At least be sure to do a build and run:
./mach python-test testing/mochitest

There's a test in there that makes sure crashes still work.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8960516 [details]
Bug 1441287 - [mozcrash] Add support for unicode paths.

https://reviewboard.mozilla.org/r/229256/#review235498

All fine here:

> testing/mochitest/tests/python/test_basic_mochitest_plain.py::test_output_crash PASSED

I would have wondered if something got broken here. I checked each and every change to mozcrash very carefully, and got the new tests added. Thanks for the reviews!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e7e21c4d3e2
[mozcrash] Refactor unit tests. r=ahal
https://hg.mozilla.org/integration/autoland/rev/5082e91767c2
[mozcrash] check_for_crashes should always return count of crashes. r=ahal
https://hg.mozilla.org/integration/autoland/rev/fef8c2970f6a
[mozcrash] Convert unit tests to pytest. r=ahal
https://hg.mozilla.org/integration/autoland/rev/bb4716acbe83
[mozcrash] Add support for unicode paths. r=ahal

Comment 22

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/6e7e21c4d3e2
https://hg.mozilla.org/mozilla-central/rev/5082e91767c2
https://hg.mozilla.org/mozilla-central/rev/fef8c2970f6a
https://hg.mozilla.org/mozilla-central/rev/bb4716acbe83