853437 - email address with leading whitespace in Address book, Account manager, or Composition [ [e-mail]] displays wrongly with added quotes when composing [" foo"@bar.com], causing lost mails and malformed "duplicate" AB entry

Status: VERIFIED DUPLICATE of bug 286760

(MailNews Core :: Address Book, defect)

Description

[Mark Banner (:standard8)]

+++ This bug was initially created as a clone of Bug #286760 +++

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; es-AR; rv:1.7.6) Gecko/20050226 Firefox/1.0.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; es-ES; rv:1.7.5) Gecko/20041219

When you start an email address with a space in address book, and later you try to send a mail to this address, Thunderbird will try to send the mail to an incorrect email address and the mail will be lost.

Example: correo electrónico:(Mail adress:) <[e-mail] >. Later I try
to send a mail to [e-mail] clicking in the adress book and I'll get: 
Para:(To:) <"example"@example.com> and, obviously, [e-mail] won't
receive the mail.

Reproducible: Always

Steps to Reproduce:
1.Go to "Libreta de direcciones/Nueva targeta" (Adress book/New card) and fill
it with a mail adress ending with a space.
2.Try to send a mail to this adress. 


Actual Results:  
The email address that will appear in the "To: " field will not match and the mail
will be lost.

Expected Results:  
The mail should be sent correctly.

Comment 1

[Mark Banner (:standard8)]

Note: bug 286760 has fixed the trailing space case, this bug is for the leading space case.

Comment 2

[JC Reyes]

Is this still a bug? If so, my partner and I would like to work on this for a school project. I'm assuming you, Mark Banner, will be the one reviewing our patch?

Comment 3

[Thomas D. (:thomas8)]

(In reply to JC Reyes from comment #2)
> Is this still a bug? If so, my partner and I would like to work on this for
> a school project. I'm assuming you, Mark Banner, will be the one reviewing
> our patch?

^^

Comment 4

[JC Reyes]

How would I go about assigning this bug to myself? Or does a mod need to do that?

Comment 5

[Thomas D. (:thomas8)]

(In reply to JC Reyes from comment #4)
> How would I go about assigning this bug to myself? Or does a mod need to do
> that?

Needs editbug privs, so I've gladly done that for you.

(In reply to JC Reyes from comment #2)
> Is this still a bug?

Yes.

> If so, my partner and I would like to work on this for
> a school project.

Most welcome :)

> I'm assuming you, Mark Banner, will be the one reviewing
> our patch?

Yes, since Mark reviewed the patch of twin bug 286760 which fixed the trailing space case, he'll certainly review the patch here, too.

Comment 6

[Thomas D. (:thomas8)]

FTR:

Some scenarios of this bug (involving display names, I think) change invalid email address from AB
[ [e-mail]] (without brackets)
into
<" foo"@bar.com> in composition's addressing widget.

According to http://en.wikipedia.org/wiki/E-mail_address#Valid_email_addresses, 
<" foo"@bar.com> is actually a valid email address.

I conclude that the approach which I believe was taken in bug 286760 (post-fix addresses by trimming before sending) cannot just be copied for this bug.

For the patch here, I'm quite sure that we need to validate the email address  *before* it gets added to the composition, because after composition has munged invalid [ [e-mail]] from AB into valid <" foo"@bar.com>, we have no way of telling after the fact if that strange, but valid address was intended or not. So I think we are already pretty near those preventative UI validations which I requested in Bug 286760 Comment 42.

I don't fully understand the current patch of Bug 286760 (attachment 727672 [details] [diff] [review]), but I suspect it doesn't cover all cases of trailing space. Furthermore, the patch has been backed out as it crashes per Bug 286760 Comment 41.

I think the first step here should be clear STR, Actual result, Expected result, to find out which of the many cases of leading whitespace corruption we're trying to cover, and how.

Comment 7

[Mark Banner (:standard8)]

Hi JC, yes this is still an issue, thanks for your interest.

I'd suggest that you might want to start by looking at the patch on bug 286760 - that will give you some pointers as to where the code and unit tests are. You might even want to see if you can isolate the random crash that was introduced with that bug.

(In reply to Thomas D. from comment #6)
> I conclude that the approach which I believe was taken in bug 286760
> (post-fix addresses by trimming before sending) cannot just be copied for
> this bug.

The same fix should be able to used, as this code is called from multiple places, not just before sending.

> For the patch here, I'm quite sure that we need to validate the email
> address  *before* it gets added to the composition, because after
> composition has munged invalid [ [e-mail]] from AB into valid <"
> foo"@bar.com>, we have no way of telling after the fact if that strange, but
> valid address was intended or not. So I think we are already pretty near
> those preventative UI validations which I requested in Bug 286760 Comment 42.

As above - the code is called from multiple places, so fixing this in a similar way to bug 286760 should fix the issue.

> I think the first step here should be clear STR, Actual result, Expected
> result, to find out which of the many cases of leading whitespace corruption
> we're trying to cover, and how.

As JC is new to Thunderbird, the best thing is to start off with adding a test to the unit test, and writing a fix for that. Once we've got that, we can test and file bugs for any new issues, but knowing how the code is structured, I'd be surprised if there were cases where we'd need to "pre-handle" addresses.

Comment 8

[JC Reyes]

To be clear, we want to copy the solution from Bug 286760, but resolving the crash in Bug 286760, Comment 41? Also, what did your most recent patch for Bug 286760 do?

Comment 9

[JC Reyes]

Attachment: Similar patch to 286760

A reassignment of the variable "length" after the while-loop was added because it is called in several other places within the same scope. Could this may be the cause of crashes in 286760?

Comment 10

[Thomas D. (:thomas8)]

(In reply to JC Reyes from comment #9)
> Created attachment 754616 [details] [diff] [review]
> Similar patch to 286760
> 
> A reassignment of the variable "length" after the while-loop was added
> because it is called in several other places within the same scope. Could
> this may be the cause of crashes in 286760?

^^

Comment 11

[Mark Banner (:standard8)]

I'm sorry for looking at this much earlier. I'm pretty sure I tried various options when I originally looked at the patch in addition to change length.

However, given the work ongoing in bug 858337, I think it might be easier and safer to fix these two bugs once that one completes, as then we'll be in a much less crash-likely javascript environment.

Comment 12

[Magnus Melin [:mkmelin]]

Dupeing to bug 286760, it's easier to track as one.

Comment 14

[Serge Gautherie (:sgautherie)]

V.Duplicate