584962 - automatic CC to self stripped on Reply All

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Walter Doekes]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US) AppleWebKit/533.4 (KHTML, like Gecko) Chrome/5.0.375.125 Safari/533.4
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; nl; rv:1.9.2.4) Gecko/20100608 Thunderbird/3.1

I have automatic CC set to my own email address. When I hit Reply, my CC address is added like expected. When I hit Reply All, my CC address isn't.

I'm not 100% sure, but I expect the problem to be in the code below. First my address is added to the CC list and then Reply All triggers the needToRemoveDup code below. That code ensures that my own e-mail address is stripped, even though I want it to be in there.


http://hg.mozilla.org/releases/comm-1.9.2/file/c1186371ddee/mailnews/compose/src/nsMsgCompose.cpp

  2431 NS_IMETHODIMP QuotingOutputStreamListener::OnStopRequest(nsIRequest *request, nsISupports *ctxt, nsresult status)
...
  2521         if (type == nsIMsgCompType::ReplyAll)
...
  2550           needToRemoveDup = PR_TRUE;
...
...
  2722           if (mIdentity)
  2723           {
  2724             nsCString email;
  2725             mIdentity->GetEmail(email);
  2726             addressToBeRemoved.AppendLiteral(", ");
  2727             addressToBeRemoved.Append(email);
  2728             rv = parser->RemoveDuplicateAddresses(nsDependentCString(_compFields->GetTo()),
  2729                                                   email, resultStr);
  2730             if (NS_SUCCEEDED(rv))
  2731             {
  2732               if (type == nsIMsgCompType::ReplyAll && !mailFollowupTo.IsEmpty())
  2733                 _compFields->SetTo(resultStr.get());
  2734             }
  2735           }
  2736           rv = parser->RemoveDuplicateAddresses(nsDependentCString(_compFields->GetCc()),
  2737                                                 addressToBeRemoved, resultStr);

Reproducible: Always

Steps to Reproduce:
1. Set your own email address in Account Settings -> .. -> CC to these addresses.
2. Find a mail sent to you.
3a. Click Reply: see your address in Cc and the original From in the To field.
3b. Click Reply All: (diverges)
Actual Results:  
3b. Click Reply All: see only the original From in the To field.

Expected Results:  
3b. Click Reply All: see your address in Cc and the original From in the To field.

There is a different bug in which the Cc isn't added for regular replies either if the original mail contains a Reply-To. If you do not get your e-mail address Cc in step (3a) you might see that bug.


Proposed fix: if the code is indeed broken in the place that I think it is, one could add a check against the auto-CC option there. An alternative fix is to add the auto-CC email address first after the above code has run.


Regards,
Walter Doekes
OSSO B.V.

Comment 1

[Walter Doekes]

The different bug that I mention is actually the same:

  2619           else if (! replyTo.IsEmpty())
  2620           {
  2621             // default behaviour for messages without Mail-Reply-To
  2622             compFields->SetTo(replyTo);
  2623             needToRemoveDup = PR_TRUE;
  2624           }

needToRemoveDup is set to true if the Reply-To field is non-empty causing my CC-to-self to get stripped in the regular Reply case as well.

Comment 2

[Ludovic Hirlimann [:Usul]]

Patch are welcome :-)

Comment 3

[richard]

Confirming that this bug still exists in Miramar 3.3a2

Comment 4

[Angelo Locritani]

I confirm this bug is still present in v.7.0 b3

Comment 5

[Walter Doekes]

Attachment: check whether myEmail is in automaticCCList and if so, don't remove it from CC

Ok. This bug is still present in hg tip. This patch should fix it.

I tried to obey coding style of the rest of the file. I don't know if any review flags should be set.

Regards,
Walter Doekes
OSSO B.V.

Comment 6

[Ludovic Hirlimann [:Usul]]

Comment on attachment 563329 [details] [diff] [review]
check whether myEmail is in automaticCCList and if so, don't remove it from CC

>I tried to obey coding style of the rest of the file. I don't know if any review 
>flags should be set.

Yes if you want the pacth to go into the tree.

Comment 7

[David :Bienvenu]

Comment on attachment 563329 [details] [diff] [review]
check whether myEmail is in automaticCCList and if so, don't remove it from CC

thx for the patch - it's a little out of date against the trunk.

PRBool needs to be bool, PR_TRUE should be true, PR_FALSE false

you don't need braces here:

+              if (ccListEmailAddresses.Find(myEmail) != kNotFound)
+              {
+                removeMyEmailInCc = PR_FALSE;
+              }

this line looks like it's over 80 chars, so should be split up:

+            // Only remove own address from CC if it's not in automatic-CC-to-self (see bug 584962)

+              rv = parser->ExtractHeaderAddressMailboxes(ccList,
+                                                         ccListEmailAddresses);
+              NS_ENSURE_SUCCESS(rv, rv);

This would totally abort if the cc list is not parseable, and I'm not sure we currently do that. So better to warn, but try to keep going.

Finally, why do you need the code inside the if (automaticCc) { }? Can't you just set removeMyMailInCc to !automaticCc? Or simply change if (removeMyEmailInCc) to if (!automaticCc) ?

Comment 8

[Walter Doekes]

> PRBool needs to be bool, PR_TRUE should be true, PR_FALSE false

done

> you don't need braces here:

done

> this line looks like it's over 80 chars, so should be split up:

done

> This would totally abort if the cc list is not parseable

I just copied that from nsMsgCompose::CreateMessage. I have not bothered to check what it does. But it looks like Reply'ing to a message would abort too, if the parsing were to fail. So I'm guessing the parser is not that picky.

> Finally, why do you need the code inside the if (automaticCc)

There are three cases here:
- user has no automaticCc
- user has automaticCc without self
- user has automaticCc with self

removeMyEmailInCc was always true before this patch, so it should still be. We only change it to false when there is automaticCc *and* self is in it.

I could reduce the use of two variables to a single one, but that would not make the code any clearer, IMO. That would become:

if (doNotRemoveMyEmailInCc) {
  ...
  if (ccListEmailAddresses.Find(myEmail) == kNotFound)
    doNotRemoveMyEmailInCc = false;
}
if (!doNotRemoveMyEmailInCC) {
  addressToBeRemove.Append...
}

You see how ridiculous that looks.

Comment 9

[Walter Doekes]

> > This would totally abort if the cc list is not parseable

> I just copied that from nsMsgCompose::CreateMessage. I have not bothered
> to check what it does. But it looks like Reply'ing to a message would
> abort too, if the parsing were to fail. So I'm guessing the parser is not
> that picky.

Oh wait. I'm wrong. I'll fix go that.

Comment 10

[David :Bienvenu]

(In reply to Walter Doekes from comment #8)
> 
> There are three cases here:
> - user has no automaticCc
> - user has automaticCc without self
> - user has automaticCc with self
>
Ah, thx, I see, a comment to that effect would be helpful...

Comment 11

[Walter Doekes]

Attachment: changes after feedback from david bienvenu

(Didn't test, got compile error in nsLDAPOperation.cpp.. but I don't think I made any typo's.)

Comment 12

[David :Bienvenu]

Comment on attachment 567522 [details] [diff] [review]
changes after feedback from david bienvenu

Thx, Walter, that looks good.

Comment 13

[Ludovic Hirlimann [:Usul]]

(In reply to David :Bienvenu from comment #12)
> Comment on attachment 567522 [details] [diff] [review] [diff] [details] [review]
> changes after feedback from david bienvenu
> 
> Thx, Walter, that looks good.

Adding checking-needed.

Comment 14

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/b4ae149b9eb5

Comment 15

[Walter Doekes]

Did someone break this in Thunderbird 19?

http://forums.mozillazine.org/viewtopic.php?f=29&t=2648633
"""The "Cc" behavior was changed as part of bug 250187, which indeed became effective with the 19.0 cycle."""

https://bugzilla.mozilla.org/show_bug.cgi?id=250187

I'm not even sure I like that feature.

Cheers,
Walter

Comment 16

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Yes, bug 917231.