1284166 - <setting type="file"> and <setting type="directory"> does not properly handle non-ASCII characters

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect, P2)

Description

[Roy Kokkelkoren]

Attachment: todo2àâêô.txt

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160628004053

Steps to reproduce:

Thunderbird Add-On

nsIFile.exists() returns False when file exists for files with irregular files names such as todo2àâêô.txt. 

1) Retrieved nsIFile from user-input [todoFile = prefs.getComplexValue("todo-txt", Components.interfaces.nsIFile);]
2) Verifying whether file exists [todoFile.exists()] 
3) Creating file when file does not exists [todoFile.create(0,0600);]


Actual results:

todoFile.exists() returns False while file exists.

todoFile.create(0,0600), succeeds even though file exists and results in the creation of another file which shows invalid encoding in the file name.

e.g. todo2$'\340'$'\342'$'\352'$'\364'.txt


Expected results:

todoFile.exists() should return true if file exists
todoFile.create(0,0600), should create a file with the following name: todo2àâêô.txt

Comment 1

[Tooru Fujisawa [:arai]]

sounds like a character encoding issue of the pref value.
what encoding is the pref value using?  what's the actual codepoints?
can you provide working testcase add-on, or codelet?

Comment 2

[Tooru Fujisawa [:arai]]

I meant, the codelet related to setting the pref value.

Comment 3

[Roy Kokkelkoren]

The bug is in the current version of my add-on which can be found at https://github.com/rkokkelk/todo.txt-ext. 

This issues resides in https://github.com/rkokkelk/todo.txt-ext/blob/master/modules/todoclient.js. 

I do not manually set the pref value, but it is set by the user using a file selector. The nsIFile is retrieved using.

var prefs = Components.classes["@mozilla.org/preferences-service;1"]
                            .getService(Components.interfaces.nsIPrefService);
prefs = prefs.getBranch("extensions.todotxt.");
todoFile = prefs.getComplexValue("todo-txt", Components.interfaces.nsIFile);

Comment 4

[Tooru Fujisawa [:arai]]

Thanks :)

<setting type="file"> doesn't handle file path correctly.

https://dxr.mozilla.org/mozilla-central/rev/39dffbba764210b25bfc1e749b4f16db77fa0d46/toolkit/mozapps/extensions/content/setting.xml#408
>             this.value = filePicker.file.path;
> ...
>           Services.prefs.setCharPref(this.pref, this.value);

It just stores the file path without handling Unicode characters.
setCharPref cannot handle non-ASCII characters, and it results in corrupted text.


It should set pref value with Ci.nsILocalFile, or at least Ci.nsISupportsString for file path, like following:

https://dxr.mozilla.org/mozilla-central/rev/39dffbba764210b25bfc1e749b4f16db77fa0d46/toolkit/content/widgets/preferences.xml#410
>             this.preferences.rootBranch
>                 .setComplexValue(this.name, Components.interfaces.nsILocalFile, lf);

https://dxr.mozilla.org/mozilla-central/rev/39dffbba764210b25bfc1e749b4f16db77fa0d46/toolkit/content/widgets/preferences.xml#396
>             var iss = Components.classes["@mozilla.org/supports-string;1"]
>                                 .createInstance(Components.interfaces.nsISupportsString);
>             iss.data = val;
>             this.preferences.rootBranch
>                 .setComplexValue(this.name, Components.interfaces.nsISupportsString, iss);

Comment 5

[Wayne Mery (:wsmwk)]

is this a regression?

Comment 6

[Tooru Fujisawa [:arai]]

(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #5)
> is this a regression?

no.

Comment 7

[Geoff Lankow (:darktrojan)]

I'll take this, since I should've done it right originally. It also affects radiogroup/menulist settings with non-ASCII values, although if somebody is using those they are quite possibly mad.

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: 1284166-1.diff

This patch switches from [gs]etCharPref to [gs]etComplexValue and changes the tests so that they use some non-ASCII characters.

Comment 9

[Geoff Lankow (:darktrojan)]

Comment on attachment 8767943 [details] [diff] [review]
1284166-1.diff

Rob, Andrew: You seem to be the most likely candidates for reviewing this. I hate to land you in the middle of some old code you've probably never seen before, but the people who do know it are unavailable these days.

Comment 10

[Andrew Swan [:aswan]]

I'm not very familiar with this code, :kmag might be a more qualified reviewer.
But are there compatibility issues?  i.e., what if a user has an existing path preference when they upgrade from a version without this patch to a version with this patch?  Do they just lose the existing preference setting?

Comment 11

[Geoff Lankow (:darktrojan)]

(In reply to Andrew Swan [:aswan] from comment #10)
> But are there compatibility issues?  i.e., what if a user has an existing
> path preference when they upgrade from a version without this patch to a
> version with this patch?  Do they just lose the existing preference setting?

Using nsISupportsString works just the same as before, except the character encoding/decoding works properly. So an existing pref can still be read.

> I'm not very familiar with this code, :kmag might be a more qualified reviewer.

How 'bout it, Kris?

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8767943 [details] [diff] [review]
1284166-1.diff

Review of attachment 8767943 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good for the most part, but we should be using Preferences.jsm rather than old-style preference service code.

::: toolkit/mozapps/extensions/content/setting.xml
@@ +408,5 @@
>          <![CDATA[
> +          const nsISupportsString = Components.interfaces.nsISupportsString;
> +          let iss = Components.classes["@mozilla.org/supports-string;1"].createInstance(nsISupportsString);
> +          iss.data = this.value;
> +          Services.prefs.setComplexValue(this.pref, nsISupportsString, iss);

I'd rather use Preferences.jsm rather than setComplexValue/getComplexValue in new code.

::: toolkit/mozapps/extensions/test/browser/browser_inlinesettings.js
@@ +283,5 @@
>        is(input.value, "", "Label value should be empty");
>        is(input.tooltipText, "", "Label tooltip should be empty");
>  
>        var profD = Services.dirsvc.get("ProfD", Ci.nsIFile);
> +      profD.append("\u2622")

Missing semicolon. And please use a different variable name if this isn't going to point to the profile directory anymore.

Comment 13

[Geoff Lankow (:darktrojan)]

Attachment: 1284166-2.diff

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8772640 [details] [diff] [review]
1284166-2.diff

Review of attachment 8772640 [details] [diff] [review]:
-----------------------------------------------------------------

Getting close. Just some minor nits at this point.

::: toolkit/mozapps/extensions/content/setting.xml
@@ +298,5 @@
>      <implementation>
>        <method name="valueFromPreference">
>          <body>
>          <![CDATA[
> +          this.value = Components.utils.import("resource://gre/modules/Preferences.jsm").Preferences.get(this.pref);

Please just import Preferences.jsm once from the constructor of the <settings> binding. The `Preferences` global should then be available everywhere.

@@ +394,5 @@
>          <body>
>          <![CDATA[
> +          const Preferences = Components.utils.import("resource://gre/modules/Preferences.jsm").Preferences;
> +          if (Preferences.has(this.pref)) {
> +            this.value = Preferences.get(this.pref);

We want to set this unconditionally. Probably `this.value = Preferences.get(this.pref, "")` rather than the `Preferences.has` check.

::: toolkit/mozapps/extensions/test/browser/browser_inlinesettings.js
@@ +255,2 @@
>      is(gManagerWindow.document.getBindingParent(gManagerWindow.document.activeElement), input, "Search box should not have focus");
> +    is(Services.prefs.getComplexValue("extensions.inlinesettings1.string", Ci.nsISupportsString).data, "bar\u03DE/", "String pref should have been updated");

We should be using Preferences.jsm rather than `getComplexValue` and `setComplexValue` here (and below), too.

Comment 15

[Geoff Lankow (:darktrojan)]

Attachment: 1284166-4.diff

> Please just import Preferences.jsm once from the constructor of the <settings>
> binding. The `Preferences` global should then be available everywhere.

Now that you mention it, the settings binding isn't used anywhere. Even the place it is referenced is CSS that applies to an element that doesn't exist. So while I'm here, I'm going to remove the binding and reference.

I've put the import into aboutAddons.js and extensions.js instead. This will mean extensions that reuse these bindings (very uncommon) will also need to import Preferences.jsm.

Comment 16

[Kris Maglione [:kmag]]

Comment on attachment 8773547 [details] [diff] [review]
1284166-4.diff

Review of attachment 8773547 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Thanks!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/d12890b78b04
<setting>s do not properly handle non-ASCII characters. r=kmag

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d12890b78b04