Attached vCard throws error message last is undefined

RESOLVED FIXED in Thunderbird 15.0

Status

Thunderbird
Mail Window Front End
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Alexander Bergmann, Assigned: squib)

Tracking

({regression})

unspecified
Thunderbird 15.0
x86
All
regression
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
If I open a message with an attached vCard, I get the following error message in the Error Console of Thunderbird:

Error: last is undefined
Source File: chrome://messenger/content/msgHdrViewOverlay.js
Line: 629

1. Create a New Profile
2. Add POP or IMAP account
3. Create a New Message and attach a vCard ("File -> Attach -> vCard")
4. "File -> Send Later"
5. Open the message in the Outbox ("Local Folders -> Outbox")
6. Open the Error Console ("Tools -> Error Console")

Arch Linux / Thunderbird 7.0.1

Comment 1

6 years ago
(In reply to myaddons from comment #0)
Is it possible to attach the vCard file in question so we can test it?
(Reporter)

Comment 2

6 years ago
I get the error message with every vCard, that is attached to the message. If you follow the steps in my first comment, you can add any value in the vCard.

It might help to open the "Account Settings", select "Attach my vCard to messages" and click "Edit Card", e.g.: "FirstName: John", "LastName: Doe", "Email: info@example.com"
Confirming I'm seeing the following :
Error: last is undefined
Source File: chrome://messenger/content/msgHdrViewOverlay.js
Line: 632

with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:9.0a2) Gecko/20111026 Thunderbird/9.0a2
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Keywords: regression
(Assignee)

Comment 4

5 years ago
Regressed when we added file sizes. This should be easy to fix.
Assignee: nobody → squibblyflabbetydoo
Blocks: 559559
Status: NEW → ASSIGNED
(Assignee)

Comment 5

5 years ago
Created attachment 623871 [details] [diff] [review]
Fix this

Ok, here's the fix. I'm testing this by adding two attachments: a regular one and a vcard. Without the fix, the vcard's size will overwrite the regular attachment's size. With the fix, the size is correct.
Attachment #623871 - Flags: review?(mconley)
Comment on attachment 623871 [details] [diff] [review]
Fix this

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

Jim,

You're more of an expert on this attachment stuff (and libmime) than I am, but I'm a little wary of stashing that skipped value for each attachment.

My initial instinct is to store a hashtable that contains the URI's of attachments that we skip. I guess the boolean flag feels fragile, like it exposes us to edge cases - but I can't really back that instinct up with any contrivance.

Again, however, you know this code better than I do. If you think the boolean is safe, let me know, and I'll give 'er the ol' r+.

-Mike
Comment on attachment 623871 [details] [diff] [review]
Fix this

Like I said in IRC, the only thing I think this needs is an explanation for why we can trust skipAttachment.

Other than that, I'm happy with this.
Attachment #623871 - Flags: review?(mconley) → review+
(Assignee)

Comment 8

5 years ago
I've been thinking about this a little bit more, and perhaps we should be hiding the vCard at the libmime level. There's an upside and a downside to this:

The upside: we'll be able to show the vCard as a regular attachment if parsing failed, which would fix bug 583572.

The downside: we won't be able to detect that there's a vCard at all if we display it inline. I think that's ok though, since we could just make sure *not* to display the vCard inline for the JS MIME emitter (which is what Gloda uses).

mconley, asuth: opinions on the above?
No longer blocks: 559559
(Assignee)

Updated

5 years ago
Blocks: 559559
Short answer: Sounds good.  The JS MIME Emitter should keep being told about the vcards as attachments as that is how we get a URL so consumers can stream them and that way there are no subtle API changes.


Bigger Picture / longer answer:

Gloda requirement-wise-speaking, I don't think we put any thought into the treatment of vcards other than consistency with the message reader.  My own personal experience with vcards is that the people who send them with their emails always send them with their emails (as disposition: attachment), and so treating them as attachments and showing the paperclip is not helpful because it's always on for that sender and therefore conveys no useful information.  Except for the attachment search extensions experiments that I believe are dead-ish, the information is not otherwise used.

I think gloda should filter vcards out of its attachment list itself (in a separate bug, if someone wants to file it and Blake agrees); it's logic that should be separate from MIME processing.  In the case of the message reader, its logic is baked into libmime already, so it would not be crazy to make the change there.
I'm actually in favor of always displaying the vcard inline, as long as we manage to parse it, regardless of whether View > Display Attachments Inline is on or not. Jim, with your current patch, if the user has View > Display Attachments Inline *off*, they will totally miss out on the vcard, and will fail to realize that the user has attached one. Maybe we should still show it as an attachment in that case?

(Not sure what's the right behavior, just my two cents :)).
(In reply to Mike Conley (:mconley) from comment #6)
> Comment on attachment 623871 [details] [diff] [review]
> Fix this
> 
> Review of attachment 623871 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Jim,
> 
> You're more of an expert on this attachment stuff (and libmime) than I am,
> but I'm a little wary of stashing that skipped value for each attachment.
> 
> My initial instinct is to store a hashtable that contains the URI's of
> attachments that we skip. I guess the boolean flag feels fragile, like it
> exposes us to edge cases - but I can't really back that instinct up with any
> contrivance.
> 
> Again, however, you know this code better than I do. If you think the
> boolean is safe, let me know, and I'll give 'er the ol' r+.
> 
> -Mike

The bad case would be handleAttachment throwing an exception for some obscure reason before we could reach the line that says "this.skipAttachment = false". In that case, as long as a call to handleAttachment is *always* followed by a call to addAttachmentField (do we even have that invariant?), we would end up showing an attachment that we intended not to show, so I believe it's safe. This sounds less future-proof than a hash-table, though :)
(Assignee)

Comment 12

5 years ago
(In reply to Jonathan Protzenko [:protz] from comment #10)
> I'm actually in favor of always displaying the vcard inline, as long as we
> manage to parse it, regardless of whether View > Display Attachments Inline
> is on or not.

That might be doable, but I'll have to look more at how libmime handles the "Display Attachments Inline" option. Perhaps what we should do here is check in the current patch while I fiddle around with handling this in libmime instead?

> Jim, with your current patch, if the user has View > Display > Attachments Inline *off*,
> they will totally miss out on the vcard, and will fail to realize that the user has
> attached one.

The vCard is only hidden from the attachment pane if display inline attachments is on and if message bodies are set to show original HTML, which is how it used to be.

> Maybe we should still show it as an attachment in that case?

The downside of always showing a vCard as an attachment is that many senders *always* send a vCard, and thus the attachment icon in the thread pane becomes useless for those senders.

(In reply to Jonathan Protzenko [:protz] from comment #11)
> The bad case would be handleAttachment throwing an exception for some
> obscure reason before we could reach the line that says "this.skipAttachment
> = false".

The only part that might throw is already in a try/catch block, so we should be safe.

> In that case, as long as a call to handleAttachment is *always*
> followed by a call to addAttachmentField (do we even have that invariant?),

Yes: http://mxr.mozilla.org/comm-central/source/mailnews/mime/src/mimemoz2.cpp#695
(In reply to Jim Porter (:squib) from comment #12)
> > Maybe we should still show it as an attachment in that case?
> 
> The downside of always showing a vCard as an attachment is that many senders
> *always* send a vCard, and thus the attachment icon in the thread pane
> becomes useless for those senders.

It's worth noting that the gloda indexing process will clear the attachment flags on headers when it judges a message to have no actual attachments.  Obviously, it's better to not rely on that at all, but it is a band-aid that's frequently around (and may impact testing by hand, whereas it will never happen in mozmill tests.)
(Assignee)

Comment 14

5 years ago
(In reply to Andrew Sutherland (:asuth) from comment #13)
> It's worth noting that the gloda indexing process will clear the attachment
> flags on headers when it judges a message to have no actual attachments. 
> Obviously, it's better to not rely on that at all, but it is a band-aid
> that's frequently around (and may impact testing by hand, whereas it will
> never happen in mozmill tests.)

True, though my understanding is that the gloda pass will just hide the paperclip when the attachment list would have zero elements. If we changed vCards to show in the list, it wouldn't strip the paperclip. Of course, we could enhance gloda to strip the paperclip from emails with only a vCard attached...
(Assignee)

Comment 15

5 years ago
Thinking about it more, I've decided we should just check in the current patch, since 1) it's done, 2) it's simple, and 3) it fixes a bug. People interested in making vCard handling smarter should consult bug 757710, where I've laid out some ideas.
That sounds right :). I'll comment on bug 757710.
(Assignee)

Comment 17

5 years ago
Checked in: http://hg.mozilla.org/comm-central/rev/d0c13c84ee7e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 15.0
You need to log in before you can comment on or make changes to this bug.