1345294 - nsIPrefBranch should have methods to get/set unicode strings

Status: RESOLVED FIXED

(Core :: Preferences: Backend, enhancement)

Description

[Florian Quèze [:florian]]

As discussed in bug 1338306 comment 5 and bug 1338306 comment 6, getCharPref does not handle non-ASCII characters and we should provide a JS-counterpart of Preferences::GetString.

The

  Services.prefs.getComplexValue(pref, Ci.nsISupportsString).data;

and

  let str = Cc["@mozilla.org/supports-string;1"]
              .createInstance(Ci.nsISupportsString);
  str.data = val;
  Services.prefs.setComplexValue(aPrefName, Ci.nsISupportsString, str);

are patterns we should eliminate.

Comment 1

[Florian Quèze [:florian]]

Attachment: Patch

Comment 2

[Masatoshi Kimura [:emk]]

Comment on attachment 8844696 [details] [diff] [review]
Patch

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

::: modules/libpref/nsIPrefBranch.idl
@@ +139,5 @@
> +  [optional_argc,binaryname(GetStringPrefWithDefault)]
> +  AUTF8String getStringPref(in string aPrefName,
> +                            [optional] in AUTF8String aDefaultValue);
> +  [noscript,binaryname(GetStringPref)]
> +  AUTF8String getStringPrefXPCOM(in string aPrefName);

Do we need this? C++ callers can use Preferences::GetString(). We had to add methods for other methods due to compatibility, but new methods have no compatibility problem.

Comment 3

[Masatoshi Kimura [:emk]]

"this" means the noscript getStringPrefXPCOM method.

Comment 4

[Florian Quèze [:florian]]

Attachment: Patch v2

Addressed comment 2.

Comment 5

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f14c8d6cb83

Comment 6

[Florian Quèze [:florian]]

Attachment: xpcshell script

Comment 7

[Florian Quèze [:florian]]

Attachment: script generated patch

Comment 8

[Florian Quèze [:florian]]

Attachment: hand cleanup of the script generated patch

Attached separately for easier review, but I will fold this into the script generated patch before landing.

Comment 9

[Florian Quèze [:florian]]

Attachment: add an eslint rule

Comment 10

[Florian Quèze [:florian]]

Attachment: make the eslint rule pass

This can be folded into the patch adding the eslint rule when landing.

Comment 11

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e16defdafbf

Comment 12

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e8fab91c7b5330502facd1317d1ddcb824c96b6
Bug 1345294 - nsIPrefBranch should have methods to get/set unicode strings, r=bsmedberg.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a8192a650e92565aa2e85721569dad58cc1922c
Bug 1345294 - script generated patch (+ some hand cleanup) to replace {get,set}ComplexValue for nsISupportsString by {get,set}StringPref, r=Mossop.

https://hg.mozilla.org/integration/mozilla-inbound/rev/79bacf0ea46664a09a1f09996e70d0d3a4af042d
Bug 1345294 - add an eslint rule to reject usage of {get,set}ComplexValue for nsISupportsString and suggest {get,set}StringPref instead, and make it pass, r=Mossop.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6e8fab91c7b5
https://hg.mozilla.org/mozilla-central/rev/5a8192a650e9
https://hg.mozilla.org/mozilla-central/rev/79bacf0ea466

Comment 14

[Nicholas Nethercote [inactive]]

As far as I can tell, libpref actually stores strings internally as Latin1. E.g. modules/libpref/test/unit/data/testPref.js has this test pref:

> user_pref("testPref.char2", "älskar");

Furthermore, nsIPrefBranch::{get,set}CharPref() work fine with Latin1 strings.

If you call nsIPrefBranch::getCharPref() from JS on that test pref you'll get an XPConnect error, because it'll try to interpret the non-ASCII Latin1 string "älskar" as UTF-8, but it's not valid UTF-8.

Maybe things are ok so long as nsIPrefBranch::{get,set}StringPref() are used in tandem. Though I'm wondering how UTF-8 string values work when written to and read from file.

I haven't worked out all the details here yet, but it's a bit worrying.

Comment 15

[Masatoshi Kimura [:emk]]

I don't follow. getCharPref() will never try to interpret non-ASCII values as UTF-8. getStringPref() does.

Comment 16

[Nicholas Nethercote [inactive]]

(In reply to Masatoshi Kimura [:emk] from comment #15)
> I don't follow. getCharPref() will never try to interpret non-ASCII values
> as UTF-8. getStringPref() does.

A problem occurs if a pref is written with a Latin1 string value (either via setCharPref() or from reading a prefs file) and then you call getStringPref() on it, which tries to interpret it as UTF-8.

Comment 17

[Nicholas Nethercote [inactive]]

Oh, I see the problem: where I wrote "If you call nsIPrefBranch::getCharPref() from JS on that test pref" in comment 14, I should have written "If you call nsIPrefBranch::getStringPref() from JS on that test pref". Apologies for the error.

Comment 18

[Florian Quèze [:florian]]

(In reply to Nicholas Nethercote [:njn] from comment #17)
> Oh, I see the problem: where I wrote "If you call
> nsIPrefBranch::getCharPref() from JS on that test pref" in comment 14, I
> should have written "If you call nsIPrefBranch::getStringPref() from JS on
> that test pref". Apologies for the error.

There's a problem if someone uses non-ascii characters with {get,set}CharPref (this worked for the latin1 case but has never been supported, and that's the reason why we needed getComplexValue(Ci.nsISupportsString)), or if someone mixes the {get,set}CharPref methods with the {get,set}StringPref methods.

Comment 19

[Nicholas Nethercote [inactive]]

Right. So internally it uses 8-bit chars, and the encoding used is determined by how the pref is set: prefs set by reading from file or by setCharPref() are effectively Latin1, and prefs set by setStringPref() are UTF-8. And as long as you use the appropriate getter things work out.

Comment 20

[Florian Quèze [:florian]]

(In reply to Nicholas Nethercote [:njn] from comment #19)
> prefs set by reading from file or by setCharPref() are effectively Latin1,

Is it always latin1? I was afraid it would be the operating system's current default encoding.

Comment 21

[Masatoshi Kimura [:emk]]

(In reply to Florian Quèze [:florian] [:flo] from comment #20)
> (In reply to Nicholas Nethercote [:njn] from comment #19)
> > prefs set by reading from file or by setCharPref() are effectively Latin1,
> 
> Is it always latin1? I was afraid it would be the operating system's current
> default encoding.

It is always latin1 because we don't depend on operating system's feature to convert getCharPref() results and setCharPref() params.

Comment 23

[opera wang]

It seems the interface doc was not updated?
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIPrefBranch