Closed Bug 761654 Opened 8 years ago Closed 8 years ago

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

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set
normal

Tracking

(thunderbird15+ fixed, thunderbird16 fixed)

RESOLVED FIXED
Thunderbird 17.0
Tracking Status
thunderbird15 + fixed
thunderbird16 --- fixed

People

(Reporter: jik, Assigned: mkmelin)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

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
(In reply to Jonathan Kamens from comment #0)
See bug 729742.
That bug does not seem to have anything to do with this one.
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
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. :-)
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
I think this is from bug 684865.
Blocks: 684865
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: x86_64 → All
Summary: "Always load remote content from [address]" link in the preview pane is failing → "Always load remote content from [address]" link in the preview pane is failing when displayName isn't set
Attached patch proposed fix (obsolete) — Splinter Review
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #633834 - Flags: review?(mconley)
Comment on attachment 633834 [details] [diff] [review]
proposed fix

Er, ignore this :/
Attachment #633834 - Flags: review?(mconley)
(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.
It's only a problem if the sender didn't set a name for himself, which i think is fairly rare.
Attached patch proposed fix, v2Splinter Review
Correct patch
Attachment #633834 - Attachment is obsolete: true
Attachment #633877 - Flags: review?(mconley)
Attachment #633877 - Flags: review?(iann_bugzilla)
(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.
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.
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?
Blocks: 562048
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.
Attachment #633877 - Flags: review?(mconley) → review-
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
Imo it's a waste of time. mconley?
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 on attachment 633877 [details] [diff] [review]
proposed fix, v2

mconley, see previous comments
Attachment #633877 - Flags: review- → review?(mconley)
(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.
Attachment #633877 - Flags: review?(iann_bugzilla) → review+
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!
Attachment #633877 - Flags: review?(mconley) → review+
http://hg.mozilla.org/comm-central/rev/db9f634cc302 -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 17.0
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.)
Attachment #633877 - Flags: approval-comm-beta?
Attachment #633877 - Flags: approval-comm-aurora?
Attachment #633877 - Flags: approval-comm-beta?
Attachment #633877 - Flags: approval-comm-beta+
Attachment #633877 - Flags: approval-comm-aurora?
Attachment #633877 - Flags: approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.