Last Comment Bug 737014 - Plaintext signatures are not received properly due to lack of BR nodes.
: Plaintext signatures are not received properly due to lack of BR nodes.
Status: VERIFIED FIXED
: regression
Product: Thunderbird
Classification: Client Software
Component: Message Compose Window (show other bugs)
: 13 Branch
: All All
: -- normal (vote)
: Thunderbird 14.0
Assigned To: Mike Conley (:mconley)
:
:
Mentors:
: 737736 (view as bug list)
Depends on:
Blocks: BigFiles
  Show dependency treegraph
 
Reported: 2012-03-19 07:26 PDT by ohghoo8n
Modified: 2012-03-31 14:14 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
verified


Attachments
A sample (redacted) showing a received message where the newlines have been stripped from my signature, and stay on line after the "--". (8.17 KB, image/png)
2012-03-19 07:26 PDT, ohghoo8n
no flags Details
A screenshot showing the newlines are properly inserted in the composition window. (100.63 KB, image/png)
2012-03-19 07:33 PDT, ohghoo8n
no flags Details
tweak mconley's patch to get it to compile (2.40 KB, patch)
2012-03-19 12:19 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
Patch v1 (7.48 KB, patch)
2012-03-19 14:06 PDT, Mike Conley (:mconley)
mozilla: review-
Details | Diff | Splinter Review
parse string inline (5.67 KB, patch)
2012-03-19 21:26 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
Patch v2 (5.30 KB, patch)
2012-03-20 07:10 PDT, Mike Conley (:mconley)
mozilla: review+
mozilla: approval‑comm‑aurora+
Details | Diff | Splinter Review
fix typo in kSeperator (1.80 KB, patch)
2012-03-21 12:25 PDT, :aceman
mconley: review+
Details | Diff | Splinter Review
Use ints instead of iterators for external API compatibility (1.33 KB, patch)
2012-03-26 09:42 PDT, neil@parkwaycc.co.uk
mozilla: review+
Details | Diff | Splinter Review

Description ohghoo8n 2012-03-19 07:26:01 PDT
Created attachment 607153 [details]
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: me@company.tld
Office: +123 12345678
Cell: +123 12345678

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:me@company.tld
title:My Work Position
tel;work:+123 12345678
tel;cell:+123 12345678
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: me@company.tld
Office: +123 12345678
Cell: +123 12345678

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: me@company.tld Office: +123 12345678 Cell: +123 12345678

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 2012-03-19 07:33:55 PDT
Created attachment 607157 [details]
A screenshot showing the newlines are properly inserted in the composition window.
Comment 2 rsx11m 2012-03-19 08:20:59 PDT
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 2012-03-19 12:19:23 PDT
Created attachment 607252 [details] [diff] [review]
tweak mconley's patch to get it to compile
Comment 4 Mike Conley (:mconley) 2012-03-19 14:06:00 PDT
Created attachment 607309 [details] [diff] [review]
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 2012-03-19 14:26:22 PDT
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 2012-03-19 21:25:58 PDT
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 2012-03-19 21:26:25 PDT
Created attachment 607443 [details] [diff] [review]
parse string inline
Comment 8 David :Bienvenu 2012-03-19 21:27:17 PDT
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) 2012-03-20 07:10:09 PDT
Created attachment 607532 [details] [diff] [review]
Patch v2

Thanks for the assist on this, David - I've removed SplitString in nsMsgCompose.h/.cpp as suggested.
Comment 10 David :Bienvenu 2012-03-20 08:46:52 PDT
Comment on attachment 607532 [details] [diff] [review]
Patch v2

great, thx.
Comment 11 Mike Conley (:mconley) 2012-03-20 08:48:27 PDT
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) 2012-03-20 09:19:36 PDT
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] 2012-03-21 06:00:27 PDT
Seems fine now with:
Mozilla/5.0 (Windows NT 5.1; rv:14.0) Gecko/20120321 Thunderbird/14.0a1
Comment 14 Mike Conley (:mconley) 2012-03-21 06:03:46 PDT
Joe:

Awesome, thanks!

-Mike
Comment 15 rsx11m 2012-03-21 06:37:45 PDT
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 2012-03-21 06:53:36 PDT
Works for me after applying update. Thanks guys.
Comment 17 ohghoo8n 2012-03-21 06:54:42 PDT
(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 2012-03-21 06:56:04 PDT
Easy to "re-fix" but I'm wondering why this happened twice in a row... :-\
Comment 19 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-21 07:53:27 PDT
*** Bug 737736 has been marked as a duplicate of this bug. ***
Comment 20 :aceman 2012-03-21 09:41:53 PDT
+  const kSeperator = "-- ";

This probably wanted to be kSeparator?
Comment 21 Mike Conley (:mconley) 2012-03-21 09:49:12 PDT
(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 2012-03-21 09:54:03 PDT
Surely. Can I attach it here?
Comment 23 Mike Conley (:mconley) 2012-03-21 09:54:49 PDT
Sure - and then I'll commit it as a followup.
Comment 24 :aceman 2012-03-21 12:25:26 PDT
Created attachment 608054 [details] [diff] [review]
fix typo in kSeperator
Comment 25 Mike Conley (:mconley) 2012-03-21 12:50:33 PDT
Comment on attachment 608054 [details] [diff] [review]
fix typo in kSeperator

Great, thanks!
Comment 26 Mike Conley (:mconley) 2012-03-21 14:00:15 PDT
aceman:

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

-Mike
Comment 27 neil@parkwaycc.co.uk 2012-03-26 09:42:09 PDT
Created attachment 609355 [details] [diff] [review]
Use ints instead of iterators for external API compatibility
Comment 28 neil@parkwaycc.co.uk 2012-03-31 14:14:20 PDT
Comment on attachment 609355 [details] [diff] [review]
Use ints instead of iterators for external API compatibility

Pushed changeset 6208e3c32bad to comm-central.

Note You need to log in before you can comment on or make changes to this bug.