Closed Bug 999269 Opened 10 years ago Closed 10 years ago

Forwarding a message without CC header inline gets an empty "CC:" line in header table

Categories

(MailNews Core :: Composition, defect)

defect
Not set
major

Tracking

(thunderbird29 wontfix, thunderbird30 fixed, thunderbird31 fixed, seamonkey2.25 unaffected, seamonkey2.26 fixed, seamonkey2.27 fixed, seamonkey2.28 fixed, thunderbird_esr24 unaffected)

RESOLVED FIXED
Thunderbird 31.0
Tracking Status
thunderbird29 --- wontfix
thunderbird30 --- fixed
thunderbird31 --- fixed
seamonkey2.25 --- unaffected
seamonkey2.26 --- fixed
seamonkey2.27 --- fixed
seamonkey2.28 --- fixed
thunderbird_esr24 --- unaffected

People

(Reporter: rsx11m.pub, Assigned: rsx11m.pub)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

Steps to reproduce:

 1. Take a message in your Inbox or folder (IMAP or local, doesn't matter)
 2. Verify that it doesn't have any "CC:" header set
 3. Forward inline (as plain text or HTML, doesn't matter either)

Expected result:

 4. No "CC:" line in the header table of the message to be composed

Actual result:

 4. An empty "CC:" line in the header table of the message to be composed

Observed with both today's TB 31.0a1 and SM 2.28a1 nightly builds, as well as a March SM 2.27a2 build on Windows 7.
Regression range (SM trunk on Windows):
 - last good build: 2.26a1 (Gecko 29.0a1) 20131211003521 0697d16f37df
 - first bad build: 2.26a1 (Gecko 29.0a1) 20131213003001 fb99ca2bed04

http://hg.mozilla.org/comm-central/graph/13377?revcount=15
Joshua, these are mostly patches from bug 842632 in the regression range.
Does this ring any bell?
Flags: needinfo?(Pidgeot18)
I'm not seeing the issue locally, which makes it hard for me to say what's going on.

Off-hand, from my recollection of relevant code, existence of the header is usually treated as a non-empty version of the field. I know my new jsmime code tends to be extremely intent on deleting whitespace wherever it can, although I also realized that I didn't introduce its use yet here (since the actual use-jsmime patches have yet to land). It's possible that the old code is not quite so whitespace-abhorent and you have something somewhere generating a " " for the Cc value instead of a null/empty string.
Flags: needinfo?(Pidgeot18)
Weird, I could reproduce this with two different profiles with no other STR than the ones specified.

Note that only the "CC" heading is affected. Forwarding a message inline which doesn't have a "To" header still results only in an empty "CC:" but not an empty "To:" line being added. Thus, it doesn't appear to be systemic to address headers but specific to the "CC" one.
Thanks to Bozz who pointed me to a message which has a Cc heading set, and forwarding that one resulted in no "CC:" line added to the forward table. Thus, the logic is reversed.

The problem lies in mimedrft.cpp line #890, which comm-central changeset fbac0bdddf0b changed
   8.358 -  if (to)
   8.359 +  if (!to.IsEmpty())
but then
   8.366 -  if (cc)
   8.367 +  if (cc.IsEmpty())
which is a reversed logic for the test in mime_insert_normal_headers().

Adding the '!' to the cc test corrects the logic.
Blocks: 842632
Severity: minor → major
OS: Windows 7 → All
Hardware: x86_64 → All
This fixes the occurrence for the normal-header case I've noticed this bug for. There are also several instances in mime_insert_micro_headers() where I don't know what micro headers are, but I've corrected them as well while I'm there.

Joshua, please double-check that this doesn't break anything else.
Attachment #8410364 - Flags: review?(neil)
Attachment #8410364 - Flags: feedback?(Pidgeot18)
Comment on attachment 8410364 [details] [diff] [review]
Re-reverse reversed logic

Review of attachment 8410364 [details] [diff] [review]:
-----------------------------------------------------------------

Derpy edit fail is derpy.
Attachment #8410364 - Flags: review?(neil)
Attachment #8410364 - Flags: review+
Attachment #8410364 - Flags: feedback?(Pidgeot18)
Thanks for the ultra-quick review, push for comm-central please.
Assignee: nobody → rsx11m.pub
Status: NEW → ASSIGNED
Keywords: checkin-needed
Comment on attachment 8410364 [details] [diff] [review]
Re-reverse reversed logic

[Approval Request Comment]
Regression caused by (bug #): bug 842632
User impact if declined: erroneous behavior for headers when forwarding inline
Testing completed (on c-c, etc.): I was planning to wait until this lands on c-c and passes all tests, but since the tree is currently "Approval required" and we should take this for the SM 2.26 release, I'll flag it this ahead of the push.
Risk to taking this patch (and alternatives if risky): low

IanN, Callek: You'll probably have to approve for comm-beta as Thunderbird's 29.0 beta was released already.
Attachment #8410364 - Flags: approval-comm-beta?
Attachment #8410364 - Flags: approval-comm-aurora?
Flags: needinfo?(iann_bugzilla)
Flags: needinfo?(bugspam.Callek)
Whiteboard: [c-n: comm-central]
(In reply to rsx11m from comment #9)
> Testing completed (on c-c, etc.): I was planning to wait until this lands on
> c-c and passes all tests,

Ok, I see that this thinking is fundamentally flawed - if there /were/ tests for the forward header table already, they should have caught this regression in the first place... ;-)

Filed bug 1000147 on expanding the mozmill test respectively.
Blocks: 1000147
https://hg.mozilla.org/comm-central/rev/94cf07d93040
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: comm-central]
Target Milestone: --- → Thunderbird 31.0
Comment on attachment 8410364 [details] [diff] [review]
Re-reverse reversed logic

Note: this is landing post TB 29b1 (there won't be another 29b1 landing).
Attachment #8410364 - Flags: approval-comm-beta?
Attachment #8410364 - Flags: approval-comm-beta+
Attachment #8410364 - Flags: approval-comm-aurora?
Attachment #8410364 - Flags: approval-comm-aurora+
Thanks Mark. Cancelling the SM-specific requests now that this has landed on the branches.
Flags: needinfo?(iann_bugzilla)
Flags: needinfo?(bugspam.Callek)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: