Closed Bug 1441287 Opened 6 years ago Closed 6 years ago

[mozcrash] Add support for unicode paths on Windows

Categories

(Testing :: Mozbase, defect, P1)

Product:
Testing
Component:
Mozbase
Version:
Version 3
Platform:
All
Windows
Type:
defect

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Blocks 2 open bugs)

Details

Attachments

(4 files)

Bug 1441287 - [mozcrash] Refactor unit tests.
59 bytes, text/x-review-board-request
ahal
: review+
Details
Bug 1441287 - [mozcrash] check_for_crashes should always return count of crashes.
59 bytes, text/x-review-board-request
ahal
: review+
Details
Bug 1441287 - [mozcrash] Convert unit tests to pytest.
59 bytes, text/x-review-board-request
ahal
: review+
Details
Bug 1441287 - [mozcrash] Add support for unicode paths.
59 bytes, text/x-review-board-request
ahal
: review+
Details 
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.
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.
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.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: -- → P1
Blocks: 918097
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)
> +
> +@pytest.fixture(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 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"
Attachment #8960513 - Flags: review?(ahalberstadt)
Attachment #8960514 - Flags: review?(ahalberstadt)
Attachment #8960515 - Flags: review?(ahalberstadt)
Attachment #8960516 - Flags: review?(ahalberstadt)
Comment on attachment 8960513 [details]
Bug 1441287 - [mozcrash] Refactor unit tests.

https://reviewboard.mozilla.org/r/229250/#review235486
Attachment #8960513 - Flags: review?(ahalberstadt) → review+
Comment on attachment 8960514 [details]
Bug 1441287 - [mozcrash] check_for_crashes should always return count of crashes.

https://reviewboard.mozilla.org/r/229252/#review235488
Attachment #8960514 - Flags: review?(ahalberstadt) → review+
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!
Attachment #8960515 - Flags: review?(ahalberstadt) → review+
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.
Attachment #8960516 - Flags: review?(ahalberstadt) → review+
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!
Pushed by hskupin@mozilla.com:
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
Depends on: 1451319
Blocks: 1397417
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: