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)
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: yuki, Assigned: emk)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(5 files, 2 obsolete files)
162 bytes,
text/x-csrc
|
Details | |
138 bytes,
application/javascript
|
Details | |
110 bytes,
application/x-wine-extension-ini
|
Details | |
4.70 KB,
patch
|
mkaply
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
ritu
:
approval-mozilla-esr45+
|
Details | Diff | Splinter Review |
5.49 KB,
patch
|
emk
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
ritu
:
approval-mozilla-esr45+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•8 years ago
|
||
The bug 545099 is similar to this, but clearly this is a regression verified on Firefox 38 and 45.
Reporter | ||
Comment 2•8 years ago
|
||
Reporter | ||
Updated•8 years ago
|
Keywords: regression
Reporter | ||
Comment 3•8 years ago
|
||
Confirmed on Nightly 49.0a1 (20160425102203) also.
Version: 45 Branch → Trunk
Reporter | ||
Comment 4•8 years ago
|
||
FYI, localized preference values defined in distribution.ini still work as expected. https://wiki.mozilla.org/Distribution_INI_File
Updated•8 years ago
|
Component: Preferences: Backend → AutoConfig (Mission Control Desktop)
Reporter | ||
Comment 5•8 years ago
|
||
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); --------------------------------------------
Updated•8 years ago
|
Blocks: fx-enterprise
Comment 6•8 years ago
|
||
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.
Reporter | ||
Comment 7•8 years ago
|
||
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.
Reporter | ||
Comment 8•8 years ago
|
||
One note: we cannot share same autoconfig.cfg/jsc for both old and new Firefox anymore, thus we have to something to avoid errors...
Comment 9•8 years ago
|
||
> 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.
Comment 10•8 years ago
|
||
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).
Comment 11•8 years ago
|
||
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?
Comment 12•8 years ago
|
||
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.
Assignee | ||
Comment 13•8 years ago
|
||
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?
Comment 14•8 years ago
|
||
> 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.
Assignee | ||
Comment 15•8 years ago
|
||
I added a global variable to determine the script encoding.
Assignee | ||
Comment 16•8 years ago
|
||
Try runs: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecef8c99879b31751b64776dea104c6d517d824f https://treeherder.mozilla.org/#/jobs?repo=try&revision=c08ff3b305e6e0817353c55314cfa1dc2f60e43a https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a04baf7882bfa8e4f6af2e8a9e69f51675738c9
Attachment #8748922 -
Flags: review?(mrbkap)
Comment 17•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8748922 -
Flags: review?(mrbkap) → review?(mozilla)
Assignee | ||
Comment 18•8 years ago
|
||
* 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 19•8 years ago
|
||
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+
Comment 20•8 years ago
|
||
[Tracking Requested - why for this release]: AutoConfig is an enterprise only feature and this is a regression between ESR38 and ESR45
tracking-firefox-esr45:
--- → ?
Comment 21•8 years ago
|
||
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+
Comment 22•8 years ago
|
||
Can you do a try run with tests on Windows and Mac as well? I'm nervous about the test. Thanks!
Assignee | ||
Comment 23•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a7442a5e365
Assignee | ||
Comment 24•8 years ago
|
||
(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.
Assignee | ||
Comment 25•8 years ago
|
||
Attachment #8748922 -
Attachment is obsolete: true
Attachment #8749426 -
Flags: review+
Assignee | ||
Comment 26•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0080c16e94dbd0a3488ded2bf04403324dc747e2 Bug 1267567 - Treat string prefs as Unicode when the script encoding is UTF-8. r=mkaply https://hg.mozilla.org/integration/mozilla-inbound/rev/d8b3673f1332549228d077ac87bb1d4dfde36a9a Bug 1267567 - Unit test for non-ASCII AutoConfig files. r=mkaply
Comment 27•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0080c16e94db https://hg.mozilla.org/mozilla-central/rev/d8b3673f1332
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Comment 28•8 years ago
|
||
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?
Assignee | ||
Comment 29•8 years ago
|
||
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?
status-firefox47:
--- → affected
status-firefox48:
--- → affected
status-firefox-esr45:
--- → affected
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)
Comment 31•8 years ago
|
||
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+
Comment 34•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/d2397dcceff4 https://hg.mozilla.org/releases/mozilla-aurora/rev/95eba1c7da97
Comment 35•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr45/rev/1041e712cc44 https://hg.mozilla.org/releases/mozilla-esr45/rev/629824ddac49
Comment 36•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/845ee0a38186 https://hg.mozilla.org/releases/mozilla-beta/rev/38353172c113
You need to log in
before you can comment on or make changes to this bug.
Description
•