763617 - Local file as mail start page doesn't show up

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Matthias Bockelkamp]

In recent trunk builds, a local mail start page (file:///C:/Folder/page.htm) does not show up any more. Using a remote URL (http://www.web.de) works as expected.

Regression range is somewhere between 2012-04-22 and 2012-06-03.

Comment 1

[Matthias Bockelkamp]

I tried to find the exact regression date:

2012-05-24-00-30-03-comm-central-trunk is ok
2012-05-25-00-30-03-comm-central-trunk is broken

Comment 2

[Philip Chee]

http://hg.mozilla.org/comm-central/annotate/64605d5789cc/suite/mailnews/mailWindow.js#l490
Commenting out the following line in function loadStartPage() makes file urls work:

        messenger.setDisplayCharset("");

This line was added in Bug 59787 - Messenger Startup page in Mail viewing window displays incorrectly.

Thunderbird removed this line from their mailWindow.js in Bug 258447 - Mail Start Page ignores character setting. Do you think it's OK for us to do the same?

Comment 3

[Matthias Bockelkamp]

Philip, please tell me how you found that checkin. 

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2012-05-23+23%3A00&enddate=2012-05-25+00%3A00
http://hg.mozilla.org/comm-central/pushloghtml?startdate=2012-05-23+23%3A00&enddate=2012-05-25+00%3A00

The checkin you named is not listed there.

Comment 4

[Philip Chee]

A recent checkin just exposed a pre-existing bug. Thunderbird fixed their equivalent code in Bug 258447. I haven't identified the mozilla-central bug that changed the behaviour yet but it's probably one of the 118 changesets @
2552b0541f87	Ed Morley — Merge last PGO-green changeset of mozilla-inbound to mozilla-central.

Comment 5

[Philip Chee]

Further investigation shows:
It fails if the local file doesn't contain a <meta ... charset>
If the file contains something like: <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1" />, then the local file works.
If you load a non working local file in the browser the following shows up in the error console:

Wed Jun 13 2012 19:12:01
Error: The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must to be declared in the document or in the transfer protocol.
Source file: file:///C:/..../TestCases/foobar.html

Comment 6

[[e-mail]]

(In reply to Philip Chee from comment #2)
> Commenting out the following line in function loadStartPage() makes file
> urls work:
> 
>         messenger.setDisplayCharset("");
> 
> This line was added in Bug 59787 - Messenger Startup page in Mail viewing
> window displays incorrectly.
> 
> Thunderbird removed this line from their mailWindow.js in Bug 258447 - Mail
> Start Page ignores character setting. Do you think it's OK for us to do the
> same?

Previously SetDisplayCharset set the forced character set, which is why we had to unset it in order for the start page to detect its character set correctly. Bug 253807 changed this to set the hint character set, which resets itself on every page load, so doesn't need to be unset. I couldn't work out which code now relies on the character set not being empty; there is an assertion for unsupported character set strings but it fails to fire for the empty string.

Comment 7

[[e-mail]]

So, the code that now fails when the character set is empty is nsHtml5StreamParser::SetupDecodingAndWriteSniffingBufferAndCurrentSegment, which tries to get the unicode decoder for the specified character set, but falls back to windows-1252 if the charset converter manager returns NS_ERROR_UCONV_NOCONV. Unfortunately due to bug 710693 it no longer does so for an empty character set, instead it now returns NS_ERROR_NULL_POINTER, which confuses the HTML5 parser into thinking that the document is broken.

Comment 8

[Masatoshi Kimura [:emk]]

Attachment: Don't propagate an error code from GetCharsetAlias

Indeed XSS vulnerable encodings should never be detectable from callers of GetUnicodeDecoder.

Comment 9

[Simon Montagu :smontagu]

Comment on attachment 632794 [details] [diff] [review]
Don't propagate an error code from GetCharsetAlias

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

::: intl/uconv/src/nsCharsetConverterManager.cpp
@@ +178,5 @@
>  
>    // fully qualify to possibly avoid vtable call
>    nsresult rv = nsCharsetConverterManager::GetCharsetAlias(aSrc, charset);
>    if (NS_FAILED(rv))
> +    return NS_ERROR_UCONV_NOCONV;

You don't really need rv at all any more, you could just do 
if (NS_FAILED( ... 
  return NS_ERROR_UCONV_NOCONV;

but r=me either way.

Comment 10

[Masatoshi Kimura [:emk]]

Attachment: Don't propagate an error code from GetCharsetAlias. r=smontagu

patch for checkin

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0de74d9fd081

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0de74d9fd081

Comment 13

[Matthias Bockelkamp]

Verified with Build ID 20120615003002