Most uses of nsCharsetAlias and nsCharsetConverterManager in Firefox have been replaced by the less COMtaminated and more Encoding Standard-compliant mozilla::dom::EncodingUtils. Once the remaining uses go away, nsCharsetAlias and nsCharsetConverterManager should move to comm-central (or be removed if comm-central introduces an email-oriented analog of mozilla::dom::EncodingUtils or drops support for non-Encoding Standard encodings and moves to using mozilla::dom::EncodingUtils).

prop2arrays isn't working. It's not exactly clear to me what I'm doing wrong. Also, jar manifests are missing pending example from bug 943252.

Comment on attachment 8410886 [details] [diff] [review] m-c patch Notes: * This removes the ability to use x-euc-tw as an nsIPlatformCharset. * This removes the ability to have aliases for "internal" (i.e. non-Encoding Standard) encodings in nsIScriptableUConv. (I.e. in the internal mode, only Gecko-canonical names work.) * Extension compat: nsIScriptableUConv (in the non-internal mode), nsIConverterInputStream and nsIConverterOutputStream now only accept Encoding Standard labels. * However, for compatibility, nsIConverterInputStream and nsIConverterOutputStream give the old meaning to the label UTF-16. * UTF-16 and ISO-8859-1 remain as special Gecko-canonical encoding names when used without Encoding Standard label resolution. (Getting these special cases is out of scope for this bug, IMO.) * Language group mappings are now only supported for Encoding Standard encodings. Thunderbird will have to find another way (e.g. synthetic lang attribute) to get the same layout font selection effects for non-Encoding Standard encodings.

Comment on attachment 8410886 [details] [diff] [review] m-c patch Review of attachment 8410886 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsDocumentEncoder.cpp @@ +1178,5 @@ > return NS_ERROR_NOT_INITIALIZED; > > + nsAutoCString encoding; > + if (!EncodingUtils::FindEncodingForLabel(mCharset, encoding) || > + encoding.EqualsLiteral("replacement")) { Could you add a method or an optional parameter to indicate that the caller wants the replacement encoding rather than adding a compare everywhere? The most callers will not want the replacement encoding. ::: intl/uconv/idl/nsIScriptableUConv.idl @@ +78,2 @@ > */ > attribute boolean isInternal; Is this attribute still needed at all? ::: intl/uconv/src/nsConverterInputStream.cpp @@ +39,5 @@ > + // Compat with old test cases. Unclear if any extensions really care. > + encoding.Assign(label); > + } else if (!EncodingUtils::FindEncodingForLabel(label, encoding)) { > + // Weird API design, but retaining for compat > + encoding.AssignLiteral("ISO-8859-1"); The old code used to fallback only if aCharset was null. But it would return early in that case. So I think the old fallback was already dead. ::: intl/uconv/src/nsConverterOutputStream.cpp @@ +44,5 @@ > + encoding.Assign(label); > + } else if (!EncodingUtils::FindEncodingForLabel(label, encoding) || > + encoding.EqualsLiteral("replacement")) { > + // Weird API design, but retaining for compat > + encoding.AssignLiteral("ISO-8859-1"); The old code didn't have this (dead) fallback for the encoder. ::: intl/uconv/src/nsTextToSubURI.cpp @@ +24,5 @@ > > NS_IMETHODIMP nsTextToSubURI::ConvertAndEscape( > const char *charset, const char16_t *text, char **_retval) > { > + if(nullptr == _retval) { nit: if (!_retval) { ::: intl/uconv/tests/test_long_doc.html @@ +28,2 @@ > > +var decoders = [ Could you put the decoder name list in a common header instead of copy & pasting everywhere? ::: intl/uconv/ucvcn/nsISO2022CNToUnicode.cpp @@ +13,5 @@ > nsresult rv; > > if(!mGB2312_Decoder) { > // creating a delegate converter (GB2312) > + mGB2312_Decoder = EncodingUtils::DecoderForEncoding(NS_LITERAL_CSTRING("GB2312")); This will always fail. GB2312 is not a canonical name. @@ +29,5 @@ > nsresult rv; > > if(!mEUCTW_Decoder) { > // creating a delegate converter (x-euc-tw) > + mEUCTW_Decoder = EncodingUtils::DecoderForEncoding(NS_LITERAL_CSTRING("x-euc-tw")); This will always fail. x-euc-tw is no longer supported as you said. Why don't you just remove the ISO-2022-CN decoder especially when instantiating the decoder will always fail? Moreover, the decoder has been broken for a long time (bug 470523), but nobody cares. ::: intl/uconv/ucvja/nsJapaneseToUnicode.cpp @@ +763,5 @@ > } else { > if (!mGB2312Decoder) { > // creating a delegate converter (GB2312) > + mGB2312Decoder = > + EncodingUtils::DecoderForEncoding(NS_LITERAL_CSTRING("GB2312")); "gbk" until we decide to remove this in bug 996599. ::: intl/unicharutil/src/nsSaveAsCharset.cpp @@ +327,5 @@ > nsresult nsSaveAsCharset::SetupUnicodeEncoder(const char* charset) > { > NS_ENSURE_ARG(charset); > + nsDependentCString encoding(charset); > + mEncoder = EncodingUtils::EncoderForEncoding(encoding); |charset| can have any random value because nsSaveAsCharset::Init is scriptable. ::: js/xpconnect/src/XPCLocale.cpp @@ +192,5 @@ > if (NS_SUCCEEDED(rv)) { > nsAutoCString charset; > rv = platformCharset->GetDefaultCharsetForLocale(localeStr, charset); > if (NS_SUCCEEDED(rv)) { > + mDecoder = EncodingUtils::DecoderForEncoding(charset); Are you sure GetDefaultCharsetForLocale will always return Gecko-canonical name on UNIX? ::: layout/base/tests/test_bug399284.html @@ -73,5 @@ > > -function lastTest(frame) > -{ > - testFontSize(frame); > - SimpleTest.finish(); Isn't SimpleTest.finish(); needed? ::: netwerk/base/src/nsStandardURL.cpp @@ +211,5 @@ > bool nsStandardURL:: > nsSegmentEncoder::InitUnicodeEncoder() > { > NS_ASSERTION(!mEncoder, "Don't call this if we have an encoder already!"); > + mEncoder = EncodingUtils::EncoderForEncoding(mCharset); If i read the code correctly, mCharset will be taken from mOriginCharset and mOriginCharset may be taken from the scriptable source. ::: netwerk/test/unit/test_gre_resources.js @@ -21,5 @@ > - } > -} > - > -function run_test() { > - for each(let file in ["charsetData.properties"]) I think we should change the file to another file under resource://gre-resources/ rather than removing the entire test.

> This will always fail. GB2312 is not a canonical name. Oh, I misunderstood that DecoderForEncoding only supports Encoding Standard encodings. Please disregard this comment (and a few following comments).

(In reply to Masatoshi Kimura [:emk] from comment #8) > ::: content/base/src/nsDocumentEncoder.cpp > @@ +1178,5 @@ > > return NS_ERROR_NOT_INITIALIZED; > > > > + nsAutoCString encoding; > > + if (!EncodingUtils::FindEncodingForLabel(mCharset, encoding) || > > + encoding.EqualsLiteral("replacement")) { > > Could you add a method or an optional parameter to indicate that the caller > wants the replacement encoding rather than adding a compare everywhere? > The most callers will not want the replacement encoding. OK. > ::: intl/uconv/idl/nsIScriptableUConv.idl > @@ +78,2 @@ > > */ > > attribute boolean isInternal; > > Is this attribute still needed at all? It is still used for unit tests. > ::: intl/uconv/src/nsConverterInputStream.cpp > @@ +39,5 @@ > > + // Compat with old test cases. Unclear if any extensions really care. > > + encoding.Assign(label); > > + } else if (!EncodingUtils::FindEncodingForLabel(label, encoding)) { > > + // Weird API design, but retaining for compat > > + encoding.AssignLiteral("ISO-8859-1"); > > The old code used to fallback only if aCharset was null. But it would return > early in that case. So I think the old fallback was already dead. Good point. Yet, unit test assumed that bogus input should result in decoding as ISO-8859-1 (dunno by what mechanism now!), so I think this code needs to stay, but I'll push to try and see what happens if I remove this. > ::: intl/uconv/src/nsConverterOutputStream.cpp > @@ +44,5 @@ > > + encoding.Assign(label); > > + } else if (!EncodingUtils::FindEncodingForLabel(label, encoding) || > > + encoding.EqualsLiteral("replacement")) { > > + // Weird API design, but retaining for compat > > + encoding.AssignLiteral("ISO-8859-1"); > > The old code didn't have this (dead) fallback for the encoder. I'll see what happens if I push to try without this. > ::: intl/uconv/src/nsTextToSubURI.cpp > @@ +24,5 @@ > > > > NS_IMETHODIMP nsTextToSubURI::ConvertAndEscape( > > const char *charset, const char16_t *text, char **_retval) > > { > > + if(nullptr == _retval) { > > nit: if (!_retval) { OK. > ::: intl/uconv/tests/test_long_doc.html > @@ +28,2 @@ > > > > +var decoders = [ > > Could you put the decoder name list in a common header instead of copy & > pasting everywhere? I'd rather not. The list is not actually the same everywhere, since sometimes UTF-16 is on the list and sometimes not. Also, I don't know how to include code cleanly into xpcshell tests. > Why don't you just remove the ISO-2022-CN decoder especially when > instantiating the decoder will always fail? I'll remove the decoders in a later patch. For canonical names, your comment 9 is correct. > ::: intl/unicharutil/src/nsSaveAsCharset.cpp > @@ +327,5 @@ > > nsresult nsSaveAsCharset::SetupUnicodeEncoder(const char* charset) > > { > > NS_ENSURE_ARG(charset); > > + nsDependentCString encoding(charset); > > + mEncoder = EncodingUtils::EncoderForEncoding(encoding); > > |charset| can have any random value because nsSaveAsCharset::Init is > scriptable. :-( I audited all the in-tree callers. Re-resolving labels would break form submission when a form is inserted into a replacement-encoded document using a script from the frame parent. I suggest we just break extensions that pass a non-canonical name. > ::: js/xpconnect/src/XPCLocale.cpp > @@ +192,5 @@ > > if (NS_SUCCEEDED(rv)) { > > nsAutoCString charset; > > rv = platformCharset->GetDefaultCharsetForLocale(localeStr, charset); > > if (NS_SUCCEEDED(rv)) { > > + mDecoder = EncodingUtils::DecoderForEncoding(charset); > > Are you sure GetDefaultCharsetForLocale will always return Gecko-canonical > name on UNIX? Yes. See VerifyCharset() in nsUNIXCharset.cpp. > ::: layout/base/tests/test_bug399284.html > @@ -73,5 @@ > > > > -function lastTest(frame) > > -{ > > - testFontSize(frame); > > - SimpleTest.finish(); > > Isn't SimpleTest.finish(); needed? It indeed s not needed and is actively harmful. I have no idea how this test managed not to fail before. > ::: netwerk/base/src/nsStandardURL.cpp > @@ +211,5 @@ > > bool nsStandardURL:: > > nsSegmentEncoder::InitUnicodeEncoder() > > { > > NS_ASSERTION(!mEncoder, "Don't call this if we have an encoder already!"); > > + mEncoder = EncodingUtils::EncoderForEncoding(mCharset); > > If i read the code correctly, mCharset will be taken from mOriginCharset and > mOriginCharset may be taken from the scriptable source. :-( I will investigate. > ::: netwerk/test/unit/test_gre_resources.js > @@ -21,5 @@ > > - } > > -} > > - > > -function run_test() { > > - for each(let file in ["charsetData.properties"]) > > I think we should change the file to another file under > resource://gre-resources/ rather than removing the entire test. OK. Thanks.

(In reply to Henri Sivonen (:hsivonen) from comment #10) > > ::: intl/unicharutil/src/nsSaveAsCharset.cpp > > @@ +327,5 @@ > > > nsresult nsSaveAsCharset::SetupUnicodeEncoder(const char* charset) > > > { > > > NS_ENSURE_ARG(charset); > > > + nsDependentCString encoding(charset); > > > + mEncoder = EncodingUtils::EncoderForEncoding(encoding); > > > > |charset| can have any random value because nsSaveAsCharset::Init is > > scriptable. > > :-( I audited all the in-tree callers. Re-resolving labels would break form > submission when a form is inserted into a replacement-encoded document using > a script from the frame parent. > > I suggest we just break extensions that pass a non-canonical name. Nevermind. I'll just check for "replacement" first.

Any ideas why the Makefile.in fails to cause charsetalias.properties.h to be generated?

Adding /mozilla right after $(topsrcdir) for props2arrays.py doesn't help.

charsetData.properties removal notes: * Move knowledge of multibyteness to nsMsgI18N.cpp: https://mxr.mozilla.org/comm-central/search?string=isMultibyte * Remove .notForOutgoing: https://mxr.mozilla.org/comm-central/search?string=notForOutgoing * The m-c patch moves lang groups elsewhere. Probably not worthwhile to get the lang group right for mail-only encodings like ISO-2022-CN. * Hard-code list of "internal" encodings. Soon, charsetTitles.properties will only be used for the compose window title: https://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2441

(In reply to Henri Sivonen (:hsivonen) from comment #15) > charsetData.properties removal notes: > > * Move knowledge of multibyteness to nsMsgI18N.cpp: > https://mxr.mozilla.org/comm-central/search?string=isMultibyte The biggest use of that knowledge is the selection for RFC 2047 encoding, which is being killed off by bug 790855 anyways. The other uses of IsMultibyte look vaguely wrong ("oh, we don't have to worry about emitting charset because it's base64 encoded!"). > * Remove .notForOutgoing: > https://mxr.mozilla.org/comm-central/search?string=notForOutgoing With the removal of nsCharsetMenu, this is basically unused?

Let's open another bug for the c-c reaction to make it clearer what needs landing where and what the bug status is.

Note that potentially odd-looking #include additions are there to make things work when we withdraw code from UNIFIED_SOURCES so following sources can't rely on previous #includes.

(In reply to Henri Sivonen from comment #23) > Note that potentially odd-looking #include additions are there to make > things work when we withdraw code from UNIFIED_SOURCES so following sources > can't rely on previous #includes. How is the original code able to build in unified disabled mode?

(In reply to neil@parkwaycc.co.uk from comment #24) > (In reply to Henri Sivonen from comment #23) > > Note that potentially odd-looking #include additions are there to make > > things work when we withdraw code from UNIFIED_SOURCES so following sources > > can't rely on previous #includes. > > How is the original code able to build in unified disabled mode? I don't know. I just kept adding #includes until stuff worked again.

Comment on attachment 8417967 [details] [diff] [review] Remove nsCharsetConverterManager and nsCharsetAlias, accommodate Linux 32 debug Review of attachment 8417967 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsDocumentEncoder.cpp @@ +1195,5 @@ > if (!mDocument) > return NS_ERROR_NOT_INITIALIZED; > > + nsAutoCString encoding; > + if (!EncodingUtils::FindEncodingForLabelNoReplacement(mCharset, encoding)) { I prefer to make this function name shorter because fewer callers care about the replacement encoding. How about renaming the current FindEncodingForLabel to FindEncodingForLabelWithReplacement and renaming FindEncodingForLabelNoReplacement to FindEncodingForLabel? ::: intl/uconv/src/nsConverterInputStream.cpp @@ +37,5 @@ > + nsAutoCString encoding; > + if (label.EqualsLiteral("UTF-16")) { > + // Compat with old test cases. Unclear if any extensions really care. > + encoding.Assign(label); > + } else if (!EncodingUtils::FindEncodingForLabel(label, encoding)) { This change will shrink the supported encodings anyway, so I think FindEncodingForLabelNoReplacement (if not renamed) would be better. ::: intl/uconv/src/nsTextToSubURI.cpp @@ +24,5 @@ > > NS_IMETHODIMP nsTextToSubURI::ConvertAndEscape( > const char *charset, const char16_t *text, char **_retval) > { > + if(!_retval) { Uber nit: space between "if" and "(". (I know some existing code didn't follow the convention, but this should be consistent with other added code here.)

(In reply to Masatoshi Kimura [:emk] from comment #26) > I prefer to make this function name shorter because fewer callers care about > the replacement encoding. > How about renaming the current FindEncodingForLabel to > FindEncodingForLabelWithReplacement and renaming > FindEncodingForLabelNoReplacement to FindEncodingForLabel? I think flipping the names around so that the version that never returns "replacement" is the one without modifiers in the name would be bad, because this code should reflect the spec in an obvious way and "get an encoding" in the spec can return "replacement" and treating a returned "replacement" as if the label had been unknown is treated as a modified case by the spec. Also, we should make sure that people who aren't familiar with the issues are doing pick the one that can return "replacement", since that's less likely to lead to a security bug (though that's mainly a problem in the HTML parser and hopefully people patching the HTML parser are familiar with the issues...). If we need to make these names shorter, I'd prefer to drop "Find" from the start to make them: EncodingForLabel() and EncodingForLabelNoReplacement(). > ::: intl/uconv/src/nsConverterInputStream.cpp > @@ +37,5 @@ > > + nsAutoCString encoding; > > + if (label.EqualsLiteral("UTF-16")) { > > + // Compat with old test cases. Unclear if any extensions really care. > > + encoding.Assign(label); > > + } else if (!EncodingUtils::FindEncodingForLabel(label, encoding)) { > > This change will shrink the supported encodings anyway, so I think > FindEncodingForLabelNoReplacement (if not renamed) would be better. OK. > ::: intl/uconv/src/nsTextToSubURI.cpp > @@ +24,5 @@ > > > > NS_IMETHODIMP nsTextToSubURI::ConvertAndEscape( > > const char *charset, const char16_t *text, char **_retval) > > { > > + if(!_retval) { > > Uber nit: space between "if" and "(". (I know some existing code didn't > follow the convention, but this should be consistent with other added code > here.) OK.

This patch doesn't rename the methods yet.

Comment on attachment 8418706 [details] [diff] [review] Remove nsCharsetConverterManager and nsCharsetAlias addressing review comments Let's land this part now. I'll consider the naming little more.

(In reply to Masatoshi Kimura [:emk] from comment #30) > Let's land this part now. I'll consider the naming little more. Thank you! Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/15680e55195c

(In reply to Masatoshi Kimura [:emk] from comment #30) > I'll consider the naming little more. In order to make it clear what got fixed here, let's do that with a new bug number: bug 1007581.

Notes for the documentation team: This patch doesn't change anything for Web developers. The encodings being taken away from add-on developers were already taking away from Web developers previously. This patch makes character encodings that are not part of the Encoding Standard (http://encoding.spec.whatwg.org/) unavailable to Firefox add-ons, except in the case where you set isInternal to true on nsIScriptableUnicodeConverter, which add-on developers really shouldn't be doing. Additionally, for end users, this de-supports Unix systems whose locale encoding is EUC-TW somewhat more than previously. This probably isn't worth documenting, though, since it looks like our support for non-UTF-8 Unix systems is semi-broken already.

As this patch is currently designed, there is no mechanism by which Thunderbird can expose its non-Encoding spec based charsets to JavaScript. I would like to see this patch backed out and not relanded into this mechanism is added.

This is also breaking a FFOS extension. Current code was using charsetConverterManager to get an encoder for Shift-JIS encoding. The EncodingUtils.h used in this patch depends on internal headers that can't be used outside of internal gecko code. Either this patch should backed out or please provide a public mechanism to get an encoder for any publicly supported encoding.

(In reply to Joshua Cranmer [:jcranmer] from comment #35) > As this patch is currently designed, there is no mechanism by which > Thunderbird can expose its non-Encoding spec based charsets to JavaScript. > > I would like to see this patch backed out and not relanded into this > mechanism is added. I deliberately left in such a mechanism: isInternal on nsIScriptableUConv.

(In reply to Chris from comment #36) > This is also breaking a FFOS extension. Current code was using > charsetConverterManager to get an encoder for Shift-JIS encoding. > > The EncodingUtils.h used in this patch depends on internal headers that > can't be used outside of internal gecko code. > > Either this patch should backed out or please provide a public mechanism to > get an encoder for any publicly supported encoding. Is there a reason why any of the following don't work: TextDecoder nsIScriptableUConv nsIConverterInputStream ?

(In reply to Chris from comment #36) > This is also breaking a FFOS extension. Can you please provide a link to the code that broke so that I can better assist in fixing it?

(In reply to Henri Sivonen (:hsivonen) from comment #38) > (In reply to Chris from comment #36) > > This is also breaking a FFOS extension. Current code was using > > charsetConverterManager to get an encoder for Shift-JIS encoding. > > > > The EncodingUtils.h used in this patch depends on internal headers that > > can't be used outside of internal gecko code. > > > > Either this patch should backed out or please provide a public mechanism to > > get an encoder for any publicly supported encoding. > > Is there a reason why any of the following don't work: > TextDecoder > nsIScriptableUConv > nsIConverterInputStream > ? Oops. I misread the part where you say *en*coder. Why do you need to encode anything other than UTF-8?

Also: EncodingUtils::EncoderForEncoding hides XPCOM cruft. If you need XPCOM cruft, you can use do_CreateInstance with the contract id for the Shift_JIS encoder.

(In reply to Henri Sivonen (:hsivonen) from comment #37) > (In reply to Joshua Cranmer [:jcranmer] from comment #35) > > As this patch is currently designed, there is no mechanism by which > > Thunderbird can expose its non-Encoding spec based charsets to JavaScript. > > > > I would like to see this patch backed out and not relanded into this > > mechanism is added. > > I deliberately left in such a mechanism: isInternal on nsIScriptableUConv. That mechanism is *broken*. The code in question in the tests does this: this._encoder = Cc[""].createInstance(); this._encoder.isInternal true; this._encoder.charset label; And this results in an assertion failure: encoder (Tried to create encoder for uknown encoding.), at /src/trunk/comm-central/mozilla/dom/encoding/EncodingUtils.cpp. Actually, looking at one of the tests in more detail, it asserts instead of throwing an error on an unknown charset.

It also appears unable to be able to do any custom alias management. Crashing a program because a user-specified (potentially network-derived charset) was not found IS NOT A GOOD IDEA. Why are you not throwing an error instead?

EncodingUtils has two *distinct* concepts that are respresented as nsACStrings: labels and encodings. A label is a string you get from the network. An encoding is a symbol that denotes the concept of a particular encoding. DecoderForEncoding and EncoderForEncoding take encodings. You must never treat a string obtained from the network as an encoding. DexoderForEncosinf asserts, as you've noticed, if you break this rule. You must always treat network-originating strings as labels. Use FindEncodingForLabel to go from a label to an encoding in a Web-oriented way. Use nsCharsetAlias to do the same in an email-oriented way. When isInternal is true, nsIScriptableUConv takes an encoding instead of taking a label. This works as evidenced by tests in m-c. There's a bug about making the encoding concept not be represented by an nsACString. That bug will probably never get fixed, though.

(In reply to Henri Sivonen (:hsivonen) from comment #44) > When isInternal is true, nsIScriptableUConv takes an encoding instead of > taking a label. This works as evidenced by tests in m-c. I can tell you right off the bat that all the uses of nsIScriptableUConv (particularly in extensions) I've looked at that set internal = true do not follow this rule. Even the tests that you ported to comm-central don't follow this rule. It is extremely hostile to add-ons to make this kind of major change.

Why are extensions using an API with "internal" in its name? But OK. Let's make isInternal less hostile to extensions by making it accept Web labels as well as internal Gecko encodings as a follow-up.

(In reply to Henri Sivonen (:hsivonen) from comment #46) > Let's > make isInternal less hostile to extensions by making it accept Web labels as > well as internal Gecko encodings as a follow-up. Bug 1008832.

(In reply to Henri Sivonen (:hsivonen) from comment #38) > (In reply to Chris from comment #36) > > This is also breaking a FFOS extension. Current code was using > > charsetConverterManager to get an encoder for Shift-JIS encoding. > > > > The EncodingUtils.h used in this patch depends on internal headers that > > can't be used outside of internal gecko code. > > > > Either this patch should backed out or please provide a public mechanism to > > get an encoder for any publicly supported encoding. > > Is there a reason why any of the following don't work: > TextDecoder > nsIScriptableUConv > nsIConverterInputStream > ? Thanks for the feedback. I've reworked some of our extension code to leverage nsIScriptableUConv. We should be ok now, unless otherwise noted.