801402 - Use EncodingUtils::FindEncodingForLabel instead of nsCharsetAlias::GetPreferred from HTML5 parser and DOM APIs

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Masatoshi Kimura [:emk]]

It will take a long time to align intl/uconv to Encoding Standard because of the backward compatibility.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: Make EncodingUtil methods case sensitive

Comment 2

[Masatoshi Kimura [:emk]]

Attachment: Use EncodingUtils::FindEncodingForLabel from DOM

Comment 3

[Masatoshi Kimura [:emk]]

Attachment: Use FindEncodingForLabel from XSLT

Comment 4

[Masatoshi Kimura [:emk]]

Attachment: Use FindEncodingForLabel from FileReaderSync

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: Use FindEncodingForLabel from OS.File

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: Use FindEncodingForLabel from HTML parser

Comment 7

[Masatoshi Kimura [:emk]]

Attachment: Use FindEncodingForLabel from XML parser

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 672555 [details] [diff] [review]
Use FindEncodingForLabel from XSLT

Jonas or peterv should review changes to XSLT.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 672558 [details] [diff] [review]
Use FindEncodingForLabel from OS.File

Sorry Jonas, I think you should review this too.

Comment 10

[Masatoshi Kimura [:emk]]

Comment on attachment 672553 [details] [diff] [review]
Make EncodingUtil methods case sensitive

This patch caused many test failures.

Comment 11

[Henri Sivonen (:hsivonen)]

Comment on attachment 672560 [details] [diff] [review]
Use FindEncodingForLabel from XML parser

The new method returns a label in the lower case and the returned label is used in a context that expect a Gecko-canonical label. Unfortunately, those are not all lower-case, yet.

Comment 12

[Henri Sivonen (:hsivonen)]

Comment on attachment 672559 [details] [diff] [review]
Use FindEncodingForLabel from HTML parser

Same problem as with the SAX patch.

Comment 13

[Henri Sivonen (:hsivonen)]

Oh. I didn’t look at the patch that changed what EncodingUtils return. But looks like that’s obsolete now.

Comment 14

[Masatoshi Kimura [:emk]]

(In reply to Henri Sivonen (:hsivonen) from comment #13)
> Oh. I didn’t look at the patch that changed what EncodingUtils return. But
> looks like that’s obsolete now.
Yup, I'll attach an updated patch once test failures are resolved.

Comment 15

[Masatoshi Kimura [:emk]]

Attachment: Make EncodingUtil methods case sensitive

- Fixed some bonehead mistakes.
- Made EncodingUtils thread-safe.

Comment 16

[Masatoshi Kimura [:emk]]

Attachment: Make EncodingUtil methods case sensitive

Ah, one hank lacked.

Comment 17

[Masatoshi Kimura [:emk]]

Attachment: Use EncodingUtils::FindEncodingForLabel from DOM

Fixed some tests.

Comment 18

[Masatoshi Kimura [:emk]]

Attachment: Use FindEncodingForLabel from FileReaderSync

Fixed a test.

Comment 19

[Masatoshi Kimura [:emk]]

Attachment: Use FindEncodingForLabel from HTML parser

Henri, could you rethink on the premise of the change of EncodingUtils?

Comment 20

[Henri Sivonen (:hsivonen)]

Comment on attachment 673025 [details] [diff] [review]
Make EncodingUtil methods case sensitive

Drive-by comments:
Adding a mutex seems unfortunate. I’d prefer a design that only reads the shared data and, therefore, doesn’t need a mutex. The old alias code does not need a mutex, because it works with plain old C data in a read-only fashion: http://mxr.mozilla.org/mozilla-central/source/intl/locale/src/nsUConvPropertySearch.cpp#10

>  CopyASCIItoUTF16(mEncoding, aEncoding);
>  nsContentUtils::ASCIIToLower(aEncoding);

Why ASCIIToUTF16 instead of UTF8ToUTF16? What does the ASCIIToUTF16 version do to non-ASCII bytes?

Comment 21

[Henri Sivonen (:hsivonen)]

Comment on attachment 673032 [details] [diff] [review]
Use FindEncodingForLabel from HTML parser

>@@ -1186,21 +1188,18 @@ nsHtml5StreamParser::PreferredForInterna

>-  rv = nsCharsetAlias::GetPreferred(newEncoding, preferred);
>-  if (NS_FAILED(rv)) {
>+  if (!EncodingUtils::FindEncodingForLabel(newEncoding, preferred)) {
>     // This charset has been blacklisted for permitting XSS smuggling.
>     // EncMetaNonRoughSuperset is a reasonable approximation to the
>     // right error message.

For this code to continue to make sense, you need to change the earlier nsCharsetAlias::Equals call to use the same data as EncodingUtils::FindEncodingForLabel. Once you change that, if your new Equals method has the same rv semantics as the old one (the old one returns a failure rv for bogus labels rather than returning success rv and setting the boolean to false), this call should never fail, because unsupported encodings would have been caught by the Equals check returning a failure rv for a bogus label. If you introduce an equals check with more intuitive semantics (merely returns false for bogus labels), then you need to make sure the error messages still work the right way with bogus labels but catching the bogus labels by other means (or with FindEncodingForLabel here and rearranging the error messages).

Comment 22

[Henri Sivonen (:hsivonen)]

In general, nsCharsetAlias::Equals calls need to be replaced with something that uses the same data EncodingUtils::FindEncodingForLabel.

Comment 23

[Masatoshi Kimura [:emk]]

Attachment: Make EncodingUtil methods case sensitive

(In reply to Henri Sivonen (:hsivonen) from comment #20)
> Adding a mutex seems unfortunate. I’d prefer a design that only reads the
> shared data and, therefore, doesn’t need a mutex. The old alias code does
> not need a mutex, because it works with plain old C data in a read-only
> fashion:
> http://mxr.mozilla.org/mozilla-central/source/intl/locale/src/
> nsUConvPropertySearch.cpp#10
I borrowed nsUConvPropertySearch. Mutex is not longer present. Now EncodingUtils is completely static class.

> >  CopyASCIItoUTF16(mEncoding, aEncoding);
> >  nsContentUtils::ASCIIToLower(aEncoding);
> 
> Why ASCIIToUTF16 instead of UTF8ToUTF16? What does the ASCIIToUTF16 version
> do to non-ASCII bytes?
mEncoding is alwas ASCII.

Comment 24

[Masatoshi Kimura [:emk]]

Attachment: Use EncodingUtils::FindEncodingForLabel from DOM

Fixed one more test.

Comment 25

[Masatoshi Kimura [:emk]]

Attachment: Use FindEncodingForLabel from HTML parser

(In reply to Henri Sivonen (:hsivonen) from comment #21)
> >@@ -1186,21 +1188,18 @@ nsHtml5StreamParser::PreferredForInterna
> 
> >-  rv = nsCharsetAlias::GetPreferred(newEncoding, preferred);
> >-  if (NS_FAILED(rv)) {
> >+  if (!EncodingUtils::FindEncodingForLabel(newEncoding, preferred)) {
> >     // This charset has been blacklisted for permitting XSS smuggling.
> >     // EncMetaNonRoughSuperset is a reasonable approximation to the
> >     // right error message.
> 
> For this code to continue to make sense, you need to change the earlier
> nsCharsetAlias::Equals call to use the same data as
> EncodingUtils::FindEncodingForLabel. Once you change that, if your new
> Equals method has the same rv semantics as the old one (the old one returns
> a failure rv for bogus labels rather than returning success rv and setting
> the boolean to false), this call should never fail, because unsupported
> encodings would have been caught by the Equals check returning a failure rv
> for a bogus label. If you introduce an equals check with more intuitive
> semantics (merely returns false for bogus labels), then you need to make
> sure the error messages still work the right way with bogus labels but
> catching the bogus labels by other means (or with FindEncodingForLabel here
> and rearranging the error messages).
I've rewritten PreferredForInternalEncodingDecl so that nsCharsetAlias::Equals is no longer needed.

(In reply to Henri Sivonen (:hsivonen) from comment #22)
> In general, nsCharsetAlias::Equals calls need to be replaced with something
> that uses the same data EncodingUtils::FindEncodingForLabel.
HTML5 parser and old HTML parser was the only callers of nsCharsetAlias::Equals.

Comment 26

[Masatoshi Kimura [:emk]]

Green on try.
https://tbpl.mozilla.org/?tree=Try&rev=78f3ec0385c6

Comment 27

[Henri Sivonen (:hsivonen)]

Comment on attachment 673583 [details] [diff] [review]
Make EncodingUtil methods case sensitive

Did this patch bitrot or does it need another patch to be applied first?

Comment 28

[Masatoshi Kimura [:emk]]

(In reply to Henri Sivonen (:hsivonen) from comment #27)
> Comment on attachment 673583 [details] [diff] [review]
> Make EncodingUtil methods case sensitive
> 
> Did this patch bitrot or does it need another patch to be applied first?

It needs attachment 671648 [details] [diff] [review] in bug 801487 patch first.

Comment 29

[Masatoshi Kimura [:emk]]

The attachment 671648 [details] [diff] [review] has been landed.

Comment 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 673583 [details] [diff] [review]
Make EncodingUtil methods case sensitive

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

I think Henri would be a better reviewer.

Comment 32

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 673584 [details] [diff] [review]
Use EncodingUtils::FindEncodingForLabel from DOM

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

::: content/base/src/nsXMLHttpRequest.cpp
@@ +755,5 @@
>    nsAutoCString charsetVal;
> +  bool ok = channel ? NS_SUCCEEDED(channel->GetContentCharset(charsetVal)) :
> +                      false;
> +  if (ok) {
> +    ok = EncodingUtils::FindEncodingForLabel(charsetVal, mResponseCharset);

bool ok = channel &&
          NS_SUCCEEDED(...) &&
          EncodingUtils::Find...;

Comment 33

[Masatoshi Kimura [:emk]]

Attachment: Make EncodingUtil methods case sensitive

Fixed a bitrot by bug 801487 update & bug 799917 backout.

Comment 34

[Masatoshi Kimura [:emk]]

Attachment: Use EncodingUtils::FindEncodingForLabel from DOM. r=jonas

Unbitrotted & resolved a review comment.

Comment 35

[Henri Sivonen (:hsivonen)]

Comment on attachment 678708 [details] [diff] [review]
Make EncodingUtil methods case sensitive

Looks good to me.

Comment 36

[Henri Sivonen (:hsivonen)]

Comment on attachment 673586 [details] [diff] [review]
Use FindEncodingForLabel from HTML parser

Bug 716579 bitrotted the nsParser and nsScanner parts of this patch. :-(

Comment 37

[Masatoshi Kimura [:emk]]

Attachment: Use FindEncodingForLabel from HTML parser

Unbitrotted

Comment 38

[Masatoshi Kimura [:emk]]

Comment on attachment 673028 [details] [diff] [review]
Use FindEncodingForLabel from FileReaderSync

Bent seems to be very busy.

Comment 39

[Masatoshi Kimura [:emk]]

https://tbpl.mozilla.org/?tree=Try&rev=368ed018dfd2

Comment 40

[Henri Sivonen (:hsivonen)]

Comment on attachment 678878 [details] [diff] [review]
Use FindEncodingForLabel from HTML parser

These changes themselves are OK, but I think the code in nsScanner after this patch can get confused and assert fatally if you don’t also change the alias resolution in nsDocument::TryChannelCharset to use the new alias code.
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#3047

Comment 41

[Masatoshi Kimura [:emk]]

(In reply to Henri Sivonen (:hsivonen) from comment #40)
> Comment on attachment 678878 [details] [diff] [review]
> Use FindEncodingForLabel from HTML parser
> 
> These changes themselves are OK, but I think the code in nsScanner after
> this patch can get confused and assert fatally if you don’t also change the
> alias resolution in nsDocument::TryChannelCharset to use the new alias code.
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.
> cpp#3047
It's changed in attachment #678713 [details] [diff] [review].

Comment 42

[Masatoshi Kimura [:emk]]

The worker part can be landed separately.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/931d42b01133
https://hg.mozilla.org/integration/mozilla-inbound/rev/e85222a1e777
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4b9eb815efa
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2a8c4a79453
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0af6d983c0e
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb7d1fd8a868

Comment 44

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/931d42b01133
https://hg.mozilla.org/mozilla-central/rev/e85222a1e777
https://hg.mozilla.org/mozilla-central/rev/b4b9eb815efa
https://hg.mozilla.org/mozilla-central/rev/b2a8c4a79453
https://hg.mozilla.org/mozilla-central/rev/c0af6d983c0e
https://hg.mozilla.org/mozilla-central/rev/eb7d1fd8a868

Comment 45

[Masatoshi Kimura [:emk]]

Attachment: Reintroduce x-windows-949 hack

Now form submission uses encoders so we need this workaround again until bug 562091 is fixed.

Comment 46

[Masatoshi Kimura [:emk]]

Comment on attachment 673028 [details] [diff] [review]
Use FindEncodingForLabel from FileReaderSync

Henri, could you review this? This patch shouldn't require any knowledge specific to workers. I really want to land this part before aurora uplifting.

Comment 47

[Masatoshi Kimura [:emk]]

https://tbpl.mozilla.org/?tree=Try&rev=8ab49f84b6eb

Comment 48

[Henri Sivonen (:hsivonen)]

Comment on attachment 679731 [details] [diff] [review]
Reintroduce x-windows-949 hack

r+, but this is so sad. We really need to align our internal encoding names with the Encoding Standard.

Comment 49

[Henri Sivonen (:hsivonen)]

Comment on attachment 673028 [details] [diff] [review]
Use FindEncodingForLabel from FileReaderSync

I’m rather surprised that our old code assumed big endian with the "UTF-16" label and no BOM.

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1889ff1d55da
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e88fade2d0c

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1889ff1d55da
https://hg.mozilla.org/mozilla-central/rev/3e88fade2d0c

Comment 52

[Masatoshi Kimura [:emk]]

Since Firefox 19, Web pages can use only encodings defined in <http://encoding.spec.whatwg.org/>.

Comment 53

[Alex Keybl [:akeybl]]

This needs to be backed out of FF19 due to bug 782412, which caused "users will not be able to open Web pages encoded in UTF-16 big endian".

Comment 54

[Alex Keybl [:akeybl]]

(In reply to Alex Keybl [:akeybl] from comment #53)
> This needs to be backed out of FF19 due to bug 782412, which caused "users
> will not be able to open Web pages encoded in UTF-16 big endian".

Nevermind we'll take bug 814900 instead.