444759 - Thunderbird does not display pictures with APPLICATION_OCTET_STREAM content-type

Status: RESOLVED WORKSFORME

(Thunderbird :: Mail Window Front End, defect)

Description

[Michel Loiseleur]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9) Gecko/2008061712 Fedora/3.0-1.fc9 Firefox/3.0
Build Identifier: 

Hi,

   We have received some html emails where some pictures were displaying and others not. After some investigation, it appears that they were valid picture files but their "Content-Type" field was set to "application/octet-stream".
  
  Current stable & unstable version of Thunderbird refuse to give them an url, so they cannot be displayed. 
  We have made a small patch which fixes this issue, you'll see it in attachments.



Reproducible: Always

Steps to Reproduce:
1. Open Thunderbird
2. Open an html email with a valid picture attached as "application/octet-stream".

Actual Results:  
The picture is not displayed, despite all others pictures are displayed.

Expected Results:  
The picture, if it is a valid picture file, should be displayed. 

This is what Evolution does, for instance.

Comment 1

[Michel Loiseleur]

Attachment: A sample fix for this issue.

Would you please review the patch, please ?

Thanks,

Comment 2

[Michel Loiseleur]

Attachment: A sample email file, which shows the problem.

You can open it in Evolution and see that it's displayed correctly.
You can open it in Thunderbird and see that only the first image is displayed.

If the patch is applied, the 2 pictures are displayed.

Note: You may need to send it via a mta like postfix if you want to test in Thunderbird, since it was unable to reopen correctly the file saved with it.

Comment 3

[Joshua Cranmer [:jcranmer]]

Michael, please follow the steps under <http://developer.mozilla.org/en/docs/Getting_your_patch_in_the_tree>.

Comment 4

[Michel Loiseleur]

Joshua,
  I have tried to follow the steps indicated. I have added M. bienvenu for a "review", in the attachment. I hope it's correct.

Regards,

Comment 5

[Michel Loiseleur]

Attachment: New patch, with better indent

Comment 6

[Michel Loiseleur]

Attachment: New patch, with better indent

Hi,

  Would you please tell me if this patch is correct ?

Thanks,

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 330209 [details] [diff] [review]
New patch, with better indent

afaik hex226 isn't a valid reviewer for this code. bienvenu is probably the best choice.

Note: you have some tabs in this file, you'll need to change them to spaces, using 2-space indentation for the new bits.

Comment 8

[Michel Loiseleur]

Attachment: 3rd version of the patch : all tabs are gone

Hi,

  I've finally found a good way to remove all tabs. I hope this one will be fine.

Regards,

Comment 9

[David :Bienvenu]

Comment on attachment 330215 [details] [diff] [review]
3rd version of the patch : all tabs are gone

+        override_content_type = opts->file_type_fn (name, opts->stream_closure);
+        PR_FREEIF(name);

You can just use PR_Free here.

And here:

+  if (override_content_type)
+    PR_FREEIF(override_content_type);

PR_Free already checks for a null pointer, so you don't need to check, or use PR_FREEIF (the difference between PR_Free and PR_FREEIF is basically that PR_FREEIF nulls out the result pointer, which you don't need)

If you can fix those nits (and I realize this is code that was basically copied), then I'll be OK with this patch. Thx for the patch!

Comment 10

[Michel Loiseleur]

Attachment: 4th Version, with correct PR_FREE/PR_FREEIF

Hi,
  I had some doubts about this, since the file I modified contains other calls to PR_FREEIF despite a "if" before.
  I'm happy to learn that it was not the good way to do.
  Please find the new version attached to this request, I hope it will be good.

Regards,

Comment 11

[David :Bienvenu]

Comment on attachment 331057 [details] [diff] [review]
4th Version, with correct PR_FREE/PR_FREEIF

I think it's just PR_Free, not PR_FREE

Comment 12

[Michel Loiseleur]

Attachment: 5th version, with the correct PR_Free call

Damn. Another mistake. I hope I have fixed it.
  Is this one good enough for integration ?

Thanks,

Comment 13

[Tobias Koenig]

Hej,

any chance to remove the 'image/' check, so that the patch works for other mimetypes as well? Especially PDF files are often sent as octect-stream but with the correct .pdf extension.

Ciao,
Tobias

Comment 14

[Magnus Melin [:mkmelin]]

... you could have mentioned that that's bug 453805. Bug 66677 is also very related.

Comment 15

[Michel Loiseleur]

It's not hard to remove the 'image/' check, but I don't know if M. bienvenu (or anyone else with commits rights) will accept it. 
So I won't update it without some further discussion.

Regards,
Michel,

Comment 16

[Jean-Christophe Coynel]

Just to point out that this is still and issue on Thunderbird 3.0b3.

As opposed to 2.x, where the inline images where not displayed and shown as attachments with an "unknown type" icon, in 3.0b3 the icons are correct, but two issues remain :
 - no mime type is available for the image, hence you are prompted each time what to do with them (and no way to remember, as it is greyed out)
 - the images are still not displayed inline

Comment 17

[Ludovic Hirlimann [:Usul]]

Bienvenu - ping for the review.

Comment 18

[David :Bienvenu]

thx for the new patch; I'll give it a quick test and if everything is ok, try to land this when the tree opens...

Comment 19

[David :Bienvenu]

Comment on attachment 332225 [details] [diff] [review]
5th version, with the correct PR_Free call

Thx for the patch, but this works fine in 3.1, so I don't think this patch is needed. If you have a test case that's broken in 3.1.x or trunk builds, please let me know, thx!

Comment 20

[Magnus Melin [:mkmelin]]

The testcase works fine for me too.
->WFM