113050 - 'Forward as attachments' for multiple IMAP messages broken in 0.9.6 (worked in 0.9.5)

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect, P2)

Description

[Russell Odom]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.5) Gecko/20011011
BuildID:    v0.9.6 (using a different version at the mo so don't know build ID)

When I select several messages in my IMAP Inbox (or any other folder, I assume),
right-click then select 'Forward as attachments', only one attachment appears in
the box - it seems to be a hash-separated list of the messages.
When I attempt to send the message, I get an error from my mailserver to the
effect that it can't find the specified messages.
This feature worked fine in 0.9.5 but not in 0.9.6 - all the messages were
displayed as separate attachments and sent with no problem.

Reproducible: Always
Steps to Reproduce:
1. Select several messages in window
2. Right click
3. Select 'Forward as attachments'
4. Enter an e-mail address for recipient
5. Click 'Send'


Actual Results:  Error message from IMAP server (haven't tried POP3) that it
couldn't find message with that ID

Expected Results:  Found all selected messages, attached them to the mail, and
sent it.

Works fine when sending single message as attachment

Comment 1

[Sheela Ravindran]

confirming on commercial build: 2001-12-03-06
Forwarding multiple messages(more than one) attaches only the first message
selected. (Forwarding as attachment).  We could forward multiple messages as
attachments before and we should do that. Since we do not do it inline which is
a known bug.  

Comment 2

[George Herson]

After I try to forward multiple messages as attachments, my Sent log shows not a
"hash-separated list of the messages" but only one of those attached emails. 
Judging by SpamCop.net's response to the forwarding, only one forwarded message
was received as well.  (Mozilla 0.9.6)

Comment 3

[Jean-Francois Ducarroz]

Tis is a regression caused by my checkin for bug 86089. in nsMsgCompose.cpp, in
the code:
          if (NS_SUCCEEDED(rv) && attachment)
          {
            attachment->SetName(decodedSubject.get());
            attachment->SetUrl(originalMsgURI);
            m_compFields->AddAttachment(attachment);
          }

originalMsgURI contains a comma separated list of url that need to be split a
part! Also, we need to query the subject of each messages and not reuse the
first one.

Comment 4

[Denis Antrushin]

Attachment: patch, v1

I'm not quite sure about use of GetTopmostMsgWindowCharacterSet and charset
override logic. Except this, multiple message forwarding seems to work...

Comment 5

[Denis Antrushin]

Attachment: (somewhat) better patch, v2

I just moved several line up-n-down and removed one unneeded SetAuthor() call.

Aso for charset handling, here's problem pieces of code:

GetTopmostMsgWindowCharacterSet(mailCharset, &mCharsetOverride);
if (mailCharset && (* (const PRUnichar *) mailCharset) )
{
  charset.Adopt(ToNewUTF8String(nsDependentString(mailCharset)));
  charsetOverride = PR_TRUE;
}
...
if (!charsetOverride)
{
  rv = msgHdr->GetCharset(getter_Copies(charset));
  if (NS_FAILED(rv)) return rv;
}
...
rv = mimeConverter->DecodeMimeHeader(subjectStr.get(),
	      getter_Copies(decodedString),
	      charset, charsetOverride);

The question is: shouldn't we use Topmost Window Charset only if window
says that charset override is on (mCharsetOverride == PR_TRUE) :
if (mCharsetOverride && mailCharset && (* (const PRUnichar *) mailCharset) ) ?

Comment 6

[Sheela Ravindran]

*** Bug 109734 has been marked as a duplicate of this bug. ***

Comment 7

[Jean-Francois Ducarroz]

Denis, can you post a patch without white space changes using the -w option.
That will make your patch easy to read. Thanks

Comment 8

[Denis Antrushin]

That's not whitespace noise - I had to move large block of code back and forward.
I think I'll just post patched version tomorrow when I'll come to work... 

Comment 9

[Denis Antrushin]

Attachment: diff -w version of the above

Sorry, it turned out to be really ws noise, not complex patch :-)
Here's diff -w output

Comment 10

[Jean-Francois Ducarroz]

I am not sure either about the overwrite charset stuff! But the fact you propose
something different for forward that for reply doesn't sound good. One of them
should be bogus!

cc'ing nhotta. Naoki, any advise?

Comment 11

[Jean-Francois Ducarroz]

About the patch, I see two problems:
1) Looks like you will set the new message subject for each URI. It should
contains the subject of the first URI and not the one of the last URI.

2) This code become a liitle bit too messy. We should be able to avoid
duplicating code!

Comment 12

[nhottanscp]

I am not sure how this bug is related to charset override. Denis, could you explain?

Comment 13

[Denis Antrushin]

> But the fact you propose something different for forward that for reply 
> doesn't sound good. One of them should be bogus!
In the case of reply you never get comma separated list of URIs!
JS code just creates separate compose window per selected message.
We can get list of URIs if and only if  we're forwarding as attachment.
Well, I can make this common for all types of composition - loop we'll
be just run once...

> 1) Looks like you will set the new message subject for each URI. It should
> contains the subject of the first URI and not the one of the last URI.
I set subject to space separated list of subjects of messages we're forwarding.
I just didn't know what else it should be. :-) I'll fix it.

> 2) This code become a liitle bit too messy. We should be able to avoid
> duplicating code!
I'll think about this. Do you have any advise or suggestion?

Comment 14

[Denis Antrushin]

> I am not sure how this bug is related to charset override. Denis, could you 
> explain?
I mean the logic of choosing what charset use for message composition.
Currently it's "If charset override of topmost message window is specified, use
it, otherwise use charset specified in message header". This works for single
message, but what to do with several selected messages? They all may have 
different charsets, so charset override may not work for all of them.
Some of them may not have MIME info in headers...so, how to choose charset?
It seems to me that there is no way to solve this problem for any possible
case. Will it be enough if multi-message forward will only work for messages
with either the same charset for all of them, or correct mime info in headers?

Comment 15

[nhottanscp]

For Reply/Forward override was always set, I recently fixed that problem. 
For multi part, charset of the part is used if specified. If not charset
specified then the default charset is used (forward inline has a bug 66098).

Comment 16

[Jean-Francois Ducarroz]

by:
But the fact you propose something different for forward that for reply doesn't
sound good. One of them should be bogus!

I was thinking about the overwrite charset part of the code.

For reply (the current code) we do always user the mail3Pane charset and set
overwrite to true, unless we don' have a mail3Pane charset.

For forward as attachment, you are proposing to use the mail3Pane charset only
if the function GetTopmostMsgWindowCharacterSet() return CharsetOverride to true!

both patch code should do the same and as Naki said, you should match the
existing code. To avoid any trouble with multiple message using different
charset, just use the subject of the first message, that will be the one which
is currently displayed and normally the user would have set the mail3Pane
charset to the correct value if needed.

Comment 17

[Denis Antrushin]

Attachment: patch, v3

How about this one?

Comment 18

[Denis Antrushin]

Attachment: diff -w version of the above

Comment 19

[Denis Antrushin]

As for charset problem, I still don't agree with existing code
(maybe I just don't understand it). 
Current code:
 rv = msgHdr->GetCharset(getter_Copies(charset));

 GetTopmostMsgWindowCharacterSet(mailCharset, &mCharsetOverride);
 if (mailCharset && (* (const PRUnichar *) mailCharset) )
 {
   charset.Adopt(ToNewUTF8String(nsDependentString(mailCharset)));
   charsetOverride = PR_TRUE;
 }

Note that charsetOverride not always equal to mCharsetOverride.
IMO, this code always uses charset of mail window (I assume it's
always set). Why then we ask for msgHdr charset?
Or it's possible for message window to not have charset set?
Well, I don't see much harm in this situation, cause message window
seems to always have right charset (is it true?), but in this case
we can remove unneeded msgHdr->GetCharset.
Am I saying something stupid again?

Comment 20

[nhottanscp]

I think you are right, GetTopmostMsgWindowCharacterSet was added later.
For the override, we used to override always, but now we only override when it's
explicitly requested. So, 'mCharsetOverride' should be used instead of
'charsetOverride' (i.e. 'charsetOverride' can be removed).
I am still not sure this bug is related to any of the charset issue. Is there
test cases of the problem so it can be verified?

Comment 21

[Denis Antrushin]

I didn't say this bug related to any charset issues :-).
I just was worried about charsetOverride and mCharsetOverride use.
(Huh, I should take few lessons of english :-))
Thanks for clarifying this issue. 

Comment 22

[nhottanscp]

I commented because I worry about changing without test cases. Would you be able
to file a separte bug and fix the charset problem there?
I think possible situations where the current code causes problems could be 
1) the message body charset and the subject header charset is different
2) no charset for message body but the subject has a charset

Comment 23

[Denis Antrushin]

Let's leave charset handling as is until I perform some testing.
If I'll find problems, I'll file new bug.
Jean-Fransois, could you please review latest patch (v3)?

Comment 24

[scottputterman]

moving out, but since it's got a patch, we should try to get for 0.9.9

Comment 25

[Jean-Francois Ducarroz]

I'll review it next week...

Comment 26

[Jean-Francois Ducarroz]

*** Bug 127217 has been marked as a duplicate of this bug. ***

Comment 27

[Jean-Francois Ducarroz]

>Let's leave charset handling as is until I perform some testing.
>If I'll find problems, I'll file new bug.
>Jean-Fransois, could you please review latest patch (v3)?

Denis, can you post a patch without those charset handling modification. Thanks

Comment 28

[Denis Antrushin]

See patch, v3 :-)
It doesn't change charset handling

Comment 29

[Jean-Francois Ducarroz]

ok, looking at it...

Comment 30

[Jean-Francois Ducarroz]

Comment on attachment 66437 [details] [diff] [review]
patch, v3

Looks good and works fine. I have only one little request. Please don't do:
goto DONE, instead do just break; or PR_Free(uriList); break;

Comment 31

[Denis Antrushin]

But I need to get out of outer while loop, not only switch statement

Comment 32

[Denis Antrushin]

Attachment: patch, v3.1

Replaced 'goto DONE' with 'PR_FREEIF(uriList); return rv'

Comment 33

[Jean-Francois Ducarroz]

Right but that doesn't really matter as you will come back to that same point
for each URL and then just break again. The potential small performance impact
is not important as this is an very unlikly scenario.

My concern is just that the use of goto is very uggly and should be avoided as
much as possible.

BTW, my second proposition should have been:

PR_Free(...)
return;

and not 

PR_Free(...)
break;

Comment 34

[Jean-Francois Ducarroz]

oops, I didn't saw your last comment.

Comment 35

[Jean-Francois Ducarroz]

The second part of the new patch (@@ -2208,25 +2214,35 @@) seems totally bogus!!!

Comment 36

[Denis Antrushin]

What's wrong with it? I don't see any problems.

Comment 37

[Jean-Francois Ducarroz]

Sorry my mistake, I totally missed the part about ProcessReplyFlags in my review
for patch v3.

Comment 38

[Jean-Francois Ducarroz]

Comment on attachment 70968 [details] [diff] [review]
patch, v3.1

R=ducarroz. I am seeking for SR now...

Thanks Denis and sorry again for the confusion during my review.

Comment 39

[R.K.Aa.]

*** Bug 127617 has been marked as a duplicate of this bug. ***

Comment 40

[David :Bienvenu]

&& charset.get() && charset.get()[0])

this can just be !charset.IsEmpty()

Also, the bracing style looks inconsistent - it looks like this file uses this
style:

if (a)
{
}
so you should use it in the new code.

also, you don't need braces if you have single statements in the if then else
clauses.

so 

+            if (NS_SUCCEEDED(rv) && decodedCString) {
+              m_compFields->SetTo(decodedCString);
+            } else {
+              m_compFields->SetTo(author);
+            }

shouldn't have any braces at all.

Other than that, it looks OK.

Comment 41

[Denis Antrushin]

Attachment: patch, v3.2

Code style fixed.

Comment 42

[David :Bienvenu]

couple more things:
+            if (!isFirstPass)       // safeguard, just in case...
+            {
+              PR_FREEIF(uriList);
+              return rv;
+            }

can just use PR_Free here, since PR_Free checks for null. PR_FREEIF also sets
uriList to null when done, which is a behaviour you don't need here (so it's
just wasted code).

+      if (isFirstPass && !charset.IsEmpty())
+      {
+        m_compFields->SetCharacterSet(charset);
+      }

don't need braces here.

If you fix those things, sr=bienvenu, and I don't need to review it again.

Comment 43

[David :Bienvenu]

Comment on attachment 71653 [details] [diff] [review]
patch, v3.2

sr=bienvenu, with comments above.

Comment 44

[Denis Antrushin]

Attachment: patch, v3.3

meet sr= conditions
(PR_FREEIF -> PR_Free and removed braces)

Comment 45

[Jean-Francois Ducarroz]

Comment on attachment 71670 [details] [diff] [review]
patch, v3.3

Per previous comment, R=ducarroz, SR=bienvenu.

I'll check it in as soon the tree open for 1.0

Comment 46

[Asa Dotzler [:asa]]

Comment on attachment 71670 [details] [diff] [review]
patch, v3.3

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

Comment 47

[Jean-Francois Ducarroz]

Fix checked in. Thanks Denis for the patch.

Comment 48

[Michael Lefevre]

*** Bug 130030 has been marked as a duplicate of this bug. ***

Comment 49

[Michael Lefevre]

*** Bug 132983 has been marked as a duplicate of this bug. ***

Comment 50

[Russell Odom]

Just tried this on W2K 2002032203 and all works. You are all wonderful people :-)

Comment 51

[esther]

Using build 20020322 one winxp, mac 9.1 and linux this is fixed.  I checked IMAP
and POP accounts. Verified

Comment 52

[Vadim Berezniker]

*** Bug 137156 has been marked as a duplicate of this bug. ***