365713 - BMO's review emails now appear to be double spaced

Status: RESOLVED DUPLICATE of bug 486206

(Bugzilla :: Email Notifications, defect)

Description

[Nelson Bolyard (seldom reads bugmail)]

When a review of a patch is requested, or granted, or denied, bugzilla 
sends out an email to the reviewer (or to the person who requested the 
review) whose subject starts with "review requested:" or "review granted".

bugzilla also sends a similar email to people on the CC list, but that
email does not have the "review requested" or "review granted" prefix on
the subject line.

The email with the "review requested:" or "review granted:" subject prefix
has the body double spaced.  That is, as displayed, there is a blank line 
between every pair of non-blank lines in that email message body. 

The email sent to the CC list does not have this double spacing.

This behavior began with the December 26 upgrade.

Examining the body of one of these emails shows that the message is 
sent in quoted-printable encoding and every line ends with =0D .
It's those =0D line endings that cause the double spacing.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: example of a double spaced email

Comment 2

[Reed Loden [:reed]]

Is this still an issue, Nelson?

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

No, I last observed the problem in an email dated 02-02.
The next such review email I received, dated 02-08, did not exhibit the 
problem and I have not seen it since.
So I guess this was fixed.  Anybody know what fixed it?

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: another double-spaced email

This problem is back with a vengeance.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Two double-spaced review emails

Two more double-spaced emails received just minutes ago.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

This is continuing to occur.  More such emails received just today.

Comment 7

[Dave Miller [:justdave]]

I can confirm this with my own review mail.  The top portion of the message has unix line endings, the comment section has windows line endings.  Thunderbird ignores it, but obviously whatever email client Nelson is using doesn't.

The also only seems to affect messages that are quoted-printable encoded.  Text/plain messages have the proper line endings all the way through.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

I'm using a mozilla mail client.
Unix line endings in emails are always wrong.  
RFC 822 demands that all lines end in CR LF, that is \r\n.

But this bug is not about those lines.  
It is about the lines with the quoted-printable line endings 
as seen in the attached message.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Attachment: single-spaced message - to contrast with the "double spaced" ones previously attached

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Attachment: A particularly egregious example

Still happening

Comment 11

[timeless]

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/webtools/bugzilla/Bugzilla/Mailer.pm&rev=1.10&mark=60#52

sounds like someone will get to look at the mailer impl...

Comment 12

[Max Kanat-Alexander]

Hey rjbs--is encoding_set doing something wrong here, or are we doing something wrong with encoding_set?

Comment 13

[Ricardo SIGNES]

There are a few things worth saying, here:

First, you guys should consider using Mail::Box.  It's not quite as simple to use as Email::MIME, but it is *far* more thorough about making sure that your messages are correctly constructed.  Email::MIME is much better for quickly slapping together something simple.

Secondly, this line is weird:

  $part->encoding_set('quoted-printable') if !is_7bit_clean($part->body);

I'm not sure where $msg comes from, or what was in it, but ->body returns the the *decoded* body, so it might be so 7-bit-unclean as to suggest base64 encoding rather than QP.  Of course, I don't have enough context (and haven't dug much for more).  If this message was created with something sane, elsewhere, you should not have to set the encoding.

Finally, you should try these tests with the *very* latest Email::Simple and Email::MIME::* modules.  Be sure you have the *very* latest ones, not the latest ones to hit your local mirror.  That is:

 Email::Simple 2.002
 Email::MIME   1.860
 EM::Creator   1.454
 EM::Modifier  1.442

These versions have corrected quite a few linebreak bugs.  They're also quite fresh out of the gate, and there have been a few little quirks, so please test them LOCALLY before deploying to The Big Install. ;)  I think they're safe, at this point, though.

Comment 14

[Max Kanat-Alexander]

Hey Ricardo. Thanks for your comment. :-)

I can look into Mail::Box. I really like the API of the Email:: modules, though, which is why I'm using them. Also, I wanted to get away from the fact that MailTools doesn't throw understandable error messages when it fails. (Maybe Mail::Box is better about that?)

$msg comes from the templates. Which means that parts of it come from the user. For example, it could contain one of these bug comments.

Perhaps we should be using body_raw? I seem to recall some reason I didn't do that, but I forget now.

Comment 15

[Ricardo SIGNES]

The interface provided by Mail::Message (part of the Mail-Box distribution) is not all that different from Email::Simple, although it provides a *larger* interface.  I should hide you from worrying at all about manually figuring out encodings.

It seems like you should be accepting parts of the message, putting them together into pieces, and then be using Email::MIME::Creator if that's what you want to do.  You're only going to cause yourself serious problems by screwing with encoding and MIME headers after the fact.

Comment 16

[Max Kanat-Alexander]

(In reply to comment #15)
> It seems like you should be accepting parts of the message, putting them
> together into pieces, and then be using Email::MIME::Creator if that's what you
> want to do.  You're only going to cause yourself serious problems by screwing
> with encoding and MIME headers after the fact.

  This particular message has only one part.

  There's only one message that has two parts (the Whine mail), and we haven't had any particular bugs reported with that one.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Attachment: More example

still going.  Call this the "Energizer bug".

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Attachment: This is getting ridiculous

I want bugzilla to stop sending me any more of these
ridiculous emails until this is fixed.  
There doesn't seem to be an option to get it to stop 
sending review notifications.

Comment 19

[Reed Loden [:reed]]

(In reply to comment #18)
> There doesn't seem to be an option to get it to stop 
> sending review notifications.

https://bugzilla.mozilla.org/userprefs.cgi?tab=email -- see the two options under "Global options:".

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Attachment: dummy patch for testing the problem

Please ignore this patch.  I am testing review emails.
This comment has an email address <[e-mail]>

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 275259 [details] [diff] [review]
dummy patch for testing the problem

This comment has a quoted line
>+            if (info) PORT_Free(info);
>+            if (info) PORT_Free(info);

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 275259 [details] [diff] [review]
dummy patch for testing the problem

More test
> quoted line without any special characters
> quoted line without any special characters
> quoted line without any special characters

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 275259 [details] [diff] [review]
dummy patch for testing the problem

Binary search for characters that trigger double-spacing:
> (parenthenses)
> (parenthenses)
> (parenthenses)

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 275259 [details] [diff] [review]
dummy patch for testing the problem

Binary search continues.  Wasn't parentheses.  
There were only 5 special characters in the last message 
that was double-spaced, and I've eliminated 3 (>).
Plus and semicolon remain.  Try plus (and period).
> +
> +
> +

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 275259 [details] [diff] [review]
dummy patch for testing the problem

Binary search continues.  Wasn't parentheses.  
There were only 5 special characters in the last message 
that was double-spaced, and I've eliminated 3 (>).
Plus and semicolon remain.  Try plus (and period).
> > +
> > +
> > +

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 275259 [details] [diff] [review]
dummy patch for testing the problem

OK, just semi-colon left, I think.  (Also testing comma)
> ;
> ;
> ;

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 275259 [details] [diff] [review]
dummy patch for testing the problem

Oops, I missed underscore.  
The last review request or denial message that was 
double spaced was the denial in comment 21.
All my attempts to reproduce the double spacing 
using a subset of the characters in that denial 
have failed to be double spaced.  But I see now
that I overlooked the underscore, so here goes.
>  bla_bla
>  bla_bla
>  bla_bla

Comment 28

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 275259 [details] [diff] [review]
dummy patch for testing the problem

Something about comment 21 triggered double spacing.
None of my attempts to trigger double-spacing using a 
subset of those characters has triggered double spacing.
I don't know what was special about comment 21.

Comment 29

[Max Kanat-Alexander]

3.0.1 is *in QA*. Of course this isn't a blocker now. :-) 3.1.1 is also in the release process.

Also, Nelson, please use landfill to test, not bmo.

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 275259 [details] [diff] [review]
dummy patch for testing the problem

Hmm. Maybe juxtaposition of characters,
Trying greater than followed by plus, 
with no intervening space
>+  bla bla bla
>+  bla bla bla
>+  bla bla bla
>+  bla bla bla

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

Attachment: all emails from the above tests

OK, I'm done testing.  
I'm attaching all the emails I received from the above tests.
Only one of them was double-spaced, due to improper quoted-printable
encoding.

Comment 32

[Max Kanat-Alexander]

Nelson, why change the TM? This is still a problem in 3.0, right?

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

> Why change the TM?  
Because comment 29 says the 3.0 train has left the station.

Bugs targetted at already-released versions don't get fixed
(at least, not in the already released version).
The Version field records the version of the product in which 
the problem occurs.  The fix will occur in some later (higher
numbered) release, if any, which is why I set the target to 
the next available higher numbered target. (Why is there no 3.1
target?) 

But if you think this bug will be fixed soonest by being targetted 
at an already released version, I will defer to your choice.

Comment 34

[Max Kanat-Alexander]

(In reply to comment #33)
> > Why change the TM?  
> Because comment 29 says the 3.0 train has left the station.

  No, I was just talking about 3.0.1. You can come into IRC if you want an explanation of our development process.

Comment 35

[Nelson Bolyard (seldom reads bugmail)]

Attachment: another particularly egregious example

Comment 36

[Max Kanat-Alexander]

This isn't a blocker until somebody proves that it happens on other installations and can actually reproduce it. I just don't want to block any release forever on a bug we can't reproduce predictably.

I also need to know if bmo has upgraded as rjbs suggested in comment 13.

Comment 37

[Nelson Bolyard (seldom reads bugmail)]

Attachment: yet another example

Comment 38

[Dave Miller [:justdave]]

(In reply to comment #36)
> I also need to know if bmo has upgraded as rjbs suggested in comment 13.

bmo currently has:

perl-Email-Simple-1.995-1.el4.rf
perl-Email-MIME-1.859-1.el4.rf
perl-Email-MIME-Creator-1.453-1.el4.rf
perl-Email-MIME-Modifier-1.441-2.el4.rf

Those are the latest versions available in rpmforge.

The following were installed just now via CPAN:

R/RJ/RJBS/Email-Simple-2.003.tar.gz
R/RJ/RJBS/Email-MIME-1.860.tar.gz
R/RJ/RJBS/Email-MIME-Creator-1.454.tar.gz
R/RJ/RJBS/Email-MIME-Modifier-1.442.tar.gz

Comment 39

[Dave Miller [:justdave]]

According to LpSolit, this upgrade seems to have made it worse.

Comment 40

[Dave Miller [:justdave]]

OK, we've had additional upgrades to the version of Email::* that we're running since the last time this bug was touched.  Is this still an issue?

perl-Email-Simple-2.003-1.el5.rf
perl-Email-MIME-1.861-1.el5.rf
perl-Email-MIME-Creator-1.454-2.el5.rf
perl-Email-MIME-Modifier-1.442-1.el5.rf

Comment 41

[Nelson Bolyard (seldom reads bugmail)]

I propose that we wait a few weeks.  If no more problematic messages arrive
by end of month, I think we can call it fixed.

Comment 42

[Nelson Bolyard (seldom reads bugmail)]

Nope, not fixed. 
I received another double-spaced review email today.
Same problem as before.  Each line of text ends with =0D .

Comment 43

[Nelson Bolyard (seldom reads bugmail)]

I received another double-spaced review request email today.

Comment 44

[Nelson Bolyard (seldom reads bugmail)]

Attachment: latest example

Comment 45

[Nelson Bolyard (seldom reads bugmail)]

I think we have a reproducible test case, at long last. 

Alexei Volkov has found that he can past some text (a stack trace) into 
the comment window when he requests a patch review, and this causes the 
review request email to have that "double-spaced" appearance (actually 
improper quoted printable encoding), every time.  See bug 429230.

Comment 46

[Nelson Bolyard (seldom reads bugmail)]

still occuring

Comment 47

[Frédéric Buclin]

The Bugzilla 3.0 branch is now locked to security bugs and dataloss fixes only. This bug doesn't fit into one of these two categories and is retargetted to 3.2 as part of a mass-change. To catch bugmails related to this mass-change, use lts081207 in your email client filter.

Comment 48

[Frédéric Buclin]

mkanat, is this bug fixed by bug 467920?

Comment 49

[Max Kanat-Alexander]

(In reply to comment #48)
> mkanat, is this bug fixed by bug 467920?

  Oh, very likely. Let's see if it still happens after the upgrade.

Comment 50

[Nelson Bolyard (seldom reads bugmail)]

Today, about half of the emails from bugzilla have this same appearance 
of double-spacing in them.  They include ordinary bug mails, from comments
given on the main bug page, and not only review comments (or comments given
on the "details" page.  

This is suddenly MUCH worse.  It was a minor/normal problem before, but 
now, I'd say it's "major".

Comment 51

[Max Kanat-Alexander]

Looking at both bug 486206 and the examples on this bug, I think that bug 486206 is just a broader case of this bug that's started happening everywhere now that we enforce standard line endings in emails. bug 486206 has a patch, so I'm going to dup forward to that bug.