Closed Bug 532603 Opened 10 years ago Closed 10 years ago

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

Categories

(MailNews Core :: MIME, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jzhang918, Assigned: jzhang918)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

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.
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.
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.
Assignee: nobody → jzhang918
Status: UNCONFIRMED → NEW
Component: General → MIME
Ever confirmed: true
Product: Thunderbird → MailNews Core
QA Contact: general → mime
Attachment #415817 - Flags: superreview?(dmose)
Attachment #415817 - Flags: review?(bienvenu)
Now I have asked for review and super-review.
Comment on attachment 415817 [details] [diff] [review]
The proposed patch

looks reasonable to me.
Attachment #415817 - Flags: review?(bienvenu) → review+
Attachment #415817 - Flags: superreview?(dmose) → superreview?(neil)
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...
Attachment #415817 - Flags: superreview?(neil) → superreview+
(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;
      }
Ping?
fix checked in - in future, you can add the checkin-needed keyword and someone should check it in. Thx again for the fix.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(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.
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.
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...
That's fine. Thanks.
Depends on: 538407
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"
yes, that's the idea, iirc.
Depends on: 587660
You need to log in before you can comment on or make changes to this bug.