Since bug 254519 is fixed, header values containing unencoded special characters (eg. umlauts) break at the first of these characters in the headerpane display. Thunderbird doesn't seem to be affected.

Note: unencoded special characters are not allowed in the headers anyway.

Yes, but we did "guess" before what was meant; and even without guessing we should replace illegal characters by � (u+FFFD)...

...and not drop the rest of the string.

Thunderbird trunk is affected as well. Steps to reproduce: - Drop the attached mbox into your eg. Local Folders folder. - Set your default display encoding to UTF-8. - Get a message with unencoded ISO-8859-1 characters like the one attached. - The Subject: in threadpane and headerpane shows only �s from the first illegal byte onward instead of just replacing the offending byte. - The From: is blank in the threadpane; the From: is cut off at the first illegal byte in the headerpane. - The illegal umlauts in the body are correctly replaced by one � each.

The frontend gets passed already broken stuff, a debug build will show: ###!!! ASSERTION: not a UTF8 string: 'Error', file ../../dist/include/string/nsUTF8Utils.h, line 130 ###!!! ASSERTION: length mismatch: 'calculator.Length() == converter.Length()', file /home/kd/projekte/mozilla/mozilla.org/src/trunk/mozilla/xpcom/string/src/nsReadableUtils.cpp, line 444 ###!!! ASSERTION: Not a UTF-8 string. This code should only be used for converting from known UTF-8 strings.: 'Error', file ../../../dist/include/string/nsUTF8Utils.h, line 516 ###!!! ASSERTION: not a UTF8 string: 'Error', file ../../dist/include/string/nsUTF8Utils.h, line 130 ###!!! ASSERTION: Input wasn't UTF8 or incorrect length was calculated: 'Error', file /home/kd/projekte/mozilla/mozilla.org/src/trunk/mozilla/xpcom/string/src/nsReadableUtils.cpp, line 287

Bug 471423 also has an unencoded Big5 testcase. I'm currently investigating the issue.

(In reply to comment #0) > Since bug 254519 is fixed, header values containing unencoded special > characters (eg. umlauts) break at the first of these characters in the > headerpane display. And breaks fully in the threadpane. Screenshot: http://www.flickr.com/photos/83837423@N00/3371877887/sizes/l/

How big of a regression is this? How visible is it to most users? Does it only happen for emails received from broken clients?

Here is anexample. Look at the different effect in threadpane, headerpane and compose window. I am seeing this nearly daily on newsgroups.

The regression is very visible in non-English environments where e.g. (older?) Outlook (and Express) versions are common. I still think that the fix in bug 254519 was partly wrong (and that taught me not to trunk-port patches for others without looking closely at them, even if they are reviewed :-( ), but I haven't had the time yet to dig into this for real...

Karsten, are you still busy? I've made a patch to fix the problem. Can I take your bug?

Sure!

We can't use CopyUTF8toUTF16 / NS_ConvertUTF8toUTF16 because message header may contain any octet sequence. Also, we need to see folder charset in case charset is not available from the message itself.

> - The Subject: in threadpane and headerpane shows only �s from the first > illegal byte onward instead of just replacing the offending byte. This is not a regression of bug 254519. We didn't change subject handling. That said, I've also made a patch for this issue. This patch is independent of the other parts of patch.

Another reliable way of triggering the assertions in comment #5: - Subscribe to news.newsfan.net, then subscribe to any Chinese newsgroup (which shows as a jumbled mix of characters) - Subscribe to a Chinese group, then download messages. Lots of assertions shown. Gentle nudge to bienvenu for the review + superreview. :)

Bienvenu, could you review the patches?

Comment on attachment 379331 [details] [diff] [review] patch (mailnews part) sorry for the delay - Neil might be better for looking at the string stuff, if he has time...

Comment on attachment 379331 [details] [diff] [review] patch (mailnews part) > rv = aHdr->GetCharset(getter_Copies(charset)); >+ if (NS_FAILED(rv) || charset.IsEmpty() || charset.Equals("us-ascii") || >+ charsetOverride) I would write this as if (charsetOverride || NS_FAILED(aHdr->GetCharset(getter_Copies(charset)) || charset.IsEmpty() || charset.Equals("us-ascii")) (lines wrapped for Bugzilla readability) >+ const char* cur = inString.get(); >+ const char* end = cur + inString.Length(); Use BeginReading() and EndReading() >+ PRInt32 c = PRUint8(*cur++); >+ if (c & 0x80) char c = *cur++; if (c & char(0x80)) [xpcom/string actually uses char(~0x7F)] >+ nsAutoString utf16Text; >+ nsresult rv = ConvertToUnicode(charset.get(), inString, utf16Text); >+ if (NS_FAILED(rv)) I think I'd prefer if (NS_SUCCEDED(rv)) { CopyUTF16toUTF8(utf16Text, outString); return; } >+ // EF BF BD (UTF-8 encoding of U+FFFD) >+ static const char utf8ReplacementChar[] = "\357\277\275"; Use NS_NAMED_LITERAL_STRING >+ ConvertRawBytesToUTF16(value.get(), opt->default_charset, output); >+ CopyUTF16toUTF8(output, value); Use ConvertRawBytesToUTF8 Rest looks OK to me but I didn't test it so setting sr+ instead of r+

Comment on attachment 379332 [details] [diff] [review] patch (core part) [Checkin: Comment 38 & 43] thx for the patch.

Comment on attachment 389240 [details] [diff] [review] [backed out] (mailnews part) addressed review comment thx for the patch!

Masatoshi, if you run a debug build, and click on this message, we get a ton of assertions in nsMimeConverter::DecodeMimeHeader() because the string isn't a UTF8 string. I'm wondering if you could look at that code and propose a fix for that assertion...

(In reply to comment #26) > Masatoshi, if you run a debug build, and click on this message, we get a ton of > assertions in nsMimeConverter::DecodeMimeHeader() because the string isn't a > UTF8 string. If you get assertions without a patch, my patch should also fix this. If my patch doesn't help, please attach an offending message. I get no assertions using the patched build at the moment.

Does it matter if the mozilla-central and comm-central patches land separately? Are they both needed to fix the bug? Will we be able to get the mozilla-central part on 1.9.1?

(In reply to comment #28) > Does it matter if the mozilla-central and comm-central patches land separately? You can land both patches separately. > Are they both needed to fix the bug? The mozilla-central patch is required to fix the subject issue. The comm-central one is for all the rest. > Will we be able to get the mozilla-central part on 1.9.1? It can be applied to 191 cleanly.

(In reply to comment #27) > If you get assertions without a patch, my patch should also fix this. > If my patch doesn't help, please attach an offending message. I get no > assertions using the patched build at the moment. I hadn't applied the mozilla-central patch - I'm not getting any assertions anymore - the assertions were from the test case in this bug. Thx for fixing this!

Comment on attachment 389240 [details] [diff] [review] [backed out] (mailnews part) addressed review comment Checked in: http://hg.mozilla.org/comm-central/rev/a8ac240e2ba4

Comment on attachment 389240 [details] [diff] [review] [backed out] (mailnews part) addressed review comment Backed out, this caused test failures (that look like crashes) in the mailnews tests: TEST-UNEXPECTED-FAIL | /buildbot/linux-comm-1.9.1-check/build/objdir/mozilla/_tests/xpcshell/test_bayes/unit/test_bug228675.js | test failed (with xpcshell return code: -11), see following log: >>>>>>> ### XPCOM_MEM_LEAK_LOG defined -- logging leaks to /tmp/runxpcshelltests_leaks.log TEST-INFO | (xpcshell/head.js) | test 1 pending Directory request for: MailD that we (mailDirService.js) are not handling, leaving it to another handler. Directory request for: MFCaF that we (mailDirService.js) are not handling, leaving it to another handler. Directory request for: DefRt that we (mailDirService.js) are not handling, leaving it to another handler. TEST-INFO | (xpcshell/head.js) | test 2 pending Directory request for: CurWorkD that we (mailDirService.js) are not handling, leaving it to another handler. TEST-INFO | (xpcshell/head.js) | test 2 finished TEST-INFO | (xpcshell/head.js) | running event loop <<<<<<< and several other files.

Yep, it's a crash. Doesn't seem related to the bayes code, which the test is purportedly about. Here's a partial stack: msvcr80d.dll!1023b130() [Frames below may be incorrect and/or missing, no symbols loaded for msvcr80d.dll] > mime.dll!nsCharTraits<char>::length(const char * s=0x00000000) Line 629 + 0x9 bytes C++ mime.dll!nsDependentCString::nsDependentCString(const char * data=0x00000000) Line 90 + 0x12 bytes C++ mime.dll!MimeHeaders_convert_header_value(MimeDisplayOptions * opt=0x03610f60, nsCString & value={...}, int convert_charset_only=0x00000001) Line 76 + 0x13 bytes C++ mime.dll!MimeHeaders_write_all_headers(MimeHeaders * hdrs=0x02402df8, MimeDisplayOptions * opt=0x03610f60, int attachment=0x00000000) Line 616 + 0x11 bytes C++ mime.dll!MimeMessage_write_headers_html(MimeObject * obj=0x03611200) Line 757 + 0x15 bytes C++ mime.dll!MimeMessage_close_headers(MimeObject * obj=0x03611200) Line 428 + 0x9 bytes C++ mime.dll!MimeMessage_parse_line(const char * aLine=0x023fa490, int aLength=0x00000001, MimeObject * obj=0x03611200) Line 277 + 0x9 bytes C++ mime.dll!convert_and_send_buffer(char * buf=0x023fa490, int length=0x00000001, int convert_newlines_p=0x00000001, int (char *, unsigned int, void *)* per_line_fn=0x02cccce0, void * closure=0x03611200) Line 184 + 0xf bytes C++ mime.dll!mime_LineBuffer(const char * net_buffer=0x03622b5c, int net_buffer_size=0x0000003b, char * * bufferP=0x03611228, int * buffer_sizeP=0x03611230, unsigned int * buffer_fpP=0x03611238, int convert_newlines_p=0x00000001, int (char *, unsigned int, void *)* per_line_fn=0x02cccce0, void * closure=0x03611200) Line 272 + 0x1d bytes C++ mime.dll!MimeObject_parse_buffer(const char * buffer=0x03622ac0, int size=0x000000d7, MimeObject * obj=0x03611200) Line 275 + 0x31 bytes C++ mime.dll!mime_display_stream_write(_nsMIMESession * stream=0x036112d0, const char * buf=0x03622ac0, int size=0x000000d7) Line 949 + 0x16 bytes C++ mime.dll!nsStreamConverter::OnDataAvailable(nsIRequest * request=0x0361001c, nsISupports * ctxt=0x0360f848, nsIInputStream * aIStream=0x03611390, unsigned int sourceOffset=0x00000000, unsigned int aLength=0x000000d7) Line 957 + 0x1a bytes C++ msglocal.dll!nsMailboxProtocol::ReadMessageResponse(nsIInputStream * inputStream=0x03611390, unsigned int sourceOffset=0x00000000, unsigned int length=0x000000d7) Line 585 + 0x50 bytes C++ msglocal.dll!nsMailboxProtocol::ProcessProtocolState(nsIURI * url=0x0360f850, nsIInputStream * inputStream=0x03611390, unsigned int offset=0x00000000, unsigned int length=0x000000d7) Line 682 + 0x14 bytes C++ msgbsutl.dll!nsMsgProtocol::OnDataAvailable(nsIRequest * request=0x036119f0, nsISupports * ctxt=0x0360f848, nsIInputStream * inStr=0x03611390, unsigned int sourceOffset=0x00000000, unsigned int count=0x000000d7) Line 351 + 0x22 bytes C++ necko.dll!nsInputStreamPump::OnStateTransfer() Line 508 + 0x40 bytes C++ necko.dll!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream * stream=0x03611390) Line 398 + 0xb bytes C++ xpcom_core.dll!nsInputStreamReadyEvent::Run() Line 112 C++ xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=0x00000001, int * result=0x0012bcd8) Line 511 C++ xpcom_core.dll!NS_InvokeByIndex_P(nsISupports * that=0x00000008, unsigned int methodIndex=0x00000002, unsigned int paramCount=0x0012bcc8, nsXPTCVariant * params=0x02100178) Line 102 C++

Comment on attachment 389240 [details] [diff] [review] [backed out] (mailnews part) addressed review comment >+void nsMsgI18NConvertRawBytesToUTF16(const nsCString& inString, >+ const nsCString& charset, >+ nsAString& outString) >+{ >+ if (IsUTF8(inString)) >+ { >+ CopyUTF8toUTF16(inString, outString); >+ return; >+ } >+ >+ nsresult rv = ConvertToUnicode(charset.get(), inString, outString); ConvertToUnicode accepts a null charset (equivalent to ASCII), so to fix the crash we could change the signature of the second parameter of these conversion functions to const char *charset and pass charset.get() in everywhere... >+ ConvertRawBytesToUTF8(value, nsDependentCString(opt->default_charset), >+ output); ...except here where you just pass opt->default_charset of course.

Changed charset parameter per comment #34. This patch passed all mailnews tests.

Comment on attachment 390590 [details] [diff] [review] patch (mailnews part) v3 [Checkin: Comment 37] yes, all mailnews tests pass. Thx for the new patch.

Comment on attachment 390590 [details] [diff] [review] patch (mailnews part) v3 [Checkin: Comment 37] Checked in: http://hg.mozilla.org/comm-central/rev/1ad44651b264 I'll do the mozilla-central patch tomorrow.

Comment on attachment 379332 [details] [diff] [review] patch (core part) [Checkin: Comment 38 & 43] Checked in to trunk: http://hg.mozilla.org/mozilla-central/rev/53abd74e07d0

Marking as fixed and adding tb3needs. The tb3needs flag will keep this on the lists so that we can hopefully drive the core patch into 1.9.1. Masatoshi: please can you request approval on the core patch in a day or so once this has baked a little, and give the appropriate risk analysis/details that drivers need? Thanks.

Comment on attachment 379332 [details] [diff] [review] patch (core part) [Checkin: Comment 38 & 43] The changed code path will be executed only if aDefaultCharset is specified. http://mxr.mozilla.org/mozilla-central/source/netwerk/mime/src/nsMIMEHeaderParamImpl.cpp#572 However, browser calls it without aDefaultCharset. http://mxr.mozilla.org/mozilla-central/source/netwerk/mime/src/nsMIMEHeaderParamImpl.cpp#101 And this is an only caller from browser. Therefore this change should not affect Firefox. (I'm not sure why this code belongs to the core...)

Comment on attachment 379332 [details] [diff] [review] patch (core part) [Checkin: Comment 38 & 43] Not for 1.9.1.2.

Comment on attachment 379332 [details] [diff] [review] patch (core part) [Checkin: Comment 38 & 43] Approved for 1.9.1.4, a=dveditz for release-drivers

Comment on attachment 379332 [details] [diff] [review] patch (core part) [Checkin: Comment 38 & 43] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9d244916bf08

If IMAP, same problem as this bug still occurs even after fix of this bug. Bug 513472 is IMAP version of this bug.