[mozprofile] If no path for profile given mozprofile should include non-ascii characters into the path

NEW
Unassigned

Status

Testing
Mozbase
5 years ago
3 months ago

People

(Reporter: whimboo, Unassigned)

Tracking

(Depends on: 6 bugs)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

As seen on bug 902532 we had a regression in the spellchecker in the final release of Firefox. That happened because some non-ascii characters were included in the profile name. 

To prevent those major regressions we should update mozprofile so it automatically adds some non-ascii characters to the profile path by default if the consumer hasn't been specified any location.

Benjamin proposed to have such an option in our automation frameworks. And given that mochitests will be based on mozbase soon (see bug 746243) this would be a great feature.

Will, Jeff already agreed with that proposal via a mail thread, but wanted to get your opinion first. What do you think of it?
Flags: needinfo?(wlachance)
I'd support at least doing this optionally (you can probably pass it in as an argument to Profile's __init__  method, then down to create_new_profile). At this point, you should be able to make mozmill use that setting.

I'd worry that making this the default might cause problems with *automation* that isn't expecting the non-ascii characters, so I'd prefer to avoid that.
Flags: needinfo?(wlachance)
(In reply to William Lachance (:wlach) from comment #1)
> I'd worry that making this the default might cause problems with
> *automation* that isn't expecting the non-ascii characters, so I'd prefer to
> avoid that.

We might want to do that as the second step given that mozbase is used by mochitests soon. How does it sound to you? Means I would file a follow-up bug and get the optional parameter in for now.
Given all the troubles we have had in the past with profiles containing non-ASCII characters I think we should force using at least one unicode character by default. Also because this will increase the test coverage a lot, additionally to bug 1423897.

We indeed might see issues in automation, but those should only happen due to bugs in Firefox. As I checked with Marionette yesterday and on bug 1438035, our mozbase packages are working just fine with unicode paths.

I think that I will just submit a full try job to check our status.

Ted, what do you think?
Flags: needinfo?(ted)
Comment hidden (mozreview-request)
As the try job (https://treeherder.mozilla.org/#/jobs?repo=try&revision=684bd5606e3547152b6061f2651b0d28e1c29ce5&selectedJob=162374068) shows a couple of harnesses would need an update to be able to cope with unicode paths.

If my proposal seems worthwhile I can clearly have a look at and fix that.
So if we would make such a change we actually break `mozprofile.diff()` when using the default `diff_function` which is `difflib.unified_diff()`. Reason is that this method is not unicode compatible. At least in Python 2.7.
I'm not opposed to this idea, and it would increase our test coverage. If this winds up requiring a ton of work on other things then I'm not sure it's worthwhile. If making it the *default* is hard, maybe we could just make our harnesses individually use a profile path with Unicode characters?
Flags: needinfo?(ted)
Lets see what we can do. First I will make sure to make our related mozbase packages unicode safe on Windows. So important in that regard would be mozcrash (bug 1441287). I will add more dependencies when necessary.
Depends on: 1441287
Depends on: 1451062
Depends on: 1452864
Depends on: 1451255
Depends on: 1452957
Depends on: 1453596
Depends on: 1453647
Now that I can run reftests and mochitest locally I pushed another try build to see how test results look on TC:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1df3f7469bfbcd391e8e1ef5128b24c5b45fe7ba
Depends on: 1453945

Updated

3 months ago
Depends on: 1454938

Updated

3 months ago
Depends on: 1454943
Depends on: 1457399
You need to log in before you can comment on or make changes to this bug.