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.

Now I have asked for review and super-review.

Comment on attachment 415817 [details] [diff] [review] The proposed patch looks reasonable to me.

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

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

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

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.