1423897 - Add (more) automated test to prevent startup fallouts due to non-ASCII characters in the path

Status: NEW

(Toolkit :: Startup and Profile System, enhancement, P3)

Description

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

Several bugs in the past months have shown that our test coverage for profile paths including special (non-ASCII) characters isn't that great. Having to include fixes like bug 1420427 for a point release is not something which should come up in the future again.

I'm not sure what the best test suite will be, but I'm going to investigate that now.

An earlier idea (bug 918097) was to include some random characters when using mozprofile to create the profile in any of the existent test harnesses which make use of mozbase packages. Given that xpcshell (bug 1392308) and mochitest (bug 1395414) use it, those might be dupes.

But maybe we should have specific tests for known variants which broke Firefox so far too.

I queried Bugzilla to get a sample of what kind of issues we had, and all of those have been added to the dependency list.

Comment 1

[Masatoshi Kimura [:emk]]

We will have to fix some existing bugs before doing this.

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

\o/

Comment 3

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

(In reply to Masatoshi Kimura [:emk] from comment #1)
> We will have to fix some existing bugs before doing this.

I would like to do some investigation already. So do you know if there are already some tests present? Otherwise I will have to do some querying next week to get an overview about the current coverage.

Comment 4

[Masatoshi Kimura [:emk]]

(In reply to Henrik Skupin (:whimboo) from comment #3)
> I would like to do some investigation already. So do you know if there are
> already some tests present?

None at hand. I'll add dependency when I found it.

Comment 5

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

Before I add a strategy to what and how we could test, here a list of tests as currently run for profile creation and locking:

gTest:
https://searchfox.org/mozilla-central/source/toolkit/profile/gtest/TestProfileLock.cpp
https://searchfox.org/mozilla-central/source/toolkit/profile/gtest/TestProfileLockRetry.cpp (Linux only)

Mochitest Chrome:
https://searchfox.org/mozilla-central/source/toolkit/profile/test/test_create_profile.xul

I was not able to find any other test related to profile paths and variations of encodings. Ted, do you know something?

Comment 6

[(not currently active) Ted Mielczarek]

I'm not aware of any, although I've certainly filed some bugs about this in the past, like bug 396059. Now that all our testing is running via Taskcluster this should be much simpler to achieve.

Comment 7

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

Here a summary of what we would need as collected by known and already fixed bugs:

* Unicode characters in the path component
* A mixture of short (8.3) and long path components: C:\longpa~1\longpathname\ (Windows only)
* Occurred for both the profile path, but also for the install path

Are there any other specific encodings which we should take into account here? Would it make sense to split up the tests for eg. UTF-8, UTF-16, Windows-1252, ...?

Given that a couple of bugs were present after a restart of Firefox, it will not be enough to just create such a profile and start Firefox once, but also to make sure that everything still works after a restart. I would not differentiate between restart vs. quit + start.

Here some known affected components from the past:

* Add-ons Manager: Installed add-ons were not visible in the toolbar, menu, or customize palette after a restart (eg. uBlock) while the add-on was still listed as enabled. Add-on had to be disabled and enabled to make it work again.

* Preferences: Modified preferences in about:preferences have been reset to their default values after a restart of Firefox.

* Spell checker: Inserted text into any input field and using the spell checker underlined all but not only the mis-spelled words.

* Bookmarks: Bookmarks are empty, which also resulted in not displaying the start page.

* Tabs: Not being able to open/close any tabs

The following list wasn't affected yet by this problem but it might be good to also check those components for correctness. Those are all more or less read/write data from/to files to the profile:

- certificate overrides (cert_override.txt)
- content preferences (content-prefs.sqlite)
- cookies (cookies.sqlite)
- form data (formhistory.sqlite, autofill-profiles.json)
- mime types (mimetypes.rdf)
- passwords (logins.json, signons.sqlite)
- permissions (permissions.sqlite)
- search engines (search.json.mozlz4, searchplugins/)
- sessionstore (previous.js)
- signins (signedInUser.json)
- storage
- telemetry
- xulstore (xulstore.json)

Not all may need tests but it would be good to cover the most important once for possible future regressions.

Given that Marionette is still the only framework which can do a restart of Firefox it would be good to implement those tests as Marionette tests, and put those under toolkit/profile/test/marionette.

Ted and Florian (or anyone else too) what do you both think? Is there something which I missed, which could be further tested?

Comment 8

[Florian Quèze [:florian]]

(In reply to Henrik Skupin (:whimboo) from comment #7)
> Here a summary of what we would need as collected by known and already fixed
> bugs:
> 
> * Unicode characters in the path component
> * A mixture of short (8.3) and long path components:
> C:\longpa~1\longpathname\ (Windows only)
> * Occurred for both the profile path, but also for the install path
> 
> Are there any other specific encodings which we should take into account
> here? Would it make sense to split up the tests for eg. UTF-8, UTF-16,
> Windows-1252, ...?

If this isn't already covered, I think we should test having a space in a path component.

> Given that a couple of bugs were present after a restart of Firefox, it will
> not be enough to just create such a profile and start Firefox once, but also
> to make sure that everything still works after a restart. I would not
> differentiate between restart vs. quit + start.

I think ideally we would want to verify that we are able to start:
- with an existing known good profile (eg. created by an older version)
- without profile (ie. starting Firefox creates the profile)

and for each of these 2 cases, we would like to ensure that we can restart without dataloss.

> - search engines (search.json.mozlz4, searchplugins/)

FYI we stopped writing to searchplugins/ in Firefox 45 (bug 1203167) and stopped reading from it in Firefox 57.0.1 (bug 1405670).

And yes, testing that we are not damaging search settings matters :-). We tend to get really angry bug reports when we lose some search settings and some people get accidentally reset to the original default.

> Ted and Florian (or anyone else too) what do you both think?

Thanks for working on this!

Comment 9

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

(In reply to Florian Quèze [:florian] from comment #8)
> If this isn't already covered, I think we should test having a space in a
> path component.

Good point. I will take this into account.

> I think ideally we would want to verify that we are able to start:
> - with an existing known good profile (eg. created by an older version)

This won't be possible given that we only have the current version of Firefox present. Some kind of modified Firefox UI update tests would be necessary for that, but that should be out of scope for now. 

> - without profile (ie. starting Firefox creates the profile)

That is what Marionette usually does except some default preferences necessary for automation, which are getting set on top of the mozprofile preparations.

> > - search engines (search.json.mozlz4, searchplugins/)
> 
> FYI we stopped writing to searchplugins/ in Firefox 45 (bug 1203167) and
> stopped reading from it in Firefox 57.0.1 (bug 1405670).

Thanks. So this is search.json.mozlz4 only then.

Comment 10

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

I was called to work on this because it's important to have, so the severity of the bug should reflect that.

Comment 11

[(not currently active) Ted Mielczarek]

The issue is not specific to an encoding, but is more generally "a path with characters that cannot be represented in the system code page". For en-US Windows that means windows-1252, but for other locales it'd be different. On those systems if you have a path with characters that don't exist in that code page you cannot access that path as a `char*`. This doesn't usually happen in the common case because the default install location and the default profile location are both in paths that, even if localized, are likely to contain only characters within the system code page.

I don't recall offhand, but I believe someone said once that there were one or more east asian locales that didn't have a proper code page, such that if a user created their account with a username in their local script then the path to their home directory (where we put profiles by default) would hit this case.

In any event, for testing purposes you could simply use non-BMP codepoints, like almost any emoji, in the paths.

Comment 12

[(not currently active) Ted Mielczarek]

Also: while it won't hurt anything to run these tests on non-Windows, it's not going to provide as much value. We don't support anything but UTF-8 as a system encoding on Linux, and I'm pretty sure macOS has been UTF-8 basically forever, so treating paths as `char*` on those systems will generally always work. (It's possible for things to break in weird ways if code expects that paths are *valid* UTF-8, because the operating system will allow essentially arbitrary bytes in filenames.)

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Ted, at least for spaces in the path, it would be interested to test on all platforms too!

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

Henrik, I found a problem in one of the external programs used by our crash reporting (see bug 1437156). Would it be possible to include those programs (crashreporter.exe and minidump-analyzer.exe) in this testing?

Comment 15

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

We cannot control both tools with Marionette, but I have an idea... we could cause Firefox to crash via Marionette by executing some JS with faulty ctypes code, which we already do in one of our tests. If under such a condition no crash report is getting created, we might be able to identify the problem.

Comment 16

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

All blockers have been fixed so far. I will have a look on getting the first testcases implemented soon.

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

Moving to p3 because no activity for at least 24 weeks.
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 18

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

Currently I'm not working on this bug.

Comment 19

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

Note that it would be required to wait for mozharness being Python 3 compatible before new tests should be added. Most of the test suites still depend on it when run via the packaged tests in CI.

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.