529735 - double clicking on a .eml message doesn't show remote images

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Eric Moore]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.5) Gecko/20091102 Firefox/3.5.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.5) Gecko/20091117 Thunderbird/3.0

I'm using build 2 of Thunderbird 3.0RC1 under Vista. I saved a HTML message with remote images as a .eml file. The sender is listed in the address book, "allow remote content" is checked, and I can see the remote images.

I exited Thunderbird and double clicked on the .eml file in windows explorer. It brought up a standalone window that displays the message contents. It didn't display any remote images. The placeholders for the missing images are different than what I'm used to but are consistent with other messages where remote content is disabled.

Clicking on "To view this email as a web page, go here." worked fine (displayed the remote images etc.) though the web page still contained "To view this email as a web page, go here.".

I get the same results if I use File -> Open Saved Message instead.


Reproducible: Always

Comment 1

[[:Aureliano Buendía]]

Confirmed

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.5) Gecko/20091118 Lightning/1.0pre Shredder/3.0.1pre ID:20091118034000

Comment 2

[Ludovic Hirlimann [:Usul]]

testcase pretty please ?

Comment 3

[[:Aureliano Buendía]]

Attachment: testcase

here you are! ;-)

Comment 4

[Joe Sabash [:JoeS1]]

I see the problem, but this does not appear to be a regression.
The same symptoms appear using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080915030813 Shredder/3.0b1pre ID:20080915030813

EML files as a way to archive HTML messages have always been problematic (useless)
to me.
I can't find a dup at the moment.

Comment 5

[Mark Banner (:standard8)]

This feels like the content policy not getting the right message header or the right details to work out about the remote image issue. However, I thought standalone eml files got their own dummy message header.

Need to do a bit of debug, but maybe David has an idea as well.

Comment 6

[Joe Sabash [:JoeS1]]

Looks like we are not doing the remote content check at all.
Setting mailnews.message_display.disable_remote_image to false loads most of the remote images in the testcase (One still hangs possibly it's no longer availabe)

Comment 7

[Eric Moore]

Attachment: A multipart/alternative message with remote images saved as a .eml file

Comment 8

[rsx11m]

There is also bug 541862 reflecting comment #6. If you want to concentrate on the address book issue here and handle the general remote-content notification in that other bug it should be confirmed, otherwise duped to this bug.

Comment 10

[Mark Banner (:standard8)]

I tried taking a look at this today.

nsMsgContentPolicy::ShouldAcceptContentForPotentialMsg isn't taking account of the standalone message window and loading eml files.

The obvious thing would be to use the code in nsIMessenger to get a dummy message header:

http://hg.mozilla.org/comm-central/annotate/25e697e42d9a/mailnews/base/src/nsMessenger.cpp#l1473

However, in the content policy we don't have a way of getting the msgWindow that this needs to work (or at least need to get the msgHeaderSink from that).

David, any thoughts?

Comment 11

[David :Bienvenu]

(In reply to comment #10)

> 
> However, in the content policy we don't have a way of getting the msgWindow
> that this needs to work (or at least need to get the msgHeaderSink from that).
> 
> David, any thoughts?

yeah, I think I can fix this. I'll try to write a patch now.

Comment 12

[David :Bienvenu]

Attachment: proposed fix

This should fix it - there are two main issues; one is getting to the dummy msg header in the content policy code, and the other is fixing reload in the stand-alone message window, which was completely broken, so this fix should help with a bunch of issues.

Next up is writing a mozmill test, which will take me some time...

Comment 13

[Jeff Bronks]

Any progress on this bug? It's still there in TB 12.0.1.

I can offer the following extra clues:

1. The missing images are visible in the TB Print Preview, so that works even if the normal email viewer doesn't.

2. If I change the '.eml' extension to '.mht', I can open the file in IE or Firefox (with the UnMHT plugin) and the images display fine, so the EML file itself appears to be intact.

3. If I drag and drop the EML file into my TB inbox, I get a 'Show Remote Content' button in the message header area. Clicking the button shows me the images. 

4. Double-clicking the same EML file in Windows Explorer opens it in a separate TB window that does not have the 'Show Remote Content' button. Therefore I can't see the images.

And yes, I have 'View > Message Body As' set to 'Original HTML', and 'View > Display Attachments Inline' checked, and the sender is in my address book.

Comment 14

[David :Bienvenu]

hmm, looks like this patch was forgotten about. I'll see if it still makes sense.

Comment 15

[David :Bienvenu]

Attachment: de-bitrotted patch

this patch still works. I'll try to figure out how to make the mozmill content policy tests test .eml files

Comment 16

[David :Bienvenu]

Attachment: with mozmill test

Comment 17

[[e-mail]]

Comment on attachment 630000 [details] [diff] [review]
with mozmill test

>-  nsCOMPtr<nsIMsgDBHdr> msgHdr;
>-  rv = GetMsgDBHdrFromURI(resourceURI.get(), getter_AddRefs(msgHdr));
>+  nsCOMPtr<nsIMsgMailNewsUrl> mailnewsUrl(do_QueryInterface(aOriginatorLocation, &rv));
>   NS_ENSURE_SUCCESS(rv, );
> 
>-  nsCOMPtr<nsIMsgMailNewsUrl> mailnewsUrl(do_QueryInterface(aOriginatorLocation, &rv));
>-  NS_ENSURE_SUCCESS(rv, );
>+  nsCOMPtr<nsIMsgDBHdr> msgHdr;
>+  nsCOMPtr<nsIMsgWindow> msgWindow;
>+  rv = GetMsgDBHdrFromURI(resourceURI.get(), getter_AddRefs(msgHdr));
Are you just out to confuse diff by reordering your lines? ;-)

>+  if (NS_FAILED(rv))
>+  {
>+    rv = mailnewsUrl->GetMsgWindow(getter_AddRefs(msgWindow)); 
...
>   // content header bar, so the user can override if they wish.
>   if (*aDecision == nsIContentPolicy::REJECT_REQUEST)
>   {
>-    nsCOMPtr<nsIMsgWindow> msgWindow;
>     (void)mailnewsUrl->GetMsgWindow(getter_AddRefs(msgWindow)); 
I guess it doesn't hurt to fetch the msgWindow twice, but in that case, I'd prefer it if they were separately scoped.

Comment 18

[David :Bienvenu]

(In reply to [e-mail] from comment #17)

> Are you just out to confuse diff by reordering your lines? ;-)
My patch had bit-rotted, so I had to apply it by hand...I think I thought grouping the var decls read better...
> 
> >+  if (NS_FAILED(rv))
> >+  {
> >+    rv = mailnewsUrl->GetMsgWindow(getter_AddRefs(msgWindow)); 
> ...
> >   // content header bar, so the user can override if they wish.
> >   if (*aDecision == nsIContentPolicy::REJECT_REQUEST)
> >   {
> >-    nsCOMPtr<nsIMsgWindow> msgWindow;
> >     (void)mailnewsUrl->GetMsgWindow(getter_AddRefs(msgWindow)); 
> I guess it doesn't hurt to fetch the msgWindow twice, but in that case, I'd
> prefer it if they were separately scoped.

I can do that...

Comment 19

[David :Bienvenu]

Attachment: declare msgWindow in both scopes it's used in.

Comment 20

[[e-mail]]

OK, so with the patch I see the remote content bar, although it doesn't actually work; should it?

Comment 21

[David :Bienvenu]

(In reply to [e-mail] from comment #20)
> OK, so with the patch I see the remote content bar, although it doesn't
> actually work; should it?

Hmm, I thought it worked for me...

Comment 22

[David :Bienvenu]

(In reply to [e-mail] from comment #20)
> OK, so with the patch I see the remote content bar, although it doesn't
> actually work; should it?

Yeah, tried it again with the message I've been using to test this - it worked for me in TB.

Comment 23

[David :Bienvenu]

this is the test message I've been using - https://bugzilla.mozilla.org/attachment.cgi?id=413714

Comment 24

[ryujin]

Any progress on this bug? I can confirm the same behaviour on TB 17 Mac OSX.

Comment 25

[Wayne Mery (:wsmwk)]

(In reply to [e-mail] from comment #20)
> OK, so with the patch I see the remote content bar, although it doesn't
> actually work; should it?

If "show" bar isn't working, perhaps it is one of these show remote content bugs?
https://bugzilla.mozilla.org/buglist.cgi?chfieldto=Now;query_format=advanced;chfieldfrom=2012-06-01;list_id=5737646;short_desc=remote%20;field0-0-0=short_desc;short_desc_type=allwordssubstr;type0-0-0=anywordssubstr;value0-0-0=content%20image;resolution=---;resolution=FIXED;product=MailNews%20Core;product=Thunderbird

Comment 26

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

Unbitrotted the patch, and modified the test a bit.
I can confirm it works, and the show remote content button does show remote images at least in thunderbird.

I wasn't able to get my seamonkey mail build to work at all so can't say if there's still a problem with the button. Either way, surely this would be a prelude to fixing that problem.

Comment 27

[barry cox]

this bug is still there (or is there again) in 17.0.7.

works if the email is:
replied 
forwarded
viewed in browser as html
or
print previewed, but not when launched as .eml file.  no 'show remote content' button is available to force it.

Comment 28

[barry cox]

note that if you set: mailnews.message_display.disable_remote_image to FALSE, then it DOES display the images in the .eml.  

however i think using that workaround displays all images in all emails, so this should still be considered a big imho.

Comment 29

[Magnus Melin [:mkmelin]]

So the reason it doesn't work in suite - even with the patch should be this: LoadMsgWithRemoteContent() -> msgHdrForCurrentMessage(), which is null. In tb gMessageDisplay.displayedMessage is used but that doesn't work in suite - http://mxr.mozilla.org/comm-central/source/suite/mailnews/folderDisplay.js#101

That and bug 911454.

neil: review ping

Comment 30

[angel.tsankov]

Problem (no alert about remote content is shown when opening an .eml file) exists in 24.4.0.

Comment 31

[Magnus Melin [:mkmelin]]

Attachment: bug529735_eml_remote_images.patch

Refreshed, maybe we can get this old patch finally in.

Comment 32

[Magnus Melin [:mkmelin]]

Joshua: review ping

Comment 33

[Kent James (:rkent)]

Comment on attachment 8687716 [details] [diff] [review]
bug529735_eml_remote_images.patch

3 years is probably long enough to be waiting for a review (including the Neil time) Let's take this, looks good to me, LGTM and has been vetted pretty well previously.

Comment 34

[Kent James (:rkent)]

Comment on attachment 8687716 [details] [diff] [review]
bug529735_eml_remote_images.patch

https://hg.mozilla.org/comm-central/rev/6947cca9f5f1

No reason we can't push this forward to TB 45 after a little bake.

Comment 35

[Jorg K (CEST = GMT+2)]

Aurora (TB 46):
https://hg.mozilla.org/releases/comm-aurora/rev/7c15ae6a5caa

Comment 36

[Kent James (:rkent)]

Comment on attachment 8687716 [details] [diff] [review]
bug529735_eml_remote_images.patch

http://hg.mozilla.org/releases/comm-esr45/rev/cd78e4b9ee24