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

RESOLVED FIXED in Firefox 50

Status

()

defect
P2
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: roy.kokkelkoren, Assigned: darktrojan)

Tracking

Trunk
mozilla50
Unspecified
Linux
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: triaged)

Attachments

(2 attachments, 2 obsolete attachments)

Posted file 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
Component: Untriaged → General
OS: Unspecified → Linux
Product: Thunderbird → Add-on SDK
Version: 38 Branch → unspecified
Component: General → XPCOM
Product: Add-on SDK → Core
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?
Flags: needinfo?(roy.kokkelkoren)
I meant, the codelet related to setting the pref value.
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);
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);
Status: UNCONFIRMED → NEW
Component: XPCOM → Add-ons Manager
Ever confirmed: true
Flags: needinfo?(roy.kokkelkoren)
Product: Core → Toolkit
See Also: → 662012
Summary: nsIFile does not properly handle special filenames → <setting type="file"> and <setting type="directory"> does not properly handle non-ASCII characters
Version: unspecified → Trunk
is this a regression?
(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #5)
> is this a regression?

no.
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.
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Posted patch 1284166-1.diff (obsolete) — Splinter Review
This patch switches from [gs]etCharPref to [gs]etComplexValue and changes the tests so that they use some non-ASCII characters.
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.
Attachment #8767943 - Flags: review?(rhelmer)
Attachment #8767943 - Flags: review?(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?
(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?
Flags: needinfo?(kmaglione+bmo)
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.
Attachment #8767943 - Flags: review?(rhelmer)
Attachment #8767943 - Flags: review?(aswan)
Flags: needinfo?(kmaglione+bmo)
Priority: -- → P2
Whiteboard: triaged
Posted patch 1284166-2.diff (obsolete) — Splinter Review
Attachment #8767943 - Attachment is obsolete: true
Attachment #8772640 - Flags: review?(kmaglione+bmo)
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.
Attachment #8772640 - Flags: review?(kmaglione+bmo)
> 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.
Attachment #8772640 - Attachment is obsolete: true
Attachment #8773547 - Flags: review?(kmaglione+bmo)
Comment on attachment 8773547 [details] [diff] [review]
1284166-4.diff

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

Looks good. Thanks!
Attachment #8773547 - Flags: review?(kmaglione+bmo) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/d12890b78b04
<setting>s do not properly handle non-ASCII characters. r=kmag
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d12890b78b04
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.