264757 - [FIX]"Save Image as" does not respect either MIME type or content-disposition

Status: VERIFIED FIXED

(SeaMonkey :: General, defect, P1)

Description

[Robert Kaiser]

Another regression from bug 160454 - but the discussion in bug 263697 implies
this is a different issue:

[from bug 263697 comment #15]
I just figured that on a photo gallery like in
http://inconstruction.kairo.at/?d=g&i=51&m=v (and I have lots of those), when
you "Save image as...", you get a .htm filename suggested for saving though
Mozilla already knows the file is an image and decoded it that way, the image
has a correct image/* MIME type in content type set and has a proposed file name
in content-disposition. That all (at least MIME type and file name) was worth
something until HEAD requests got removed, now we seems to ignore all of that
and users end up saving a file they can't open (as file managers may link
applications by extension by default).
This is clearly a big regression and we shouldn't even ship a beta with that
one, so we should dig up a solution quite soon.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So the question is, where can we get the header from?  There is no way to get
the underlying channel from imgIRequest (though it has a pointer to the thing).

I'm a little curious what other browsers do here.  Opera 7.5 seems to get the
content-disposition filename.  Konqueror 3.0.5 (a little old, yes) gives
"index.html".  What does IE do?

Comment 2

[Martijn Wargers (dead)]

IE6 gives "untitled" and as image type it suggests .bmp, which is the only
format you can save it as.

Comment 3

[Robert Kaiser]

Interesting. IE6 suggests a random 8-char filename with JPEG file type on my
WinXP system...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Fun.

Getting the type off the image is easy (what IE6 on kairo's system does).  But
even getting the header would be easy if imagelib exposed it.  I'd like to
change imagelib to do so.

Comment 5

[Robert Kaiser]

Konqueror 3.3 still suggests index.html
Seamonkey 1.8a4 gets the Content-Disposition filename (from the HEAD request)

IMO, if we have access to the info (and we should), we should try to get the
content-disposition filename, and fall back to at least using the file' MIME
type (if there's no content-disposition header sent).

Comment 6

[Boris Zbarsky [:bzbarsky]]

> IMO, if we have access to the info (and we should)

Agreed that we should.  The question is how.

> and fall back to at least using the file' MIME type

Also agreed.  This part is easy.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So it looks like the simple fix won't work.  imgRequest drops its mChannel
pointer in OnStopRequest, presumably to avoid leaks.  Given that at least some
of our channels don't drop their nsIStreamListener after calling OnStopRequest
on it (ftp, the LDAP channel in directory/xpcom, the viewsource channel; I
didn't look at the mailnews ones), it doesn't seem to be clearly documented that
channels should release their listener after OnStopRequest (indeed, nsIChannel
says nothing of the sort).

So we can't hold on to the channel in imgRequest.

Given that, I'm going to copy only the information we actually need here (the
content-disposition header) out of the channel in OnStartRequest and stash it
away in the imgRequest.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Although, that approach isn't going to work for, say, background images. 
Perhaps what the save as code should do is try to grab the cache entry for the
URI when there is no document and grab the content-disposition from there the
way Page Info does content type at
http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/pageInfo.js#933
?

This won't work if the page is expired from cache, but we can still pass in the
type for images (though not background images), so at least we'll have a
reasonable extension.

Darin?  Biesi?  Thoughts?

Comment 9

[Marcel B.]

Even after a CGI-generated 301 pointing straight to the image, causing the
address bar to show only the image filename, it still wants to save the image as
<cgi-filename>.htm ... From a user point of view, picking the right filename
seems so obvious in this case (since it's right there in the address bar), but I
guess it isn't that simple in code.

One example of this case : 
http://www.kimble.org/cgi-bin/pic?kimorg-20010429-121819.jpg
Which redirects to: 
http://www.kimble.org/pictures/1000x656/kimorg-20010429-121819.jpg
But still wants to save as:
pic.htm

Note that this kind of redirect can occur within elements of pages as well and
then cause similar problems when trying to save.

Even the "Element Properties" dialog shows the correct filename in the
"Location" field, BUT the contents of the "Alternate text" might be interesting:
"The image “http://www.kimble.org/pictures/1000x656/kimorg-20010429-121819.jpg”
cannot be displayed, because it contains errors."

Well, clearly it CAN be displayed and perhaps the code that displays it (and
clearly recognizes the correct filetype), should have a vote in the filetype
assumed when saving...? It doesn't seem like a good idea to parse the file
header all over again when the result is available elsewhere.

Note that IE6, in this particular case, is also unable to save it properly. When
the above CGI is in an IMG tag, it saves the image as pic.jpg (close, but not
quite), whereas when the CGI is in the address bar (being replaced by the jpg as
in Mozilla), it reverts back to untitled.bmp

Comment 10

[Robert Kaiser]

(In reply to comment #9)
> Even the "Element Properties" dialog shows the correct filename in the
> "Location" field, BUT the contents of the "Alternate text" might be interesting:
> "The image �http://www.kimble.org/pictures/1000x656/kimorg-20010429-121819.jpg�
> cannot be displayed, because it contains errors."

Don't care about that alt text, as it's always like that for images loaded as
the main page content to be sure that this text is shown instead _if_ the image
can't be displayed (that's why it's the _aternate_ text).

Comment 11

[Boris Zbarsky [:bzbarsky]]

> From a user point of view, picking the right filename
> seems so obvious in this case (since it's right there in the address bar)

How are we supposed to know that one of the random server params is the filename?

> Even the "Element Properties" dialog shows the correct filename in the
> "Location" field

Note that the URL bar has the post-redirect URI too.  That's what the "element
properties" thing is treating as the image URI and that's what it uses.

Since "save image as" goes based on the actual link to the image, not the final
URI after redirects and such gunk, it doesn't have that information.

Being able to do "pic.jpg" should be much simpler than being able to do the
content-disposition filename..

Comment 12

[Marcel B.]

Right, misinterpreted the ALT text there, sorry.

> How are we supposed to know that one of the random server params is
> the filename?

We're not, but as you say the address bar contains the post-redirect URI where
filename was no longer a parameter in the above case. Never mind.

> Since "save image as" goes based on the actual link to the image, not the
> final URI after redirects and such gunk, it doesn't have that information.

Perhaps it should. The final URI is a bit more likely to contain the actual
filename than the initial link.

> Being able to do "pic.jpg" should be much simpler than being able to do
> the content-disposition filename..

Yes, doing "pic.jpg" would be a great start. Users who save more than one pic
from the same gallery would love the full names as well, but that could be
attended to later or not at all (if it was too troublesome). I imagine initial
and final URI could be matched against the content type.

Well anyway, all I wanted to do was post an extra example, another angle.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Perhaps it should. The final URI is a bit more likely to contain the actual
> filename than the initial link.

The final URI is not available in general (see the background images part of
comment 8).

Comment 14

[Boris Zbarsky [:bzbarsky]]

So ideally, we'd have a reasonable way to get the type of an image from imagelib
given the URI.  I suppose we could just loadImage it, but I'd like something
that only returns data if the image is already loaded....

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Too late for 1.8b1.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch, using pav's new API

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -w for ease of review

Comment 18

[[e-mail]]

Comment on attachment 179546 [details] [diff] [review]
Proposed patch, using pav's new API

>+function saveImageURL(aURL, aFileName, aFilePickerTitleKey, aShouldBypassCacke,
Well, at least you consistently misspelled Cache ;-) Should we be trying to get
the type and disposition when we should bypass the cache?

>+  // XXXbz and we don't want to use content.document as the document
>+  // in the latter case?  Why not?
Don't you just love those unused code paths ;-)

>+        aDocument.defaultView
>+                 .QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                 .getInterface(Components.interfaces.nsIDOMWindowUtils)
Out of interest, where is this documented?

>+ * @param aContentDisposition The caller-provided content-disposition header
>+ *         to use.
>+ * @param aContentType The caller-provided content-type to use
Any chance of documenting aReferrer? Pretty please?

>+        // Note: getReferrer wants our chrome document, not the actual
>+        // target document; it handles getting that itself.
>+        saveURL( this.linkURL(), this.linkText(), null, true,
>+                 getReferrer(document) );
When I looked at this internalSave already appears to use getReferrer(document)
by default, so just leave this null.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> at least you consistently misspelled Cache

Oops.  Fixed.

> Should we be trying to get the type and disposition when we should bypass the
> cache?

That's a good question....  Probably not.

> Don't you just love those unused code paths ;-)

No.  If it's unused, we should remove it.  Want me to?

> Out of interest, where is this documented?

http://lxr.mozilla.org/seamonkey/source/dom/public/idl/base/nsIDOMWindowUtils.idl#41

We need better ways of documenting such things, though... :(

> Any chance of documenting aReferrer? Pretty please?

Done.

> so just leave this null.

Good catch.  Done.

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

>When I looked at this internalSave already appears to use getReferrer(document)
>by default, so just leave this null.

actually please don't leave it null, I want to change that, since it causes bug
258185

Comment 21

[Boris Zbarsky [:bzbarsky]]

Ok.  Biesi, do you want an updated patch?

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #21)
> Ok.  Biesi, do you want an updated patch?

that'd be nice

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -w

Comment 25

[[e-mail]]

Comment on attachment 179598 [details] [diff] [review]
Same as diff -w

Actually I was referring to the ability to obtain an interface requestor from
the defaultView property.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Ah... That's the "defaultView is actually DOMWindow" thing... 

Fix checked in (last night).  I've filed bug 289067 on Firefox to implement this.

Comment 27

[Robert Kaiser]

it works! yay!

Comment 28

[Boris Zbarsky [:bzbarsky]]

Er.. I meant to mark this fixed with comment 26.

Comment 29

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED for me too (in addition to Robert's comment 27) by testing his
site and seeing Mozilla suggest, and correct save,
file:///C:/Documents%20and%20Settings/Owner/Desktop/Star%20Trek-%20The%20Next%20Generation_2289_thumb.jpg

Build: 2005-04-23-05 on Windows XP.