Unwanted space added at top of outgoing emails

Bug #642529 reported by chekhovs1 on 2010-09-19
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Mozilla Thunderbird
Fix Released
Medium
thunderbird (Ubuntu)
Medium
Rolf Leggewie

Bug Description

Binary package hint: thunderbird

All text-based emails that I send out contain a space at the very beginning of email body that I did not put in there. HTML emails do not suffer from the problem.

Example: type the following email in message editor (can use any recipient address) and place it into Outbox (Ctrl+Shift+Enter),

Hello,
World

Expected behavior: the message in Outbox should look like this:

Hello,
World

Instead, the message has an extra space in front of "Hello":

 Hello,
World

If I now edit this message in Outbox as new (Ctrl+E) and delete the unwanted space and then place the message into Outbox again (Ctrl+Shift+Enter), the extra space is gone.

Version info:

$lsb_release -rd
Description: Ubuntu 9.04
Release: 9.04

$ apt-cache policy thunderbird
thunderbird:
  Installed: 3.1.5~hg20100915r5798+nobinonly-0ubuntu2~umd2~jaunty
  Candidate: 3.1.5~hg20100915r5798+nobinonly-0ubuntu2~umd2~jaunty
  Version table:
 *** 3.1.5~hg20100915r5798+nobinonly-0ubuntu2~umd2~jaunty 0
        500 http://ppa.launchpad.net jaunty/main Packages
        100 /var/lib/dpkg/status
     2.0.0.21+nobinonly-0ubuntu1.9.04.1 0
        500 http://archive.ubuntu.com jaunty/main Packages

ProblemType: Bug
Architecture: amd64
DistroRelease: Ubuntu 9.04
NonfreeKernelModules: fglrx
Package: thunderbird 3.1.5~hg20100915r5798+nobinonly-0ubuntu2~umd2~jaunty
ProcEnviron:
 LANGUAGE=
 PATH=(custom, user)
 LANG=en_US.UTF-8
 SHELL=/bin/bash
SourcePackage: thunderbird
Uname: Linux 2.6.28-19-generic x86_64
UnreportableReason: This is not a genuine Ubuntu package

This only seems to happen when composing in HTML mode without any formatting applied, then it is down-converted to plain text for sending. This includes a blank space as the first character even though it wasn't typed.

Adjusted steps to reproduce:
1. Compose a message in HTML mode, do not apply any formatting.
2. Make sure Options > Format is set to Auto-Detect.
3. Send message, it will be down-converted to plain text.
4. Plain text includes an inserted space in the first line.

Thus, either an Editor (as filed) or a DOM/Serializer issue.

Confirmed on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a5pre) Gecko/20100504 SeaMonkey/2.1a1pre; first observed in incoming messages with UA-string Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2.2pre) Gecko/20100206 Lanikai/3.1b1pre.

I'm tentatively confirming this as I can't find any duplicate either.

In , Dave (fehe) wrote :

The following is the best I can do for a regression range (Apr 19 to May 8), because there are no usable Windows builds from April 20 to May 8, inclusive:

Works with: http://hg.mozilla.org/mozilla-central/rev/11f68e169ed4
Fails with: http://hg.mozilla.org/mozilla-central/rev/baf06abf0af4

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=11f68e169ed4&tochange=baf06abf0af4

Someone with Linux or Mac could probably narrow it down further.

In , Dave (fehe) wrote :

Those are 2009 dates.

In , Dave (fehe) wrote :

I wonder if this could be a regression from one of the following?: bug 423233, bug 488799, and bug 490747

Someone with Linux or Mac needs to confirm.

Per comment #1, Mac is also affected. I've got the earliest mail showing the issue with a December 2009 Lanikai build, didn't get any earlier ones though
as apparently people were still using the TB 3.0pre nightly builds back then.

This would also reconfirm a Core problem (3.1 is affected whereas 3.0 isn't).

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

Gary can you hunt that one ?

Maybe the revision history of the plain-text serializer helps to narrow down - http://hg.mozilla.org/mozilla-central/log/067e44b89712/content/base/src/nsPlainTextSerializer.cpp

(In reply to comment #2)
> The following is the best I can do for a regression range (Apr 19 to May 8),
...except that there was no check-in for it during that time frame. :-\

These links might help:

http://hg.mozilla.org/mozilla-central/rev/f6d27eb05ec1

https://bugzilla.mozilla.org/show_bug.cgi?id=422403
https://bugzilla.mozilla.org/show_bug.cgi?id=524975
https://bugzilla.mozilla.org/show_bug.cgi?id=549295

when we are serializing the text node in
nsXHTMLContentSerializer::AppendText(), mDoFormat is true, so we call
AppendToStringFormatedWrapped() which inserts the leading whitespace.

Laurent, any suggestions?

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

@seth : no suggestion for the moment, I don't see what is wrong. In fact, I would like to have a test case which call the document encoder with an example of a document, because I don't know if I really understood the issue. However, if the origin of the issue is mDoFormat, the question could be: why mDoFormat is true? Are nsIDocumentEncoder flags set correctly?

>it will be down-converted to plain text.

What does it mean? You say that the HTML serializer is called a first time, and then generated html elements are removed from the resulting string? Why do not call directly the plain text serializer? Where is the code which do this convertion in comm-central ?

Created attachment 447194
Minimal test case

I can only provide the result after sending, but this message (forced to have both plain-text and HTML parts for comparison) contains two lines/paragraphs without any formatting applied and composed in HTML. As can be seen, the first line is indented in the plain-text part whereas it is not in the HTML version (displayed as typed).

Ok, it appears to be caused by the indentation of the first line after <body>. Modified HTML code of the test case:

<html>
<head>

<meta http-equiv="content-type" content="text/html; charset=ISO-8859-1">
</head>
<body bgcolor="#FFFFFF" text="#000000">
First line<br>
Second line<br>
</body>
</html>

results in no indentation of the first line. If I add back some space before "First" and "Second" without other changes, the "First" will be indented.

I've tested that scenario by sending an HTML-only message like this, then using View > Message Body As > Plain Text to force serialization to plain text.

E-mails sent with current stable Thunderbird 3.0.5 does not exhibit the leading spaces, the generated HTML code looks like the one pasted above, thus the result looks ok on either branch. Thus, somewhere the HTML generation code in either serializer or editor added the indentation levels for the HTML, thus exposing the issue on the serializer side.

Thanks for your test rsx11m, I will do some additional unit tests only with the serializer and the flag nsIDocumentEncoder::OutputFormatted, so I could verify if the issue is really inside the serializer. (If I'm not wrong, the initial call is here http://mxr.mozilla.org/comm-1.9.2/source/mailnews/compose/src/nsMsgCompose.cpp#1072 )

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

I investigated and here my thought.

First, the HTML serializer is ok for me. Existing unit tests show that serialization is ok with the flag nsIDocumentEncoder::OutputFormatted. Changes I made are improvements on the indentation, this is why we have additionnal spaces in the html in the attachement 447194.

Second, it seems that the nsMsgCompose::sendMsg() method (if this is really this method which is called when we send a message) doesn't call the html serializer, but the plain text serializer, since it calls the document encoder with the mime type "text/plain" (well, in fact it calls nsEditor::OutputToString which then calls the encoder). So my work on the HTML serializer is not (at least directly) responsible of the bug.

So I think that the bug is inside the plain text serializer. Then, why this bug appears after my work on the HTML serializer? I guess that the HTML serializer is called some times (when and where, I don't know, maybe by the editor), and the result is probably re-parsed, so we certainly have a new DOM with these additionnals white spaces (for indendation). But the plain text serializer probably doesn't support very well these additionnal spaces and add some spaces in the result when nsMsgCompose::sendMsg() call it.

I'm not familiar at all with the mail code, so perhaps I'm saying wrong things. Don't hesitate to correct my hypothesis if they are wrong.

I could see inside the plain text serializer if there is really a bug, but not before the next week.

Laurent, thanks for looking into this. I'm not really sure where in the MailNews code the conversion is actually performed, nsMsgCompose::sendMsg() seems to cover only one of several cases. However, given that looking at an HTML-only message in Plain Text mode (comment #14) exhibits the problem as well, any issue of accidental double-serializing would have to occur on both ends. It appears to be a combination of the HTML serializer now indents the code nicely with the plain-text serializer having problems with such indentation and appears to treat it as an &nbsp; where it isn't. Thus, the second issue has been there all along and was just exposed by the new indentation behavior on the HTML side.

<email address hidden> and Laurent, thanks for looking into this.

to expand on comment #10, here's the (partial) stack to where the leading space is inserted:

...
nsXHTMLContentSerializer::AppendToStringFormatedWrapped()
nsXHTMLContentSerializer::AppendText()
nsDocumentEncoder::SerializeNodeStart()
nsDocumentEncoder::SerializeToStringRecursive() nsDocumentEncoder::SerializeToStringRecursive()
nsDocumentEncoder::SerializeToStringRecursive()
nsDocumentEncoder::SerializeToStringRecursive()
nsDocumentEncoder::EncodeToString()
nsMsgComposeAndSend::GetBodyFromEditor()
nsMsgComposeAndSend::Init()
nsMsgComposeAndSend::CreateAndSendMessage()
...

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

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

Is there any reason why this bug lives in editor?

It was filed that way, feel free to move it to Serializers instead, which appears to be the more proper component given the discussion here...

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

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

Anybody having an idea where to go from here? This is obviously gaining visibility based on the duplicate count...

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

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

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

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

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

Just confirming that I've also seen this for the first time, beginning with 3.1. It is 100% repeatable.

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

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

Same here. Since 3.1 (Win7 32Bit) I have this annoying space character at the beginning of every written mail.

Just updated to Thunderbird 3.1.1 and the bug is still there. It seems that very basis functionality is getting neglected, which is not good.

Created attachment 458852
potential fix

I've analyzed this problem and I am attaching a proposed fix. It works for me and seems to be correct, but this is the first time I've ever attempted to change anything in the innards of Mozilla / Thunderbird code, so I don't claim to be an expert and assume that someone more experienced than I will have to vet the change.

Here's the story...

Now that the HTML in the body is being pretty-printed, there are two new tokens immediately after the <body> tag being fed to nsPlainTextSerializer::DoAddLeaf -- the newline following the <body> tag, and then the whitespace indenting the first line of the body. Either of these was enough to cause a space to be inserted at the beginning of the converted plaintext message. The newline would cause a space to be inserted whenever <pre> was not in use (wrong -- shouldn't happen at the start of the body), and the indentation whitespace would cause a space to be inserted whenever not in whitespace (wrong -- shouldn't happen at the start of the body) or when OutputSelectionOnly was set (OK, but not if the selection includes the start of the body).

I don't know if I'm doing a great job of explaining this. If not, forget about my explanation, look at the eHTMLTag_whitespace and eHTMLTag_newline branches of nsPlainTextSerializer::DoAddLeaf before my patch, and observe what will happen there when whitespace or newline is passed into this function, which they will be at the start of the body.

My patch addresses all of these issues by adding another boolean flag, mSawBodyTag, which if true means that we're cutting and pasting from the start of the body and therefore initial whitespace needs to be ignored even if OutputSelectionOnly is true, and by treating newline more similarly to how other whitespace is treated.

I've got to say, working in this code is quite challenging. ;-)

Comment on attachment 458852
potential fix

Let's pick Ben as a reviewer for this patch.

Jonathan, I have to say, that picking this particular problem as the first place to fix a mailnews bug is quite impressive!

(as an aside, your name rings a bell, likely from my peripheral involvement in netnews at the tail end of the last century).

(I don't know how doable it is, but figuring out an automated test to accompany the patch would be awesome).

I fixed it because it was annoying me. ;-)

The logic of a test case should be easy, but I hope someone more familiar than I with the test infrastructure can figure out how to set one up.

Great to see a patch here, thanks!

> (comment #38) Let's pick Ben as a reviewer for this patch.

But, is Ben a "licensed" Serializer reviewer?
This is Core code, not MailNews Core...

120 comments hidden view all 200 comments

As I have already said, as far as I can tell, my patch does not break copy/paste. As I have already said, I believe that it is the tests, and not my patch, that were incorrect. As I have already said, I was asked to change my patch to make it remove whitespace at the beginning of all block elements, which is what the HTML spec appears to require, and that is what I did, and that is whi they mochi tests started failing. As I have already said, someone authoritative needs to offer an opinion about whether the behavior I was asked to implement and indeed implemented in my patch is correct. (Ben already pointed out much of this in comment 160, but I am reiterating to make sure is it is perfectly clear).

Here's something to try. Save this HTML, newlines and all, in a file and then display it in Firefox:

---cut here---
<html>
<body>
foo
<div>

Is there whitespace before this line?
</div>
</body>
</html>
---cut here---

You will see that there is no extra whitespace before the question, which means that the browser is stripping whitespace after the <div> tag, which means that as far as I can tell, my change to make the plaintext serializer do the same thing is correct, and the tests that expect whitespace to be preserved after <div> (which David Bienvenu changed) are wrong.

Who needs to decide this? What do we need to do to make that decision happen?

Comment on attachment 470264
Inferior fix

My apologies Jonathan. I rebuilt from scratch mozilla-central and comm-1.9.2 binaries with your patch and try many copy/paste in different context. It seems there isn't any regression on the copy/paste behaviors. I don't know why I saw regressions in my previous builds (and I don't understand why original unit tests are bad) :-(.

I remove my patch. Let's continue with your patch.

Can we turn off html pretty printing in mailnews compose? If so, is there any reason we wouldn't want to do that?

Someone just asked on getsatisfaction.com how (in essence) to revert from Thunderbird 3.1.x to to 3.0.x.

I advised them (in essence) to use the 3.0.x installer via http://www.mozillamessaging.com/en-US/thunderbird/all-older.html

I noticed though on that page it says "Download Thunderbird 3

Thunderbird 3.0.x will be maintained with security and stability updates for a short period of time. All users are strongly encouraged to upgrade to Thunderbird 3.1"

Can it be made for this bug to be a blocker on the dropping of support for Thunderbird 3.0.x ?

(In reply to comment #147)
> Pinging jst and Jonathan for comments on my patch for fixing the mochi-tests. I
> basically had to switch the tests arounds, in the sense that the test cases
> that used to expect whitespace no longer got it, and the tests that didn't used
> to expect whitespace now got it, which was a bit odd.

Sorry for the delay in responding. I think your changes are correct.

Comment on attachment 470227
yet another try at the patch

> (comment #162) I remove my patch. Let's continue with your patch.

Laurent, what is the review status of Jonathan's patch? If it has been
reinstated after you retracted your own patch, you should either reconsider
your r-, or remove it and restore the original r? request to jst.

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

Comment on attachment 470227
yet another try at the patch

No, I don't reconsider my review on the attachement 470227 because the patch doesn't follow guide lines for patches, and because it doesn't fix unit tests. The patch is in fact obsolete since David have been updated it with a good formating and unit tests fix. (it doesn't remove Jonathan's credits of course :))

Comment on attachment 470567
fixes mochitests

Let's say it's ok, although I would prefer more tests (with different flags) to verify there isn't any regressions

Thanks for the clarification, I was confused by the forth and back. So, unless the sr+ on attachment 460730 (comment #98) can be carried forward to
attachment 470567, we are waiting for jst's superreview again.

chekhovs1 (chekovs) wrote :

Binary package hint: thunderbird

All text-based emails that I send out contain a space at the very beginning of email body that I did not put in there. HTML emails do not suffer from the problem.

Example: type the following email in message editor (can use any recipient address) and place it into Outbox (Ctrl+Shift+Enter),

Hello,
World

Expected behavior: the message in Outbox should look like this:

Hello,
World

Instead, the message has an extra space in front of "Hello":

 Hello,
World

If I now edit this message in Outbox as new (Ctrl+E) and delete the unwanted space and then place the message into Outbox again (Ctrl+Shift+Enter), the extra space is gone.

Version info:

$lsb_release -rd
Description: Ubuntu 9.04
Release: 9.04

$ apt-cache policy thunderbird
thunderbird:
  Installed: 3.1.5~hg20100915r5798+nobinonly-0ubuntu2~umd2~jaunty
  Candidate: 3.1.5~hg20100915r5798+nobinonly-0ubuntu2~umd2~jaunty
  Version table:
 *** 3.1.5~hg20100915r5798+nobinonly-0ubuntu2~umd2~jaunty 0
        500 http://ppa.launchpad.net jaunty/main Packages
        100 /var/lib/dpkg/status
     2.0.0.21+nobinonly-0ubuntu1.9.04.1 0
        500 http://archive.ubuntu.com jaunty/main Packages

ProblemType: Bug
Architecture: amd64
DistroRelease: Ubuntu 9.04
NonfreeKernelModules: fglrx
Package: thunderbird 3.1.5~hg20100915r5798+nobinonly-0ubuntu2~umd2~jaunty
ProcEnviron:
 LANGUAGE=
 PATH=(custom, user)
 LANG=en_US.UTF-8
 SHELL=/bin/bash
SourcePackage: thunderbird
Uname: Linux 2.6.28-19-generic x86_64
UnreportableReason: This is not a genuine Ubuntu package

chekhovs1 (chekovs) wrote :
Micah Gersten (micahg) wrote :

Thank you for reporting this to Ubuntu. I have also noticed this behavior since upgrading to Thunderbird 3.1.x, I will see if I can find a similar bug upstream.

Changed in thunderbird (Ubuntu):
status: New → Confirmed
Micah Gersten (micahg) wrote :

The upstream watch has been added to this bug. Soon Launchpad will import the comments from the upstream bug and you will be able to follow the upstream discussion. Please report any other issues you may find.

Changed in thunderbird (Ubuntu):
importance: Undecided → Medium
status: Confirmed → Triaged
Changed in thunderbird:
importance: Unknown → Medium
status: Unknown → Confirmed

Emailed jst directly and asked for his help getting this resolved.

Woohoo! Reviewed and superviewed. How do we get the change pushed into the right branch for baking, which I think is the next step? Somebody was kind enough to assign this ticket to me, but I don't know how to the commit and I'm fairly certain that I don't have the necessary access rights. Who here can tell us what the next step is? Does somebody with the necessary rights want to take ownership of the ticket and shepherd it through the rest of the process?

Thanks for your persistence, Jonathan; much appreciated! To drivers, who need to handle the approved flag: this fixes a very visible bug in Thunderbird. I don't know what the cost/benefit is for Firefox, but as jst was the superreviewer here, I suspect he's the best person to make that call.

In , Jst (jst) wrote :

Comment on attachment 470567
fixes mochitests

a=jst for trunk. Let's get this in ASAP to get as much testing as we can...

thx very much, Laurent. And of course, thx very much to Jonathan!

(In reply to comment #78)
> Created attachment 461705 [details]
> fix mailnews unit test

Just got this error locally: TEST-UNEXPECTED-FAIL | mozilla/_tests/xpcshell/mailnews/base/test/unit/test_getMsgTextFromStream.js | The tags should be stripped in this case! No, seriously. == The tags should be stripped in this case! No, seriously.

So this fix is still needed.

I don't know if I have enough access rights on the comm-central repository, to land this patch in mailnews. Anyway, I don't know rules to check in this tree. Should we have a specific approval flag or keyword or... ?

Comment on attachment 461705
fix mailnews unit test - cecked in.

Is this the missing piece on the MailNews end which yet has to be reviewed?

yeah - I can land it. I'll just make sure it's ok with the tree closed and all.

That helped, tests pass now on Windows and Mac OSX.

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

Created attachment 478816
1.9.2 de bitrotted

The 1.9.2 version of this patch suffered a bit of bitrot (due to other changes not being on the branch) in the test cases only. Therefore I've fixed them up and will check this in in a bit.

Verified fixed using:
comm-1.9.2 cset: 136f1217a4aacomm-central cset: 320b3705916f

Thanks to all for getting this resolved.

Hmm. A new bug? Line 2 of comment 186 was two separate lines, when I submitted.

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

Changed in thunderbird:
status: Confirmed → Fix Released

Hi all - really glad to have found this thread. I'm a new thunderbird user and have been loving it, but have no idea how to actually use this patch to fix the extra space error. I know this is probably incredibly obvious, and apologize, but how do I actually implement this patch?

Thanks!

(In reply to comment #189)
> Hi all - really glad to have found this thread. I'm a new thunderbird user and
> have been loving it, but have no idea how to actually use this patch to fix the
> extra space error. I know this is probably incredibly obvious, and apologize,
> but how do I actually implement this patch?

If you don't know how, the best way is to wait a little under two weeks and you'll get a patched version via the automatic update system.

Fantastic, thanks! And thanks to all that worked on this bug to get it fixed.

Micah Gersten (micahg) on 2010-10-10
Changed in thunderbird:
milestone: none → 3.1.5

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

Could bug 509008 be a dupe of this bug? Bug 509008 was actually active in the oldest binaries of Firefox I could test in August 2009. (Probably Firefox 1.x or 2.x.)

I see Thunderbird 3.1.5 was released on Tuesday (19 Oct 2010).

Does 3.1.5 contain a fix to this problem?

(In reply to comment #194)
> Does 3.1.5 contain a fix to this problem?

Yes

Rolf Leggewie (r0lf) wrote :

This issue was marked as fixed upstream for version 3.1.5 which has long since hit Ubuntu. Are you still experiencing this problem?

Changed in thunderbird (Ubuntu):
assignee: nobody → Rolf Leggewie (r0lf)
status: Triaged → Incomplete
Rolf Leggewie (r0lf) on 2014-03-26
Changed in thunderbird (Ubuntu):
status: Incomplete → Fix Released
Displaying first 40 and last 40 comments. View all 200 comments or add a comment.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.