250455 - accented characters not sent correctly with simple mapi blind send

Status: RESOLVED FIXED

(MailNews Core :: Simple MAPI, defect)

Description

[David :Bienvenu]

If you do a blind send of body text that contains accented characters, they're
not sent correctly. Fix upcoming.

Comment 1

[David :Bienvenu]

Attachment: proposed fix

this fix is against the m4 branch, so I'll need to port it to the trunk, and
the aviary branch.

The fix is to try to mimic what happens when we do a send from the plain text
compose window, in terms of charset conversions, and to skip entity conversion
at send time, if we're doing mapi.  I'm not sure this is the right way to do
all this, but I'm pretty sure things were pretty broken before, so this is
better.

Comment 2

[Jungshik Shin]

Comment on attachment 152630 [details] [diff] [review]
proposed fix

>Index: mapi/mapihook/src/msgMapiHook.cpp


>+      unicodeBody.AssignWithConversion(aMessage->lpszNoteText);

This should be either
NS_CopyNativeToUnicode(nsDependentCString(aMessage->lpszNoteText), unicodeBody)

or equivalent (but a bit 'generous' implementation in nsMsgI18NUtil.cpp). As it
is now, lpszNoteText (I'm assuming here it's in the platform character
encoding) in non-ISO-8859-1 will be incorrectly inflated to UTF-16.  

>+      if (unicodeBody.Last() != nsCRT::LF)
>+        unicodeBody.AppendWithConversion(CRLF);

   AppendASCIItoUTF16(CRLF, unicodeBody); 

(xxxWithConversion's are all deprecated, I think)

Comment 3

[Jungshik Shin]

Comment on attachment 152630 [details] [diff] [review]
proposed fix

I think we shouldn't change msgMapiHook.cpp to fix this problem.
|setBodyFromCString(in string)| is scary (at least it should be marked
[noscript]).

As it stands, it seems like the current code should just work.	
nsMapiHook::BlindSendMail seems to do the right thing (on the trunk), but may
not. Or, we may have to do some special-casing for the blind MAPI sending in
nsMsgCompose::SendMsg.


BTW, mapi/mapihook/src/msgMapiHook.cpp
has a number of incorrect inflation to UTF-16 with *WithConversion, but that
should be a separate bug.  I'll file a bug for that.

Comment 4

[David :Bienvenu]

I'm open to suggestions - if it works on the trunk, that's great. I'll try it. I
thought I did, but I may have just been debugging on a branch. 

nsMsgCompose::SendMsg doesn't know we're doing a blind send - and it's too late
at that point...

Comment 5

[Jungshik Shin]

(In reply to comment #4)
> I'm open to suggestions - if it works on the trunk, that's great. I'll try it. 
> I thought I did, but I may have just been debugging on a branch.

Sorry for the confusion. I didn't try it myself, but I thought it'd work because
 I incorrectly assumed that SendMsg would convert |m_body| in UTF-16 (nsString)
(PRUnichar/AString) to a C string in the mail charset. It turns out that
|m_body| is nsCString which is sometimes interpreted as UTF-8 and other times
as, well, a C string in some character encoding. I don't like this 'overloading'
hack  at all,  but it has worked so far.  I think we have to overhaul
|nsIMsgCompField| to remove that strange 'overloading'.  (now I recall I planned
to work on this....)

For this bug, would you address what I raised in comment #2? It'd be also nice
to add a very conspicuous warning to |SetBodyFromCString| in nsIMsgCompField.idl
that this only works with a particular implementation of the interface in which
'body' is stored in |nsString|. 

Comment 6

[Jungshik Shin]

> 'body' is stored in |nsString|

Sorry it's |nsCString| (or char*). (actually, PRUnichar* and nsString can be
used as an inflated storage for C strings, but that has to be avoided like plague)

Comment 7

[Jungshik Shin]

http://lxr.mozilla.org/seamonkey/source/mailnews/compose/src/nsMsgCompose.cpp#883

883 rv = nsMsgI18NSaveAsCharset(attachment1 [details] [diff] [review]_type,m_compFields->GetCharacterSet(), 
884      NS_ConvertASCIItoUCS2(bodyString).get(),&outCString,
885      nsnull, &isAsciiOnly);

This call must be the cause of the problem.  Obviously, NS_ConvertASCIItoUCS2()
is wrong when bodyString is in UTF-8. 
 

Comment 8

[Jungshik Shin]

David, can you try replacing NS_ConvertASCIItoUCS2() with
NS_ConvertUTF8toUTF16() without changing anything on the MAPI side? It's inside
|if (!entityConversionDone)| so that it should work. 

Comment 9

[David :Bienvenu]

that works, thx a lot - you rock!

Comment 10

[David :Bienvenu]

Attachment: proposed fix

Comment 11

[David :Bienvenu]

I'll check this in r/sr=me - thx again!

Comment 12

[Jungshik Shin]

Glad that the one-liner fixed it :-)

Comment 13

[David :Bienvenu]

this fix doesn't work for the plain text compose window. I'm trying to figure
out why...if you set your default compose window to be plain text, the
characters get changed to entities...

Comment 14

[Jungshik Shin]

(In reply to comment #13)
> this fix doesn't work for the plain text compose window. I'm trying to figure
> out why...if you set your default compose window to be plain text, the
> characters get changed to entities...

 Is it the case even if those characters are covered by your default mail-send
character encoding?  Even if your answer is no, we still have a bug here. It
should prompt users to pick how to proceed (or should use question marks in
text/plain)

Comment 15

[David :Bienvenu]

Yes, I'm pretty sure the characters are covered by my default mail-send
encoding. They're just your basic accented french characters. This is blind mapi
send, so we can't put up a prompt. I don't know what will happen if the chars
aren't in my default encoding. Keep in mind that I'm only talking about the mapi
blind send case - there is no actual compose window, but there's still code that
looks at the default compose window type...

I believe I know what's going on here - we're doing entity conversion even in
plain text, which is just wrong, for the mapi blind send case. I think I can fix
it - but I'm generally wrong about issues like this :-(

Comment 16

[David :Bienvenu]

Attachment: proposed fix

Comment 17

[David :Bienvenu]

Comment on attachment 156447 [details] [diff] [review]
proposed fix

this fix basically checks if there's no editor, we still convert the message
body to the default mail charset, and if there's no editor, and we're not doing
html compose, don't do html entity conversion.

Comment 18

[David :Bienvenu]

re-opening for plain text compose window case.

Comment 19

[Jungshik Shin]

Comment on attachment 156447 [details] [diff] [review]
proposed fix

r=jshin

Because I didn't realize that _SendMsg calls nsSaveAsCharset once more if
'EntityConversion' flag is not set here, I had a hard time why it didn't work
before and how this patch could fix it.. Now everything is clear :-)

>Index: nsMsgCompose.cpp

>     // The plain text compose window was used
>     const char contentType[] = "text/plain";
>     nsAutoString msgBody;
>     nsAutoString format;format.AssignWithConversion(contentType);

   While you're at it, can you make 'format' NS_NAMED_LITERAL_STRING' (i.e. |
NS_NAMED_LITERAL_STRING(format, "text/plain");|)? 
And, perhaps it'd be also a good idea to change msgBody to nsString because
it's likely to be longer than 64 units.

>+    if (m_editor)
>+    {
	.....
>+      m_compFields->SetBody((const char *)nsnull);
> 
>     const char *charset = m_compFields->GetCharacterSet();
>     if(UseFormatFlowed(charset))
>         flags |= nsIDocumentEncoder::OutputFormatFlowed;

The 3 lines above should be indented further now that they're in |if
(m_editor)| clause.

Comment 20

[Jungshik Shin]

(In reply to comment #19)

>    While you're at it, can you make 'format' NS_NAMED_LITERAL_STRING' (i.e. |
> NS_NAMED_LITERAL_STRING(format, "text/plain");|)? 

It turned out that 'format' is used only once in the following line:

 rv = m_editor->OutputToString(format, flags, msgBody);

Then, we should just use NS_LITERAL_STRING

 rv = m_editor->OutputToString(NS_LITERAL_STRING("text/plain"),......);

Comment 21

[David :Bienvenu]

thx, I'll address your comments before checking in - except for the indentation,
which is just because it's a -w diff. 

Comment 22

[David :Bienvenu]

fixed

Comment 23

[Jungshik Shin]

David, how about aviary-1.0 and 1.7branch? Both patches here seem simple enough
for branch check-ins.

Comment 24

[David :Bienvenu]

it is fixed on the aviary branch - I'll ask for 1.7 branch approval. I can't
remember if your other fix went into the 1.7 branch or not - I don't think so.
I'll ask approval for both.

Comment 25

[Mike Kaply [:mkaply]]

Comment on attachment 156447 [details] [diff] [review]
proposed fix

I didn't approve the other patch because it still doesn't have a review flag
set.

a=mkaply

Comment 26

[Mike Kaply [:mkaply]]

Comment on attachment 153528 [details] [diff] [review]
proposed fix

a=mkaply