761654 - "Always load remote content from [address]" link in the preview pane is failing when displayName isn't set

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Jonathan Kamens]

In trunk (updated and built from the repository today), when I get an email with embedded images from a sender whose remote content is not trusted and isn't in my address book, and I click the "Always load remote content from [address]" link in the preview pane to add them to my address book with remote images enabled, the add contact window that pops up doesn't have the email address filled in properly.

This works fine in Thunderbird 12.0.1.

I see this error in my error console when I click on the link (don't know if it's relevant);

Timestamp: 06/05/2012 12:17:24 PM
Error: TypeError: gEditCard.card.displayName is null
Source File: chrome://messenger/content/addressbook/abCardOverlay.js
Line: 115

Comment 1

[Hashem Masoud]

(In reply to Jonathan Kamens from comment #0)
See bug 729742.

Comment 2

[Jonathan Kamens]

That bug does not seem to have anything to do with this one.

Comment 3

[Magnus Melin [:mkmelin]]

WFM on linux/trunk. Maybe there's something special about the mail?

I do wonder why this is checking length and just not if it's falsy
http://mxr.mozilla.org/comm-central/source/mail/components/addrbook/content/abCardOverlay.js#118

Comment 4

[Jonathan Kamens]

Attachment: EML file that demonstrates the problem (for me)

The attached message, when dropped into an IMAP folder, reproduces the issue for me, and I hope it will for you as well, assuming that you don't have [e-mail] in your address book. I tested this against 16.0a1 on Linux updated from comm-central and recompiled within the last few hours, running in safe mode.

You are correct that this does not occur for every message, but it does seem to occur for this one.

Note that if you change "From: [e-mail]" to "From: Example <[e-mail]>", the problem no longer occurs. That should help you quite a bit to track down the issue. :-)

Comment 5

[Jeff Grossman]

I just noticed this with the Windows Nightly also.  Not sure when it started but I am running the 6/16/2012 build.  The first two emails I read did not display the images correctly so I went to add them to my address book using the link (Always load remote content from ...), and it does not automatically populate the address book entry anymore with the email address and you get an error saving because there is nothing in the address card.  I am also getting the same error in the Error Console as the original poster.

Timestamp: 6/16/2012 7:24:07 AM
Error: TypeError: gEditCard.card.displayName is null
Source File: chrome://messenger/content/addressbook/abCardOverlay.js
Line: 118

Comment 6

[Magnus Melin [:mkmelin]]

I think this is from bug 684865.

Comment 7

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 633834 [details] [diff] [review]
proposed fix

Er, ignore this :/

Comment 9

[Jeff Grossman]

(In reply to Magnus Melin from comment #6)
> I think this is from bug 684865.

That bug had a fix checked in on 5/14/2012.  I have a hard time believing I have not used that link in a month.  The first time it has stopped working for me was on today's nightly (6/16/2012).  I use it all the time to add senders to my collected addresses address book so their remote content is displayed.

Comment 10

[Magnus Melin [:mkmelin]]

It's only a problem if the sender didn't set a name for himself, which i think is fairly rare.

Comment 11

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2

Correct patch

Comment 12

[Jonathan Kamens]

(In reply to Magnus Melin from comment #10)
> It's only a problem if the sender didn't set a name for himself, which i
> think is fairly rare.

I'm not sure what you mean by "fairly rare." I'm also not sure why it's relevant whether a bug is "fairly rare" when it is an obvious bug and a regression.

I've run into this problem several times since it crept its way into the source tree.

A very crude survey of my recent email shows that about 8% of the senders do not have names set in the From: lines.

Comment 13

[Magnus Melin [:mkmelin]]

I don't think it matters if it's rare or not, but it explains why it might not have been noticed earlier. Bug 684865 changed it so no name = null, and not "" like it was earlier. Anyway, we have a patch.

Comment 14

[:Irving Reid (No longer working on Firefox)]

ewong, I hate to throw another wrinkle into bug 562048, but can you check your new notification implementation and make sure it doesn't have the same problem?

Comment 15

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

Comment on attachment 633877 [details] [diff] [review]
proposed fix, v2

Review of attachment 633877 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/addrbook/content/abCardOverlay.js
@@ +114,5 @@
>        gEditCard.card.displayName = window.arguments[0].displayName;
>        // if we've got a display name, don't generate
>        // a display name (and stomp on the existing display name)
>        // when the user types a first or last name
> +      if (gEditCard.card.displayName)

Whoa, hold up - so at this point, we know that "displayName" was in window.arguments[0]...and we've set gEditCard.card.displayName to be equal to window.arguments[0].displayName...

So the only case where gEditCard.card.displayName.length is not available is if a non-string was passed in to displayName when this dialog was opened.

I think we're wallpapering over the symptom - if I'm right, then I think we should be fixing the part that sends the erroneous displayName to this dialog.

Comment 16

[Magnus Melin [:mkmelin]]

Yes, displayName=null is passed as argument now (since bug 684865, see comment 13), but i don't see anything wrong with that. Why would null be special cased, as opposed to any other "value"?

If you really want i can skip including it from http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#2807

Comment 17

[Ian Neal]

There is also:
http://mxr.mozilla.org/comm-central/source/suite/mailnews/mailContextMenus.js#422
http://mxr.mozilla.org/comm-central/source/suite/mailnews/mailWindowOverlay.js#2699

Should all callers be doing the check or get the called function doing the check?

Comment 18

[Magnus Melin [:mkmelin]]

Imo it's a waste of time. mconley?

Comment 19

[Jonathan Kamens]

It has been more than a month since there was motion on this bug.

It's a regression. It works in Thunderbird 14.

It seems like it's less important to fix it "right" at this point than it is to FIX IT so that Thunderbird 15 doesn't get released with this regression in it.

Comment 20

[Magnus Melin [:mkmelin]]

Comment on attachment 633877 [details] [diff] [review]
proposed fix, v2

mconley, see previous comments

Comment 21

[Mark Banner (:standard8)]

(In reply to Jonathan Kamens from comment #19)
> It seems like it's less important to fix it "right" at this point than it is
> to FIX IT so that Thunderbird 15 doesn't get released with this regression
> in it.

If you think it is something serious, that is a regression, that we need to fix for a particular release, please use the tracking-thunderbird<nn> flag and set it to ? and include a comment as to why you think it is serious. Then drivers will know to review it and track it appropriately. Thanks.

Comment 22

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

Comment on attachment 633877 [details] [diff] [review]
proposed fix, v2

Review of attachment 633877 [details] [diff] [review]:
-----------------------------------------------------------------

Alright, let's take it. Thanks for the patch!

Comment 23

[Magnus Melin [:mkmelin]]

http://hg.mozilla.org/comm-central/rev/db9f634cc302 -> FIXED

Comment 24

[Magnus Melin [:mkmelin]]

Comment on attachment 633877 [details] [diff] [review]
proposed fix, v2

[Approval Request Comment]
Regression caused by (bug #): bug 684865 
User impact if declined: clicking allow remote content fails if sender doesn't have a display name
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky): small patch with low risk

(Please land if approved.)

Comment 25

[Mark Banner (:standard8)]

Checked in:

https://hg.mozilla.org/releases/comm-aurora/rev/c2fc73061520
https://hg.mozilla.org/releases/comm-beta/rev/cbfc7d94b03d