Closed Bug 1267567 Opened 8 years ago Closed 8 years ago

Unicode characters are unavailable in autoconfig aka MCD (regression)

Categories

(Core :: AutoConfig (Mission Control Desktop), defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- fixed
firefox48 --- fixed
firefox49 --- fixed
firefox-esr45 47+ fixed

People

(Reporter: yuki, Assigned: emk)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(5 files, 2 obsolete files)

MCD(*1) provides ability to set/lock preferences in enterprise use, but now we cannot use Unicode characters for the value of any string preferences, on Firefox 45 ESR. On Firefox 38 ESR it works, so this seems a regression.

(*1) https://developer.mozilla.org/en-US/docs/MCD,_Mission_Control_Desktop_AKA_AutoConfig


Steps to reproduce:

1. Install Firefox 45.
2. Put "autoconfig.js" at "C:\Program Files (x86)\Mozilla Firefox\defaults\pref\autoconfig.js", with the contents:
--------------------------------------------
pref("general.config.filename", "autoconfig.cfg");
pref("general.config.vendor", "autoconfig");
pref("general.config.obscure_value", 0);
--------------------------------------------
3. Put "autoconfig.cfg" at "C:\Program Files (x86)\Mozilla Firefox\autoconfig.cfg", with the contents (UTF-8, no BOM):
--------------------------------------------
// # don't remove this comment! (the first line is ignored by Mozilla)
lockPref("_test.string.ASCII", "ASCII");
lockPref("_test.string.non-ASCII", "日本語");
--------------------------------------------
4. Start Firefox.
5. Go to "about:config" and find "_test".

Actual result:
"_test.string.ASCII" appears with the specified value but "_test.string.non-ASCII" is blank.

Expected result:
Both "_test.string.ASCII" and "_test.string.non-ASCII" appear with their specified value.


This problem doesn't happen on Firefox 38 ESR - I got the expected result with same steps.
The bug 545099 is similar to this, but clearly this is a regression verified on Firefox 38 and 45.
Keywords: regression
Confirmed on Nightly 49.0a1 (20160425102203) also.
Version: 45 Branch → Trunk
FYI, localized preference values defined in distribution.ini still work as expected.
https://wiki.mozilla.org/Distribution_INI_File
Component: Preferences: Backend → AutoConfig (Mission Control Desktop)
I realized that there is a workaround. The configuration file seems to be parsed as Unicode strings. On the other hand, `lockPref()` and other directives simply hands given value to internal nsIPrefBranch service. Thus it works when I convert the value from Unicode string to UTF-8 bytes array manually, like:

--------------------------------------------
var unicodeString = "日本語";
var utf8Bytes = unescape(encodeURIComponent(unicodeString));
lockPref("_test.string.non-ASCII", utf8Bytes);
--------------------------------------------
AutoConfig used to be an ASCII File.

I fixed this in bug 1137799 so that AutoConfig files are now parsed as UTF-8, but if it it detects the file is ASCII, it should go back to the old way (See bug 1193625)

So if you do not save your AutoConfig as UTF-8, you should get the old behavior.

In general, string preferences do not support unicode strings. You've always had to do something special using complex preferences:

https://developer.mozilla.org/en-US/Add-ons/Code_snippets/Preferences#nsISupportsString

The fact that those strings used to work was probably just a side effect of using ASCII.

I could certainly investigate adding the complex preference conversion into the AutoConfig APIs.
My autoconfig.cfg is/was actually encoded in UTF-8. Anyway, the only one thing I'm interested in is: is this an intentional, incompatible spec change, or an unexpected regression?

> The fact that those strings used to work was probably just a side effect of using ASCII.

Then, this seems to be an intentional change and we should update our configuration files for the new spec.
One note: we cannot share same autoconfig.cfg/jsc for both old and new Firefox anymore, thus we have to something to avoid errors...
> My autoconfig.cfg is/was actually encoded in UTF-8. 

Was it UTF-8 in firefox 38 as well?

> One note: we cannot share same autoconfig.cfg/jsc for both old and new Firefox anymore, thus we have to something to avoid errors...

Firefox 38 will be out of support within a few weeks, so that should not be an issue.
And I would probably call this an unexpected regression, but I'm not clear why what you were doing was ever working properly.

AutoConfig files were never read as UTF8, so I can't see how this would have worked on Firefox 38

Plus, as I said, Firefox string preferences need to be complex preferences if they don't contain ASCII characters...

The reason I made the change that I made was because other international characters were not working (because the file had to be ASCII).
Sorry, one more thing.

Can you give me an example of an actual Firefox pref that needs to be in Japanese so I can test this in firefox? Versus just a test string?
For Firefox 38, this must be something that only works on Japanese systems.

If I take your autoconfig file from the attachments and use it on Firefox 38 on a US system, I get an error.
TL;DR - Firefox 45+ will convert AutoConfig pref strings to UTF-8 twice.

Firefox used to abuse the pref API to store Unicode strings to the pref.
It will read "ASCII" (actually ISO-8859-1) strings from the pref and parse them as if it is a UTF-8 string:
https://mxr.mozilla.org/mozilla-central/source/modules/libpref/Preferences.cpp?rev=fcc0936b576d&mark=1432-1432#1423
So, Firefox 38 is expected to treat
  lockPref("_test.string.non-ASCII", "é");
(in ISO-8859-1) as _test.string.non-ASCII=é.
Firefox 45 will read the above line as
  lockPref("_test.string.non-ASCII", "é");
(in UTF-8) due to the change from bug 1137799. Then Preferences::GetString will try to convert the value to a Unicode String, but it will fail because 0xe9 is not a valid sequence as a UTF-8 string.

lockPref should take UTF-8 strings when the AutoConfig is read as UTF-8 for compatibility.

(In reply to Mike Kaply [:mkaply] from comment #12)
> For Firefox 38, this must be something that only works on Japanese systems.
> 
> If I take your autoconfig file from the attachments and use it on Firefox 38
> on a US system, I get an error.

I have no idea why it fails on US system. The above logic does not depend on the locale at all. What error message did you see?
> I have no idea why it fails on US system. The above logic does not depend on the locale at all. What error message did you see?

It was a testing problem. Sorry about that. It does work.

> lockPref should take UTF-8 strings when the AutoConfig is read as UTF-8 for compatibility.

I don't know that I can detect UTF-8 at that point. The file has already been converted.

I could switch lockPref to use nsISupportsStrings, but I think we would still be broken when the AutoConfig file is read as ASCII.

I'm not sure what the right thing is here.

What really frustrates me is that we don't have any tests for any of the AutoConfig stuff and no one has figured out how to create them in our existing testing framework. It's frustrating.
I added a global variable to determine the script encoding.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #8748921 - Flags: review?(mrbkap)
Comment on attachment 8748921 [details] [diff] [review]
Treat string prefs as Unicode when the script encoding is UTF-8

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

The C++ bits look good to me. mkaply should sign off on the API change, though.

::: extensions/pref/autoconfig/src/nsJSConfigTriggers.cpp
@@ +125,5 @@
>        /* If the length is 0, the conversion failed. Fallback to ASCII */
>        convertedScript = NS_ConvertASCIItoUTF16(script);
>      }
> +    JS::Rooted<JS::Value> value(cx, JS::BooleanValue(isUTF8));
> +    JS_DefineProperty(cx, autoconfigSb, "gIsUTF8", value, JSPROP_ENUMERATE);

This should check the return value for failure.
Attachment #8748921 - Flags: review?(mrbkap)
Attachment #8748921 - Flags: review?(mozilla)
Attachment #8748921 - Flags: feedback+
Attachment #8748922 - Flags: review?(mrbkap) → review?(mozilla)
* Added an error check to JS_DefineProperty.
* Changed the tab-width to 4 for consistency with the rest of this file.
Attachment #8748921 - Attachment is obsolete: true
Attachment #8748921 - Flags: review?(mozilla)
Attachment #8748961 - Flags: review?(mozilla)
Comment on attachment 8748961 [details] [diff] [review]
Treat string prefs as Unicode when the script encoding is UTF-8, v2

Looks great to me.

Once this has landed, I think we should request esr approval. I'll justify if necessary
Attachment #8748961 - Flags: review?(mozilla) → review+
[Tracking Requested - why for this release]:

AutoConfig is an enterprise only feature and this is a regression between ESR38 and ESR45
Comment on attachment 8748922 [details] [diff] [review]
unit test

I'm shocked that you were able to figure out how to do AutoConfig unit tests.

This doesn't interfere with any of the other tests? (I.e. it cleans up properly?).

This is going to be a game changer if we can start creating tests...
Attachment #8748922 - Flags: review?(mozilla) → review+
Can you do a try run with tests on Windows and Mac as well? I'm nervous about the test. Thanks!
(In reply to Mike Kaply [:mkaply] from comment #21)
> Comment on attachment 8748922 [details] [diff] [review]
> This doesn't interfere with any of the other tests? (I.e. it cleans up
> properly?).

Good point. I added a resetPrefs() call after autoconfig.js is removed to ensure the proper cleanup.
nsReadConfig will stay loaded, but it should not interfere other tests.
Attachment #8748922 - Attachment is obsolete: true
Attachment #8749426 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/0080c16e94db
https://hg.mozilla.org/mozilla-central/rev/d8b3673f1332
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment on attachment 8748961 [details] [diff] [review]
Treat string prefs as Unicode when the script encoding is UTF-8, v2

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
AutoConfig is an enterprise only feature and this is a regression between ESR38 and ESR45.
User impact if declined: Enterprise admins will be suffered from incompatibility of AutoConfig scripts until the next ESR(52).
Fix Landed on Version: 49 (47 if aurora/beta uplift is approved)
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: None

Approval Request Comment
[Feature/regressing bug #]: 1137799
[User impact if declined]: See above. Some enterprise use non-ESR release.
[Describe test coverage new/current, TreeHerder]: A basic unit test landed.
[Risks and why]: See above
[String/UUID change made/needed]: None
Attachment #8748961 - Flags: approval-mozilla-esr45?
Attachment #8748961 - Flags: approval-mozilla-beta?
Attachment #8748961 - Flags: approval-mozilla-aurora?
Comment on attachment 8749426 [details] [diff] [review]
Unit test for non-ASCII AutoConfig files

[Approval Request Comment]
See the above comment.
Attachment #8749426 - Flags: approval-mozilla-esr45?
Attachment #8749426 - Flags: approval-mozilla-beta?
Attachment #8749426 - Flags: approval-mozilla-aurora?
Depends on: 1271032
Hello Mike, if AutoConfig is an enterprise only feature, why does this need to be uplifted to Beta? Please let me know.
Flags: needinfo?(mozilla)
We have some enterprises that are on the rapid release model. Since this is very low risk (and a regression), it would be useful to have on the beta.

But ESR is the most important.
Flags: needinfo?(mozilla)
(In reply to Mike Kaply [:mkaply] from comment #31)
> We have some enterprises that are on the rapid release model. Since this is
> very low risk (and a regression), it would be useful to have on the beta.
> 
> But ESR is the most important.

Got it. Thanks. In that case, I'll take it on all nom'd branches.
Comment on attachment 8748961 [details] [diff] [review]
Treat string prefs as Unicode when the script encoding is UTF-8, v2

Low risk fix, Aurora48+, Beta47+, ESR45+
Attachment #8748961 - Flags: approval-mozilla-esr45?
Attachment #8748961 - Flags: approval-mozilla-esr45+
Attachment #8748961 - Flags: approval-mozilla-beta?
Attachment #8748961 - Flags: approval-mozilla-beta+
Attachment #8748961 - Flags: approval-mozilla-aurora?
Attachment #8748961 - Flags: approval-mozilla-aurora+
Attachment #8749426 - Flags: approval-mozilla-esr45?
Attachment #8749426 - Flags: approval-mozilla-esr45+
Attachment #8749426 - Flags: approval-mozilla-beta?
Attachment #8749426 - Flags: approval-mozilla-beta+
Attachment #8749426 - Flags: approval-mozilla-aurora?
Attachment #8749426 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.