696414 - Attached vCard throws error message last is undefined

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Alexander Bergmann]

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

[Hashem Masoud]

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

Comment 2

[Alexander Bergmann]

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: [e-mail]"

Comment 3

[Ludovic Hirlimann [:Usul]]

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

Comment 4

[Jim Porter (:squib)]

Regressed when we added file sizes. This should be easy to fix.

Comment 5

[Jim Porter (:squib)]

Attachment: 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.

Comment 6

[Mike Conley (: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 7

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

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.

Comment 8

[Jim Porter (:squib)]

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?

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 10

[Jonathan Protzenko [:protz]]

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 :)).

Comment 11

[Jonathan Protzenko [:protz]]

(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 :)

Comment 12

[Jim Porter (:squib)]

(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

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

(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.)

Comment 14

[Jim Porter (:squib)]

(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...

Comment 15

[Jim Porter (:squib)]

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.

Comment 16

[Jonathan Protzenko [:protz]]

That sounds right :). I'll comment on bug 757710.

Comment 17

[Jim Porter (:squib)]

Checked in: http://hg.mozilla.org/comm-central/rev/d0c13c84ee7e