64092 - I18N characters printed incorrectly in BCC

Status: VERIFIED FIXED

(MailNews Core :: Internationalization, defect, P3)

Description

[Mike Kaply [:mkaply]]

Steps to create:

Create an address book entry with the name having special characters like δλοφό.

Compose a message and specify this person as the To, CC, and BCC.

Save the message to the Drafts folder.

Open the message in the Drafts folder.

Print it.

Notice that To and CC display δλοφό but BCC displays gibberish.

Comment 1

[nhottanscp]

I can reproduce this.
Reassign to ducarroz.

Comment 2

[Katsuhiko Momoi]

The problem is not limited to printing.
You can see the problem in Mail View source.
BCC line does not go through MIME-encoding
process while  the visible lines like To and
CC do.

Comment 3

[Katsuhiko Momoi]

QA contact to marina.

Comment 4

[Jean-Francois Ducarroz]

should be simple to fix. Looks like we miss something for BCC in the code

Comment 5

[scottputterman]

moving to 0.9.7

Comment 6

[Denis Antrushin]

I suspect this something is in nsMsgComposeAndSend::MimeDoFCC().
I have a working patch, but there's one thing I don't understand:
I don't have mail.strictly_mime_headers pref set, so when I use
nsMsgMIMEGetConformToStandard() for BCC field it's not get encoded.
At the same time, other headers (To:, Cc:) gets encoded even if
nsMsgMIMEGetConformToStandard() returns false.
Jean-Francois, can you shed some light on this?

Comment 7

[Denis Antrushin]

Attachment: patch, v1

Encode BCC field before writing it out.
I'm still confused by nsMsgMIMEGetConformToStandard() though,
need Jean-Francois's advice...

Comment 8

[Jean-Francois Ducarroz]

Comment on attachment 61579 [details] [diff] [review]
patch, v1

the code:
+    if (convBcc)
+    {
+      bcc_header = convBcc;
+      PR_Free(convBcc);
+    }

sound bogus, should be:

+    if (convBcc)
+    {
+     PR_Free(bcc_header);
+      bcc_header = convBcc;
+    }

Comment 9

[Jean-Francois Ducarroz]

nsMsgMIMESetConformToStandard confuse me too, looks like we have to way to
encode headers!

Comment 10

[Denis Antrushin]

Attachment: patch, v2

Damn it, I cannot believe I made SO stupid patch :-(((
Here is another one.
I'm not sure that we can change bcc_header, cause it
points to mCompFields's member variable...

Comment 11

[Jean-Francois Ducarroz]

Following your mistake in the first patch (I haven't reviewed the second one), I
am concerned that you are submitting a patch without testing it!

How have you tested the second one?

Comment 12

[Jean-Francois Ducarroz]

The second patch doesn't work either because the lenght L is not correctly
calculated in case you convert bcc. I have a fix for that but I am still not
able to correctly edit the draft! looks like we have a problem in mime when we
read the bcc field...

Comment 13

[Denis Antrushin]

Yes, second patch is not working, sorry. 
I'm trying to make proper patch...

Comment 14

[Denis Antrushin]

Attachment: patch, v3 (part 1)

Here is proper patch.
As for editing draft problem, this is wierd... value of Bcc: field is
correctly set in nsMsgCompFields when loading draft (it's in utf8, as well
as To: and other headers).
There must be place, where bcc: treated differently than to:, but I cannot
find it yet

Comment 15

[Jean-Francois Ducarroz]

the problem with bcc when editing the draft comes when we setup the bcc field in
nsMsgCompose::CreateMessage. Looks like we are missing a conversion!

Comment 16

[Jean-Francois Ducarroz]

Comment on attachment 62944 [details] [diff] [review]
patch, v3 (part 1)

R=ducarroz

Comment 17

[Jean-Francois Ducarroz]

Attachment: patch, v3 (part 2)

I removed unnecessary and bogus conversion between UTF8 and Unicode when adding
extra reply-to and bcc recipients.

Comment 18

[Denis Antrushin]

I still don't understand, why it's been working for replyToStr, but not
for bccStr. The code looks identical to me. What I'm missing?

Comment 19

[Jean-Francois Ducarroz]

It's because we get the reply to from the identity wich use ASCII while we get
the BCC from the compose field (in that particular case) which is in UTF8. At
least that my explanation!

Comment 20

[Jean-Francois Ducarroz]

Comment on attachment 62948 [details] [diff] [review]
patch, v3 (part 2)

this fix breaks reply-to headers using multi-byte language like japanese! Don't
know about bcc.

Comment 21

[Denis Antrushin]

Attachment: patch part2 v3.1

This (DecodeMimeHeader on nsMsgCompFields data) works for me (tested with 
koi8-r encoding, which is 8-bit). May be it's possible to decrease number
of string conversion here, but I'm not expert in that field :-(.
Also, I'm not sure if this is good or bad to have mime encoded headers in
nsMsgCompFields - probably we should decode headers before setting them in comp

fields?

Comment 22

[Denis Antrushin]

Attachment: patch part2 v3.1

This (DecodeMimeHeader on nsMsgCompFields data) works for me (tested with 
koi8-r encoding, which is 8-bit). May be it's possible to decrease number
of string conversion here, but I'm not expert in that field :-(.
Also, I'm not sure if this is good or bad to have mime encoded headers in
nsMsgCompFields - probably we should decode headers before setting them in comp

fields?

Comment 23

[Jean-Francois Ducarroz]

compose fields should not contains mime encoded string!

Comment 24

[Denis Antrushin]

Attachment: patch part2 v3.2

it turned out that comp fields wasn't mime encoded. Here is what I think
is a correct patch. I think the reason of the bug is mismatched GetXXX/SetXXX
calls - getter was used in ascii version (no charset conversion) and
setter was used in unicode version (with charset conversion). I wonder if it's
possible to avoid all that multiple string conversions (i.e. assign
nsXPIDLCString to nsXPIDLString - that woodoo string magic doc is really 
incomplete :-( ) I also made it that we call Get/Set stuff only when really
needed...  (Patch tested with koi8-r encoding)

Comment 25

[Denis Antrushin]

It's also possible to use opposite pair of Get/Set method: just replace

   m_compFields->SetBcc(bccStr.get());

with

   nsXPIDLCString tmp;
   tmp.Adopt(ToNewCString(bccStr));
   m_compFields->SetBcc(tmp);

which may be faster than v3.2 version 

Comment 26

[Denis Antrushin]

Attachment: patch v4 - the whole thing for review

Wow, I found how to deal with reply-to and bcc fields using
only nsXPIDLCString, so I get rid of ascii<->unicode conversions!
Ready for review, tested with koi8-r mail

Comment 27

[scottputterman]

adding nsbeta1- and moving out.  But since it looks like we have a patch, when
it's ready and reviewed, feel free to move it back.

Comment 28

[Jean-Francois Ducarroz]

I'll review once I get some spare time...

Comment 29

[nhottanscp]

The patch will fix nsbeta1+ bug 129256.

The reply part looks fine. I will review the bcc part next week.


 

Comment 30

[nhottanscp]

Comment on attachment 63948 [details] [diff] [review]
patch v4 - the whole thing for review

r=nhotta

Comment 31

[(not reading, please use [e-mail] instead)]

two small nits, then sr=sspitzer

1)

+      PRBool  aBool1, aBool2;

two problems here.  first, aFoo notation is for function / method arguments.
these are local variables.  second, these names contain no information.
instead of aBool1 and aBool2, do

PRBool bccSelf, bccOthers;

2)

+    PRInt32 L = PL_strlen(convBcc ? convBcc : bcc_header) + 20;

use strlen() instead

Comment 32

[nhottanscp]

Attachment: Changed variable names and use strlen instead of PL_strlen.

Comment 33

[(not reading, please use [e-mail] instead)]

Comment on attachment 73703 [details] [diff] [review]
Changed variable names and use strlen instead of PL_strlen.

sr=sspitzer

Comment 34

[nhottanscp]

Comment on attachment 73703 [details] [diff] [review]
Changed variable names and use strlen instead of PL_strlen.

r=nhotta

Comment 35

[Asa Dotzler [:asa]]

Comment on attachment 73703 [details] [diff] [review]
Changed variable names and use strlen instead of PL_strlen.

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 36

[nhottanscp]

checked in to the trunk

Thanks Denis.

Comment 37

[marina]

verified with 2002-04-16 trunk build, the source and the printed page show the 
non-ascii chars in the Bcc field correctly