737014 - Plaintext signatures are not received properly due to lack of BR nodes.

Status: VERIFIED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[ohghoo8n]

Attachment: A sample (redacted) showing a received message where the newlines have been stripped from my signature, and stay on line after the "--".

Using Mozilla/5.0 (Windows NT 6.1; rv:14.0) Gecko/20120318 Thunderbird/14.0a1 ID:20120318030022 for a standard account on a Mirapoint IMA4rev1 server.  I have noticed the following issue where a newline is not inserted between the "--" and the signature, or that and every line of the signature does not have their respective newlines.

Signature format:
Firstname A Lastname
My Work Position
My Department
My Institution
Email: [e-mail]
Office: [phone]
Cell: [phone]

My vCard format (snipped from the problematic message source):

--------------020207090705040603090106
Content-Type: text/x-vcard; charset=utf-8;
 name="myworkemailhandle.vcf"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="myworkemailhandle.vcf"

begin:vcard
fn:Firstname A Lastname
n:Lastname;Firstname
org:My Institution;My Department
adr:My Corporate Campus;;P.O. Box 12345;City;;12345;Country
email;internet:[e-mail]
title:My Work Position
tel;work:[phone]
tel;cell:[phone]
url:http://www.linkedin.com/in/myuserprofile
version:2.1
end:vcard


--------------020207090705040603090106--

Steps to recreate the problem:

1) Click Ctrl+N to compose a new message.

2) Enter message recipient.

3) Enter message subject.

4) Type message body content before the --, where the cursor is placed by default. After the --, Daily has automatically injected my text-only signature (which has defined per-account in Tools > Accounts Settings > Account Name > Signature text; additionally, a vCard is specified and configured to automatically attach appropriately). The signature at this point *appears correctly*.

5) Click Ctrl+Enter to send.

Desired Result:

Message has appropriately formatted signature, looking like so.
--
Firstname A Lastname
My Work Position
My Department
My Institution
Email: [e-mail]
Office: [phone]
Cell: [phone]

Actual result:

The newlines are stripped or never added. (See attached screenshot, signature.newline.1.png)

-- Firstname A Lastname My Work Position My Department My Institution Email: [e-mail] Office: [phone] Cell: [phone]

Notes:

I had not reported this for a few days because it is actually difficult to recreate.  It is not happening consistently, and I am not particularly sure why.

Comment 1

[ohghoo8n]

Attachment: A screenshot showing the newlines are properly inserted in the composition window.

Comment 2

[rsx11m]

Confirming with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120319 Thunderbird/14.0a1 with a minimal example (1-line signature "test", no vCard).
Content-Type: text/plain; charset=ISO-8859-1; format=flowed.

Using plain-text editor. Last bugs affecting signature spacing were bug 698925 (BigFiles) with a regression resolved in bug 735380.

Comment 3

[David :Bienvenu]

Attachment: tweak mconley's patch to get it to compile

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

Sorry this took me so long - string conversion in C++ is not something I'm really used to.

Here's my first run at a solution.  I've sent a plaintext message to myself with a multi-line plaintext signature and it seems to work.

I had to update a test to make it pass properly.

I'm splitting the string based on the "\n" character...am I missing any corner cases?

Comment 5

[rsx11m]

Some code in nsMsgCompose::ProcessSignature() compares on all platform variants of line terminators, i.e., "\n", "\r", and "\r\n" (the first form applies to Linux but not necessary to Mac OSX and Windows, I don't know though if any automated conversion is applied by now, e.g., when reading the signature from a file).

There was a discussion I had with Neil back in bug 428040 that signatures are allowed to either have or have not a trailing newline at the end, where both cases are considered equal to produce the same result (hence the handling of CRLF cases in the resulting fix there, which to some extent were removed in bug 698925 or bug 735380; you may want to verify that the logic envisioned there still applies after the various changes in the wake of the BigFiles implementation).

Comment 6

[David :Bienvenu]

Comment on attachment 607309 [details] [diff] [review]
Patch v1

so, I think either SplitString should be in nsMsgUtils.cpp/h where other code could use it (and probably take a PRUnichar delimiter), or we should just do it inline, like the patch I'm about to attach.

Comment 7

[David :Bienvenu]

Attachment: parse string inline

Comment 8

[David :Bienvenu]

Comment on attachment 607443 [details] [diff] [review]
parse string inline

and remove SplitString, of course :-)  This is the approach I was trying to describe over IRC...

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2

Thanks for the assist on this, David - I've removed SplitString in nsMsgCompose.h/.cpp as suggested.

Comment 10

[David :Bienvenu]

Comment on attachment 607532 [details] [diff] [review]
Patch v2

great, thx.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 607532 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Regression caused by (bug #): bug 698925
User impact if declined: Plaintext signatures will not be received correctly.
Testing completed (on c-c, etc.): comm-central.
Risk to taking this patch (and alternatives if risky): None that I can think of - the patch is pretty straight-forward.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Committed to comm-central as http://hg.mozilla.org/comm-central/rev/342f2874aa70
Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/62d1ea4c543c

Comment 13

[Joe Sabash [:JoeS1]]

Seems fine now with:
Mozilla/5.0 (Windows NT 5.1; rv:14.0) Gecko/20120321 Thunderbird/14.0a1

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Joe:

Awesome, thanks!

-Mike

Comment 15

[rsx11m]

Looks like bugzilla screwed up the flags with comment #13, restoring them except for tracking-thunderbird13+ which I can't set (but hopefully no longer relevant).

Comment 16

[ohghoo8n]

Works for me after applying update. Thanks guys.

Comment 17

[ohghoo8n]

(In reply to rsx11m from comment #15)
> Looks like bugzilla screwed up the flags with comment #13, restoring them
> except for tracking-thunderbird13+ which I can't set (but hopefully no
> longer relevant).

Ugh. I am an ass and mashed buttons quickly.  I did not mean to reset flags specifically after you mentioned it.  Apologies and mea culpa.  Let me know if you want me to do anything.

Comment 18

[rsx11m]

Easy to "re-fix" but I'm wondering why this happened twice in a row... :-\

Comment 20

[:aceman]

+  const kSeperator = "-- ";

This probably wanted to be kSeparator?

Comment 21

[Mike Conley (:mconley) (:⚙️)]

(In reply to :aceman from comment #20)
> +  const kSeperator = "-- ";
> 
> This probably wanted to be kSeparator?

Good catch!  Feel like throwing together a patch?

Comment 22

[:aceman]

Surely. Can I attach it here?

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Sure - and then I'll commit it as a followup.

Comment 24

[:aceman]

Attachment: fix typo in kSeperator

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 608054 [details] [diff] [review]
fix typo in kSeperator

Great, thanks!

Comment 26

[Mike Conley (:mconley) (:⚙️)]

aceman:

Thanks, follow-up checked in to comm-central as http://hg.mozilla.org/comm-central/rev/8a8a82bf4a24

-Mike

Comment 27

[[e-mail]]

Attachment: Use ints instead of iterators for external API compatibility

Comment 28

[[e-mail]]

Comment on attachment 609355 [details] [diff] [review]
Use ints instead of iterators for external API compatibility

Pushed changeset 6208e3c32bad to comm-central.