Last Comment Bug 763617 - Local file as mail start page doesn't show up
: Local file as mail start page doesn't show up
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Internationalization (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: mozilla16
Assigned To: Masatoshi Kimura [:emk]
:
Mentors:
Depends on:
Blocks: 710693
  Show dependency treegraph
 
Reported: 2012-06-11 12:20 PDT by Matthias Bockelkamp
Modified: 2012-06-16 14:56 PDT (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Don't propagate an error code from GetCharsetAlias (1.09 KB, patch)
2012-06-13 11:50 PDT, Masatoshi Kimura [:emk]
smontagu: review+
Details | Diff | Splinter Review
Don't propagate an error code from GetCharsetAlias. r=smontagu (1.25 KB, patch)
2012-06-14 03:25 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Splinter Review

Description Matthias Bockelkamp 2012-06-11 12:20:31 PDT
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 2012-06-11 12:46:42 PDT
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 2012-06-12 12:14:10 PDT
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 4 Philip Chee 2012-06-12 20:45:43 PDT
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 2012-06-13 04:17:50 PDT
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 neil@parkwaycc.co.uk 2012-06-13 05:37:19 PDT
(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 neil@parkwaycc.co.uk 2012-06-13 06:42:48 PDT
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] 2012-06-13 11:50:41 PDT
Created attachment 632794 [details] [diff] [review]
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 2012-06-14 00:14:11 PDT
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] 2012-06-14 03:25:02 PDT
Created attachment 633087 [details] [diff] [review]
Don't propagate an error code from GetCharsetAlias. r=smontagu

patch for checkin
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-06-14 15:49:08 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/0de74d9fd081
Comment 12 Ed Morley [:emorley] 2012-06-15 05:57:36 PDT
https://hg.mozilla.org/mozilla-central/rev/0de74d9fd081
Comment 13 Matthias Bockelkamp 2012-06-16 14:56:06 PDT
Verified with Build ID 20120615003002

Note You need to log in before you can comment on or make changes to this bug.