Last Comment Bug 761654 - "Always load remote content from [address]" link in the preview pane is failing when displayName isn't set
: "Always load remote content from [address]" link in the preview pane is faili...
Status: RESOLVED FIXED
: regression
Product: Thunderbird
Classification: Client Software
Component: Mail Window Front End (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 17.0
Assigned To: Magnus Melin
:
Mentors:
Depends on:
Blocks: 562048 684865
  Show dependency treegraph
 
Reported: 2012-06-05 09:22 PDT by Jonathan Kamens
Modified: 2012-08-21 05:49 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
fixed


Attachments
EML file that demonstrates the problem (for me) (467 bytes, message/rfc822)
2012-06-10 19:32 PDT, Jonathan Kamens
no flags Details
proposed fix (2.57 KB, patch)
2012-06-16 11:31 PDT, Magnus Melin
no flags Details | Diff | Review
proposed fix, v2 (2.56 KB, patch)
2012-06-17 02:59 PDT, Magnus Melin
mconley: review+
iann_bugzilla: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Review

Description Jonathan Kamens 2012-06-05 09:22:01 PDT
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 2012-06-07 00:19:12 PDT
(In reply to Jonathan Kamens from comment #0)
See bug 729742.
Comment 2 Jonathan Kamens 2012-06-07 10:09:14 PDT
That bug does not seem to have anything to do with this one.
Comment 3 Magnus Melin 2012-06-10 12:18:28 PDT
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 2012-06-10 19:32:55 PDT
Created attachment 631793 [details]
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 example@example.com 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: example@example.com" to "From: Example <example@example.com>", the problem no longer occurs. That should help you quite a bit to track down the issue. :-)
Comment 5 Jeff Grossman 2012-06-16 07:31:43 PDT
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 2012-06-16 11:30:32 PDT
I think this is from bug 684865.
Comment 7 Magnus Melin 2012-06-16 11:31:35 PDT
Created attachment 633834 [details] [diff] [review]
proposed fix
Comment 8 Magnus Melin 2012-06-16 11:32:35 PDT
Comment on attachment 633834 [details] [diff] [review]
proposed fix

Er, ignore this :/
Comment 9 Jeff Grossman 2012-06-16 12:16:10 PDT
(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 2012-06-17 02:12:22 PDT
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 2012-06-17 02:59:40 PDT
Created attachment 633877 [details] [diff] [review]
proposed fix, v2

Correct patch
Comment 12 Jonathan Kamens 2012-06-17 12:29:52 PDT
(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 2012-06-17 22:13:57 PDT
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) 2012-06-18 06:05:46 PDT
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) - (Away until June 29th) 2012-06-18 08:21:43 PDT
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 2012-06-18 11:30:16 PDT
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 2012-07-01 16:39:42 PDT
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 2012-07-01 23:31:47 PDT
Imo it's a waste of time. mconley?
Comment 19 Jonathan Kamens 2012-08-09 15:33:14 PDT
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 2012-08-09 22:13:52 PDT
Comment on attachment 633877 [details] [diff] [review]
proposed fix, v2

mconley, see previous comments
Comment 21 Mark Banner (:standard8) 2012-08-10 02:09:32 PDT
(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) - (Away until June 29th) 2012-08-20 12:23:35 PDT
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 2012-08-20 12:51:19 PDT
http://hg.mozilla.org/comm-central/rev/db9f634cc302 -> FIXED
Comment 24 Magnus Melin 2012-08-20 22:26:36 PDT
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.)

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