I18N characters printed incorrectly in BCC

VERIFIED FIXED in mozilla1.2alpha

Status

MailNews Core
Internationalization
P3
normal
VERIFIED FIXED
18 years ago
10 years ago

People

(Reporter: mkaply, Assigned: Jean-Francois Ducarroz)

Tracking

({intl})

Trunk
mozilla1.2alpha
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

18 years ago
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

18 years ago
I can reproduce this.
Reassign to ducarroz.
Assignee: nhotta → ducarroz
Keywords: intl

Comment 2

18 years ago
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

18 years ago
QA contact to marina.
QA Contact: momoi → marina
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.6
(Assignee)

Comment 4

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

Comment 5

17 years ago
moving to 0.9.7
Target Milestone: mozilla0.9.6 → mozilla0.9.7

Updated

17 years ago
Target Milestone: mozilla0.9.7 → mozilla0.9.9

Updated

17 years ago
Keywords: nsbeta1+
Priority: -- → P3

Comment 6

17 years ago
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

17 years ago
Created attachment 61579 [details] [diff] [review]
patch, v1

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

Updated

17 years ago
Keywords: patch, review
(Assignee)

Comment 8

17 years ago
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;
+    }
Attachment #61579 - Flags: needs-work+
(Assignee)

Comment 9

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

Comment 10

17 years ago
Created attachment 62516 [details] [diff] [review]
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...
(Assignee)

Comment 11

17 years ago
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?
(Assignee)

Comment 12

17 years ago
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

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

Comment 14

17 years ago
Created attachment 62944 [details] [diff] [review]
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
(Assignee)

Comment 15

17 years ago
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!
(Assignee)

Updated

17 years ago
Attachment #62516 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #61579 - Attachment is obsolete: true
(Assignee)

Comment 16

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

R=ducarroz
Attachment #62944 - Attachment description: patch, v3 → patch, v3 (part 1)
Attachment #62944 - Flags: review+
(Assignee)

Comment 17

17 years ago
Created attachment 62948 [details] [diff] [review]
patch, v3 (part 2)

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

Comment 18

17 years ago
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?
(Assignee)

Comment 19

17 years ago
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!
(Assignee)

Comment 20

17 years ago
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.
Attachment #62948 - Flags: needs-work+

Comment 21

17 years ago
Created attachment 63005 [details] [diff] [review]
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

17 years ago
Created attachment 63006 [details] [diff] [review]
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?
(Assignee)

Comment 23

17 years ago
compose fields should not contains mime encoded string!

Comment 24

17 years ago
Created attachment 63360 [details] [diff] [review]
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

17 years ago
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

17 years ago
Created attachment 63948 [details] [diff] [review]
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

17 years ago
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.
Blocks: 122274
Keywords: nsbeta1+ → nsbeta1-
Target Milestone: mozilla0.9.9 → mozilla1.2
(Assignee)

Comment 28

17 years ago
I'll review once I get some spare time...

Comment 29

17 years ago
The patch will fix nsbeta1+ bug 129256.

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


 

Comment 30

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

r=nhotta
Attachment #63948 - Flags: review+
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

17 years ago
Created attachment 73703 [details] [diff] [review]
Changed variable names and use strlen instead of PL_strlen.
Attachment #62944 - Attachment is obsolete: true
Attachment #62948 - Attachment is obsolete: true
Attachment #63005 - Attachment is obsolete: true
Attachment #63006 - Attachment is obsolete: true
Attachment #63360 - Attachment is obsolete: true
Attachment #63948 - Attachment is obsolete: true
Comment on attachment 73703 [details] [diff] [review]
Changed variable names and use strlen instead of PL_strlen.

sr=sspitzer
Attachment #73703 - Flags: superreview+

Comment 34

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

r=nhotta
Attachment #73703 - Flags: review+

Updated

17 years ago
Blocks: 129256

Comment 35

17 years ago
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
Attachment #73703 - Flags: approval+

Comment 36

17 years ago
checked in to the trunk

Thanks Denis.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 37

16 years ago
verified with 2002-04-16 trunk build, the source and the printed page show the 
non-ascii chars in the Bcc field correctly
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.