532603 - plain text attachment with application/octet-stream Content-Type does not shown inline

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Jie Zhang]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.5) Gecko/20091123 Iceweasel/3.5.5 (like Firefox/3.5.5; Debian-3.5.5-1)
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.5) Gecko/20091203 Shredder/3.0

There are some clients send out all attachments with a content-type of application/octet-stream, e.g. attaching a patch in Gmail. When receiving such emails, Thunderbird does not show plain text attachment inline.

Reproducible: Always

Steps to Reproduce:
1. Use a Gmail account to send a patch to another email address
2. Use Thunderbird for that email address to receive that email and open it.

Actual Results:  
See the attached patch is not shown inline.

Expected Results:  
See the attached patch is shown inline.

Comment 1

[Jie Zhang]

Attachment: The proposed patch

Comment 2

[Jie Zhang]

I attached the patch I proposed. 

The comment says:

  /* There are some clients send out all attachments with a content-type
   of application/octet-stream.  So, if we have an octet-stream attachment,
   try to guess what type it really is based on the file extension.  I HATE
   that we have to do this...
  */

And do the guessing for the following three cases of content_type:

    (!content_type ||
     !PL_strcasecmp(content_type, APPLICATION_OCTET_STREAM) ||
     !PL_strcasecmp(content_type, UNKNOWN_CONTENT_TYPE)))

But when we got a override_content_type, it's only applied for the two cases:

       if ( (!content_type) ||
            (content_type && (!PL_strcasecmp(content_type, UNKNOWN_CONTENT_TYPE))) )

So what the comment says is not done actually for APPLICATION_OCTET_STREAM. This patch fixes it.

It's against the latest trunk. I tested it on thunderbird3 rc2. It works fine for me.

Comment 3

[Ludovic Hirlimann [:Usul]]

Jie thanks for the patch. You'll need to follow a few more steps in order to get you're patch in our tree - see https://developer.mozilla.org/en/comm-central#Requirements . You'll need to ask for review and super-review as your code touches MailNews.

Comment 4

[Jie Zhang]

Now I have asked for review and super-review.

Comment 5

[David :Bienvenu]

Comment on attachment 415817 [details] [diff] [review]
The proposed patch

looks reasonable to me.

Comment 6

[[e-mail]]

Comment on attachment 415817 [details] [diff] [review]
The proposed patch

These notes are not errors with the patch.

>       override_content_type = opts->file_type_fn (name, opts->stream_closure);
Eww, this should really use the MIME(!) service...

>       if (override_content_type && (PL_strcasecmp(override_content_type, UNKNOWN_CONTENT_TYPE)))
If override_content_type was ever UNKNOWN_CONTENT_TYPE it would leak instantly,

>-          PR_FREEIF(override_content_type);
...even if we actually used matching allocators and deallocators...

Comment 7

[Jie Zhang]

(In reply to comment #6)
> (From update of attachment 415817 [details] [diff] [review])
> These notes are not errors with the patch.
> 
> >       override_content_type = opts->file_type_fn (name, opts->stream_closure);
> Eww, this should really use the MIME(!) service...
> 
I think this piece of code (determining the real type via extension file name) exists because MIME service fails here.

> >       if (override_content_type && (PL_strcasecmp(override_content_type, UNKNOWN_CONTENT_TYPE)))
> If override_content_type was ever UNKNOWN_CONTENT_TYPE it would leak instantly,
> 
> >-          PR_FREEIF(override_content_type);
> ...even if we actually used matching allocators and deallocators...

There is another

PR_FREEIF(override_content_type);

near the end of the function.

But you just remind me. Should we free content_type before assigning to it?

      if (override_content_type && (PL_strcasecmp(override_content_type, UNKNOWN_CONTENT_TYPE)))
      {
        PR_FREEIF(content_type);
        content_type = override_content_type;
      }

Comment 8

[Jie Zhang]

Ping?

Comment 9

[David :Bienvenu]

fix checked in - in future, you can add the checkin-needed keyword and someone should check it in. Thx again for the fix.

Comment 10

[[e-mail]]

(In reply to comment #7)
> There is another
> 
> PR_FREEIF(override_content_type);
> 
> near the end of the function.
Thanks for pointing that out.

> But you just remind me. Should we free content_type before assigning to it?
No, we don't own it.

Comment 11

[Jie Zhang]

Thanks for committing for me. Is there a chance for it to be in Thunderbird 3.0.x? Do you think it is a good idea to backport to Thunderbird 2? If yes, I can do the backpirting.

Comment 12

[David :Bienvenu]

It will be in TB 3.1, and we're trying to have a much shorter release cycle for 3.1, e.g., late March, early April for 3.1, so I think that should be sufficient. That's not much later than 3.01 or the next release of 2.0x...

Comment 13

[Jie Zhang]

That's fine. Thanks.

Comment 14

[Neville Hillyer]

Can I assume that this update will allow images to be displayed inline when they are incorrectly described as follows?

Content-type: application/octet-stream; name="photo.jpg"

Comment 15

[David :Bienvenu]

yes, that's the idea, iirc.